[PATCH v2 00/11] sockets: Attempt to drain the abstract socket swamp

[PATCH v2 00/11] sockets: Attempt to drain the abstract socket swamp

[Markus Armbruster]

In my opinion, the Linux-specific abstract UNIX domain socket feature
introduced in 5.1 should have been rejected.  The feature is niche,
the interface clumsy, the implementation buggy and incomplete, and the
test coverage insufficient.  Review fail.

Fixing the parts we can still fix now is regrettably expensive.  If I
had the power to decide, I'd unceremoniously revert the feature,
compatibility to 5.1 be damned.  But I don't, so here we go.

I'm not sure this set of fixes is complete.  However, I already spent
too much time on this, so out it goes.  Lightly tested.

For additional information, see

    Subject: Our abstract UNIX domain socket support is a mess
    Date: Wed, 28 Oct 2020 13:41:06 +0100
    Message-ID: <87o8kmwmjh.fsf@dusky.pond.sub.org>

v2:
* PATCH 03: Have unix_server_thread_func(), unix_client_thread_func()
  store their parameter in a variable in the hope of communicating its
  type more clearly [Daniel].  No lasting effect, as the former goes
  away in PATCH 5, and the latter is rewritten in PATCH 6.
* PATCH 05: Fix commit message typo [Eric]
* PATCH 07: Improve commit message [Paolo]
* PATCH 08: Fix computation of @tight [Paolo, Eric], and copying of
  @sun_path [Eric]
* PATCH 11: Tweak QAPI schema [Eric], less ugly ifdeffery [Eric],
  fix logic error in use of new saddr_is_tight().

Markus Armbruster (11):
  test-util-sockets: Plug file descriptor leak
  test-util-sockets: Correct to set has_abstract, has_tight
  test-util-sockets: Clean up SocketAddress construction
  test-util-sockets: Factor out test_socket_unix_abstract_one()
  test-util-sockets: Synchronize properly, don't sleep(1)
  test-util-sockets: Test the complete abstract socket matrix
  sockets: Fix default of UnixSocketAddress member @tight
  sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets
  char-socket: Fix qemu_chr_socket_address() for abstract sockets
  sockets: Bypass "replace empty @path" for abstract unix sockets
  sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX

 qapi/sockets.json         |  14 ++--
 chardev/char-socket.c     |  24 +++++-
 chardev/char.c            |   2 +
 tests/test-util-sockets.c | 155 ++++++++++++++++++++------------------
 util/qemu-sockets.c       |  54 ++++++++++---
 5 files changed, 157 insertions(+), 92 deletions(-)

-- 
2.26.2

[PATCH v2 01/11] test-util-sockets: Plug file descriptor leak

[Markus Armbruster]

Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index f6336e0f91..15da867b8f 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -252,6 +252,7 @@ static gpointer unix_server_thread_func(gpointer user_data)
 
     connfd = accept(fd, (struct sockaddr *)&un, &len);
     g_assert_cmpint(connfd, !=, -1);
+    close(connfd);
 
     close(fd);
 
-- 
2.26.2

[PATCH v2 02/11] test-util-sockets: Correct to set has_abstract, has_tight

[Markus Armbruster]

The code tested doesn't care, which is a bug I will fix shortly.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 15da867b8f..9d317e73a6 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -243,7 +243,9 @@ static gpointer unix_server_thread_func(gpointer user_data)
 
     addr.type = SOCKET_ADDRESS_TYPE_UNIX;
     addr.u.q_unix.path = abstract_sock_name;
+    addr.u.q_unix.has_tight = true;
     addr.u.q_unix.tight = user_data != NULL;
+    addr.u.q_unix.has_abstract = true;
     addr.u.q_unix.abstract = true;
 
     fd = socket_listen(&addr, 1, &err);
@@ -267,7 +269,9 @@ static gpointer unix_client_thread_func(gpointer user_data)
 
     addr.type = SOCKET_ADDRESS_TYPE_UNIX;
     addr.u.q_unix.path = abstract_sock_name;
+    addr.u.q_unix.has_tight = true;
     addr.u.q_unix.tight = user_data != NULL;
+    addr.u.q_unix.has_abstract = true;
     addr.u.q_unix.abstract = true;
 
     fd = socket_connect(&addr, &err);
-- 
2.26.2

[PATCH v2 03/11] test-util-sockets: Clean up SocketAddress construction

[Markus Armbruster]

The thread functions build the SocketAddress from global variable
@abstract_sock_name and the tight flag passed as pointer
argument (either NULL or (gpointer)1).  There is no need for such
hackery; simply pass the SocketAddress instead.

While there, dumb down g_rand_int_range() to g_random_int().

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 64 ++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 9d317e73a6..a4792253ba 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,25 +230,15 @@ static void test_socket_fd_pass_num_nocli(void)
 #endif
 
 #ifdef __linux__
-static gchar *abstract_sock_name;
-
 static gpointer unix_server_thread_func(gpointer user_data)
 {
-    SocketAddress addr;
-    Error *err = NULL;
-    int fd = -1;
-    int connfd = -1;
+    SocketAddress *addr = user_data;
+    int fd;
+    int connfd;
     struct sockaddr_un un;
     socklen_t len = sizeof(un);
 
-    addr.type = SOCKET_ADDRESS_TYPE_UNIX;
-    addr.u.q_unix.path = abstract_sock_name;
-    addr.u.q_unix.has_tight = true;
-    addr.u.q_unix.tight = user_data != NULL;
-    addr.u.q_unix.has_abstract = true;
-    addr.u.q_unix.abstract = true;
-
-    fd = socket_listen(&addr, 1, &err);
+    fd = socket_listen(addr, 1, &error_abort);
     g_assert_cmpint(fd, >=, 0);
     g_assert(fd_is_socket(fd));
 
@@ -257,69 +247,67 @@ static gpointer unix_server_thread_func(gpointer user_data)
     close(connfd);
 
     close(fd);
-
     return NULL;
 }
 
 static gpointer unix_client_thread_func(gpointer user_data)
 {
-    SocketAddress addr;
-    Error *err = NULL;
-    int fd = -1;
-
-    addr.type = SOCKET_ADDRESS_TYPE_UNIX;
-    addr.u.q_unix.path = abstract_sock_name;
-    addr.u.q_unix.has_tight = true;
-    addr.u.q_unix.tight = user_data != NULL;
-    addr.u.q_unix.has_abstract = true;
-    addr.u.q_unix.abstract = true;
-
-    fd = socket_connect(&addr, &err);
+    SocketAddress *addr = user_data;
+    int fd;
 
+    fd = socket_connect(addr, &error_abort);
     g_assert_cmpint(fd, >=, 0);
-
     close(fd);
-
     return NULL;
 }
 
 static void test_socket_unix_abstract_good(void)
 {
-    GRand *r = g_rand_new();
+    SocketAddress addr;
 
-    abstract_sock_name = g_strdup_printf("unix-%d-%d", getpid(),
-                                         g_rand_int_range(r, 100, 1000));
+    addr.type = SOCKET_ADDRESS_TYPE_UNIX;
+    addr.u.q_unix.path = g_strdup_printf("unix-%d-%u",
+                                         getpid(), g_random_int());
+    addr.u.q_unix.has_abstract = true;
+    addr.u.q_unix.abstract = true;
 
     /* non tight socklen serv and cli */
+
+    addr.u.q_unix.has_tight = false;
+    addr.u.q_unix.tight = false;
+
     GThread *serv = g_thread_new("abstract_unix_server",
                                  unix_server_thread_func,
-                                 NULL);
+                                 &addr);
 
     sleep(1);
 
     GThread *cli = g_thread_new("abstract_unix_client",
                                 unix_client_thread_func,
-                                NULL);
+                                &addr);
 
     g_thread_join(cli);
     g_thread_join(serv);
 
     /* tight socklen serv and cli */
+
+    addr.u.q_unix.has_tight = true;
+    addr.u.q_unix.tight = true;
+
     serv = g_thread_new("abstract_unix_server",
                         unix_server_thread_func,
-                        (gpointer)1);
+                        &addr);
 
     sleep(1);
 
     cli = g_thread_new("abstract_unix_client",
                        unix_client_thread_func,
-                       (gpointer)1);
+                       &addr);
 
     g_thread_join(cli);
     g_thread_join(serv);
 
-    g_free(abstract_sock_name);
-    g_rand_free(r);
+    g_free(addr.u.q_unix.path);
 }
 #endif
 
-- 
2.26.2

[PATCH v2 04/11] test-util-sockets: Factor out test_socket_unix_abstract_one()

[Markus Armbruster]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 48 ++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index a4792253ba..40ff893e64 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -261,6 +261,24 @@ static gpointer unix_client_thread_func(gpointer user_data)
     return NULL;
 }
 
+static void test_socket_unix_abstract_one(SocketAddress *addr)
+{
+    GThread *serv, *cli;
+
+    serv = g_thread_new("abstract_unix_server",
+                        unix_server_thread_func,
+                        addr);
+
+    sleep(1);
+
+    cli = g_thread_new("abstract_unix_client",
+                       unix_client_thread_func,
+                       addr);
+
+    g_thread_join(cli);
+    g_thread_join(serv);
+}
+
 static void test_socket_unix_abstract_good(void)
 {
     SocketAddress addr;
@@ -272,40 +290,14 @@ static void test_socket_unix_abstract_good(void)
     addr.u.q_unix.abstract = true;
 
     /* non tight socklen serv and cli */
-
     addr.u.q_unix.has_tight = false;
     addr.u.q_unix.tight = false;
-
-    GThread *serv = g_thread_new("abstract_unix_server",
-                                 unix_server_thread_func,
-                                 &addr);
-
-    sleep(1);
-
-    GThread *cli = g_thread_new("abstract_unix_client",
-                                unix_client_thread_func,
-                                &addr);
-
-    g_thread_join(cli);
-    g_thread_join(serv);
+    test_socket_unix_abstract_one(&addr);
 
     /* tight socklen serv and cli */
-
     addr.u.q_unix.has_tight = true;
     addr.u.q_unix.tight = true;
-
-    serv = g_thread_new("abstract_unix_server",
-                        unix_server_thread_func,
-                        &addr);
-
-    sleep(1);
-
-    cli = g_thread_new("abstract_unix_client",
-                       unix_client_thread_func,
-                       &addr);
-
-    g_thread_join(cli);
-    g_thread_join(serv);
+    test_socket_unix_abstract_one(&addr);
 
     g_free(addr.u.q_unix.path);
 }
-- 
2.26.2

[PATCH v2 05/11] test-util-sockets: Synchronize properly, don't sleep(1)

[Markus Armbruster]

The abstract sockets test spawns a thread to listen and accept, and a
second one to connect, with a sleep(1) in between to "ensure" the
former is listening when the latter tries to connect.  Review fail.
Risks spurious test failure, say when a heavily loaded machine doesn't
schedule the first thread quickly enough.  It's also slow.

Listen and accept in the main thread, and start the connect thread in
between.  Look ma, no sleep!  Run time drops from 2s wall clock to a
few milliseconds.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 40 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 40ff893e64..4cedf622f0 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,26 +230,6 @@ static void test_socket_fd_pass_num_nocli(void)
 #endif
 
 #ifdef __linux__
-static gpointer unix_server_thread_func(gpointer user_data)
-{
-    SocketAddress *addr = user_data;
-    int fd;
-    int connfd;
-    struct sockaddr_un un;
-    socklen_t len = sizeof(un);
-
-    fd = socket_listen(addr, 1, &error_abort);
-    g_assert_cmpint(fd, >=, 0);
-    g_assert(fd_is_socket(fd));
-
-    connfd = accept(fd, (struct sockaddr *)&un, &len);
-    g_assert_cmpint(connfd, !=, -1);
-    close(connfd);
-
-    close(fd);
-    return NULL;
-}
-
 static gpointer unix_client_thread_func(gpointer user_data)
 {
     SocketAddress *addr = user_data;
@@ -263,20 +243,26 @@ static gpointer unix_client_thread_func(gpointer user_data)
 
 static void test_socket_unix_abstract_one(SocketAddress *addr)
 {
-    GThread *serv, *cli;
+    int fd, connfd;
+    GThread *cli;
+    struct sockaddr_un un;
+    socklen_t len = sizeof(un);
 
-    serv = g_thread_new("abstract_unix_server",
-                        unix_server_thread_func,
-                        addr);
-
-    sleep(1);
+    fd = socket_listen(addr, 1, &error_abort);
+    g_assert_cmpint(fd, >=, 0);
+    g_assert(fd_is_socket(fd));
 
     cli = g_thread_new("abstract_unix_client",
                        unix_client_thread_func,
                        addr);
 
+    connfd = accept(fd, (struct sockaddr *)&un, &len);
+    g_assert_cmpint(connfd, !=, -1);
+    close(connfd);
+
+    close(fd);
+
     g_thread_join(cli);
-    g_thread_join(serv);
 }
 
 static void test_socket_unix_abstract_good(void)
-- 
2.26.2

[PATCH v2 06/11] test-util-sockets: Test the complete abstract socket matrix

[Markus Armbruster]

The test covers only two out of nine combinations.  Test all nine.
Four turn out to be broken.  Marked /* BUG */.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 87 ++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 24 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 4cedf622f0..f8b6586e70 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,60 +230,99 @@ static void test_socket_fd_pass_num_nocli(void)
 #endif
 
 #ifdef __linux__
+
+#define ABSTRACT_SOCKET_VARIANTS 3
+
+typedef struct {
+    SocketAddress *server, *client[ABSTRACT_SOCKET_VARIANTS];
+    bool expect_connect[ABSTRACT_SOCKET_VARIANTS];
+} abstract_socket_matrix_row;
+
 static gpointer unix_client_thread_func(gpointer user_data)
 {
-    SocketAddress *addr = user_data;
-    int fd;
+    abstract_socket_matrix_row *row = user_data;
+    Error *err = NULL;
+    int i, fd;
 
-    fd = socket_connect(addr, &error_abort);
-    g_assert_cmpint(fd, >=, 0);
-    close(fd);
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        if (row->expect_connect[i]) {
+            fd = socket_connect(row->client[i], &error_abort);
+            g_assert_cmpint(fd, >=, 0);
+        } else {
+            fd = socket_connect(row->client[i], &err);
+            g_assert_cmpint(fd, ==, -1);
+            error_free_or_abort(&err);
+        }
+        close(fd);
+    }
     return NULL;
 }
 
-static void test_socket_unix_abstract_one(SocketAddress *addr)
+static void test_socket_unix_abstract_row(abstract_socket_matrix_row *test)
 {
-    int fd, connfd;
+    int fd, connfd, i;
     GThread *cli;
     struct sockaddr_un un;
     socklen_t len = sizeof(un);
 
-    fd = socket_listen(addr, 1, &error_abort);
+    /* Last one must connect, or else accept() below hangs */
+    assert(test->expect_connect[ABSTRACT_SOCKET_VARIANTS - 1]);
+
+    fd = socket_listen(test->server, 1, &error_abort);
     g_assert_cmpint(fd, >=, 0);
     g_assert(fd_is_socket(fd));
 
     cli = g_thread_new("abstract_unix_client",
                        unix_client_thread_func,
-                       addr);
+                       test);
 
-    connfd = accept(fd, (struct sockaddr *)&un, &len);
-    g_assert_cmpint(connfd, !=, -1);
-    close(connfd);
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        if (test->expect_connect[i]) {
+            connfd = accept(fd, (struct sockaddr *)&un, &len);
+            g_assert_cmpint(connfd, !=, -1);
+            close(connfd);
+        }
+    }
 
     close(fd);
-
     g_thread_join(cli);
 }
 
-static void test_socket_unix_abstract_good(void)
+static void test_socket_unix_abstract(void)
 {
-    SocketAddress addr;
+    SocketAddress addr, addr_tight, addr_padded;
+    abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {
+        { &addr,
+          { &addr_tight, &addr_padded, &addr },
+          { false /* BUG */, true /* BUG */, true } },
+        { &addr_tight,
+          { &addr_padded, &addr, &addr_tight },
+          { false, false /* BUG */, true } },
+        { &addr_padded,
+          { &addr, &addr_tight, &addr_padded },
+          { true /* BUG */, false, true } }
+    };
+    int i;
 
     addr.type = SOCKET_ADDRESS_TYPE_UNIX;
     addr.u.q_unix.path = g_strdup_printf("unix-%d-%u",
                                          getpid(), g_random_int());
     addr.u.q_unix.has_abstract = true;
     addr.u.q_unix.abstract = true;
-
-    /* non tight socklen serv and cli */
     addr.u.q_unix.has_tight = false;
     addr.u.q_unix.tight = false;
-    test_socket_unix_abstract_one(&addr);
 
-    /* tight socklen serv and cli */
-    addr.u.q_unix.has_tight = true;
-    addr.u.q_unix.tight = true;
-    test_socket_unix_abstract_one(&addr);
+    addr_tight = addr;
+    addr_tight.u.q_unix.has_tight = true;
+    addr_tight.u.q_unix.tight = true;
+
+    addr_padded = addr;
+    addr_padded.u.q_unix.has_tight = true;
+    addr_padded.u.q_unix.tight = false;
+
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        test_socket_unix_abstract_row(&matrix[i]);
+    }
 
     g_free(addr.u.q_unix.path);
 }
@@ -330,8 +369,8 @@ int main(int argc, char **argv)
     }
 
 #ifdef __linux__
-    g_test_add_func("/util/socket/unix-abstract/good",
-                    test_socket_unix_abstract_good);
+    g_test_add_func("/util/socket/unix-abstract",
+                    test_socket_unix_abstract);
 #endif
 
 end:
-- 
2.26.2

[PATCH v2 07/11] sockets: Fix default of UnixSocketAddress member @tight

[Markus Armbruster]

An optional bool member of a QAPI struct can be false, true, or absent.
The previous commit demonstrated that socket_listen() and
socket_connect() are broken for absent @tight, and indeed QMP chardev-
add also defaults absent member @tight to false instead of true.

In C, QAPI members are represented by two fields, has_MEMBER and MEMBER.
We have:

            has_MEMBER    MEMBER
    false         true     false
    true          true      true
    absent       false  false/ignore

When has_MEMBER is false, MEMBER should be set to false on write, and
ignored on read.

For QMP, the QAPI visitors handle absent @tight by setting both
@has_tight and @tight to false.  unix_listen_saddr() and
unix_connect_saddr() however use @tight only, disregarding @has_tight.
This is wrong and means that absent @tight defaults to false whereas it
should default to true.

The same is true for @has_abstract, though @abstract defaults to
false and therefore has the same behavior for all of QMP, HMP and CLI.
Fix unix_listen_saddr() and unix_connect_saddr() to check
@has_abstract/@has_tight, and to default absent @tight to true.

However, this is only half of the story.  HMP chardev-add and CLI
-chardev so far correctly defaulted @tight to true, but defaults to
false again with the above fix for HMP and CLI.  In fact, the "tight"
and "abstract" options now break completely.

Digging deeper, we find that qemu_chr_parse_socket() also ignores
@has_tight, leaving it false when it sets @tight.  That is also wrong,
but the two wrongs cancelled out.  Fix qemu_chr_parse_socket() to set
@has_tight and @has_abstract; writing testcases for HMP and CLI is left
for another day.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 chardev/char-socket.c     | 2 ++
 tests/test-util-sockets.c | 6 +++---
 util/qemu-sockets.c       | 4 ++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 95e45812d5..1ee5a8c295 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1439,7 +1439,9 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
         q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
         q_unix->path = g_strdup(path);
+        q_unix->has_tight = true;
         q_unix->tight = tight;
+        q_unix->has_abstract = true;
         q_unix->abstract = abstract;
     } else if (host) {
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index f8b6586e70..7ecf95579b 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -294,13 +294,13 @@ static void test_socket_unix_abstract(void)
     abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {
         { &addr,
           { &addr_tight, &addr_padded, &addr },
-          { false /* BUG */, true /* BUG */, true } },
+          { true, false, true } },
         { &addr_tight,
           { &addr_padded, &addr, &addr_tight },
-          { false, false /* BUG */, true } },
+          { false, true, true } },
         { &addr_padded,
           { &addr, &addr_tight, &addr_padded },
-          { true /* BUG */, false, true } }
+          { false, false, true } }
     };
     int i;
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 38f82179b0..3ceaa81226 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -925,7 +925,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     if (saddr->abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], path, pathlen);
-        if (saddr->tight) {
+        if (!saddr->has_tight || saddr->tight) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {
@@ -985,7 +985,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
     if (saddr->abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], saddr->path, pathlen);
-        if (saddr->tight) {
+        if (!saddr->has_tight || saddr->tight) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {
-- 
2.26.2

[PATCH v2 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

[Markus Armbruster]

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update socket_sockaddr_to_address_unix().  The
function returns a non-abstract socket address for abstract
sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
member must never be null).

The null @path is due to confused code going back all the way to
commit 17c55decec "sockets: add helpers for creating SocketAddress
from a socket".

Add the required special case, and simplify the confused code.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-sockets.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 3ceaa81226..a578c434c2 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1270,10 +1270,20 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
 
     addr = g_new0(SocketAddress, 1);
     addr->type = SOCKET_ADDRESS_TYPE_UNIX;
-    if (su->sun_path[0]) {
-        addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
+#ifdef CONFIG_LINUX
+    if (!su->sun_path[0]) {
+        /* Linux abstract socket */
+        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
+                                        sizeof(su->sun_path) - 1);
+        addr->u.q_unix.has_abstract = true;
+        addr->u.q_unix.abstract = true;
+        addr->u.q_unix.has_tight = true;
+        addr->u.q_unix.tight = salen < sizeof(*su);
+        return addr;
     }
+#endif
 
+    addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
     return addr;
 }
 #endif /* WIN32 */
-- 
2.26.2

[PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Markus Armbruster]

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update qemu_chr_socket_address().  It shows
shows neither @abstract nor @tight.  Fix that.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 chardev/char-socket.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1ee5a8c295..dc1cf86ecf 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
                                s->is_listen ? ",server" : "");
         break;
     case SOCKET_ADDRESS_TYPE_UNIX:
-        return g_strdup_printf("%sunix:%s%s", prefix,
+    {
+        UnixSocketAddress *sa = &s->addr->u.q_unix;
+
+        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
                                s->addr->u.q_unix.path,
+                               sa->has_abstract && sa->abstract
+                               ? ",abstract" : "",
+                               sa->has_tight && sa->tight
+                               ? ",tight" : "",
                                s->is_listen ? ",server" : "");
         break;
+    }
     case SOCKET_ADDRESS_TYPE_FD:
         return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str,
                                s->is_listen ? ",server" : "");
-- 
2.26.2

[PATCH v2 10/11] sockets: Bypass "replace empty @path" for abstract unix sockets

[Markus Armbruster]

unix_listen_saddr() replaces empty @path by unique value.  It obtains
the value by creating and deleting a unique temporary file with
mkstemp().  This is racy, as the comment explains.  It's also entirely
undocumented as far as I can tell.  Goes back to commit d247d25f18
"sockets: helper functions for qemu (Gerd Hoffman)", v0.10.0.

Since abstract socket addresses have no connection with filesystem
pathnames, making them up with mkstemp() seems inappropriate.  Bypass
the replacement of empty @path.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-sockets.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index a578c434c2..671717499f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -877,7 +877,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         return -1;
     }
 
-    if (saddr->path && saddr->path[0]) {
+    if (saddr->path[0] || saddr->abstract) {
         path = saddr->path;
     } else {
         const char *tmpdir = getenv("TMPDIR");
-- 
2.26.2

[PATCH v2 11/11] sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX

[Markus Armbruster]

The abstract socket namespace is a non-portable Linux extension.  An
attempt to use it elsewhere should fail with ENOENT (the abstract
address looks like a "" pathname, which does not resolve).  We report
this failure like

    Failed to connect socket abc: No such file or directory

Tolerable, although ENOTSUP would be better.

However, introspection lies: it has @abstract regardless of host
support.  Easy enough to fix: since Linux provides them since 2.2,
'if': 'defined(CONFIG_LINUX)' should do.

The above failure becomes

    Parameter 'backend.data.addr.data.abstract' is unexpected

I consider this an improvement.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/sockets.json         | 14 ++++++++------
 chardev/char-socket.c     | 22 +++++++++++++++------
 chardev/char.c            |  2 ++
 tests/test-util-sockets.c |  7 ++++---
 util/qemu-sockets.c       | 40 +++++++++++++++++++++++++++++----------
 5 files changed, 60 insertions(+), 25 deletions(-)

diff --git a/qapi/sockets.json b/qapi/sockets.json
index c0c640a5b0..2e83452797 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -74,18 +74,20 @@
 # Captures a socket address in the local ("Unix socket") namespace.
 #
 # @path: filesystem path to use
-# @tight: pass a socket address length confined to the minimum length of the
-#         abstract string, rather than the full sockaddr_un record length
-#         (only matters for abstract sockets, default true). (Since 5.1)
-# @abstract: whether this is an abstract address, default false. (Since 5.1)
+# @abstract: if true, this is a Linux abstract socket address.  @path
+#            will be prefixed by a null byte, and optionally padded
+#            with null bytes.  Defaults to false.  (Since 5.1)
+# @tight: if false, pad an abstract socket address with enough null
+#         bytes to make it fill struct sockaddr_un member sun_path.
+#         Defaults to true.  (Since 5.1)
 #
 # Since: 1.3
 ##
 { 'struct': 'UnixSocketAddress',
   'data': {
     'path': 'str',
-    '*tight': 'bool',
-    '*abstract': 'bool' } }
+    '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' },
+    '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } }
 
 ##
 # @VsockSocketAddress:
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index dc1cf86ecf..213a4c8dd0 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
         break;
     case SOCKET_ADDRESS_TYPE_UNIX:
     {
+        const char *tight = "", *abstract = "";
         UnixSocketAddress *sa = &s->addr->u.q_unix;
 
-        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
-                               s->addr->u.q_unix.path,
-                               sa->has_abstract && sa->abstract
-                               ? ",abstract" : "",
-                               sa->has_tight && sa->tight
-                               ? ",tight" : "",
+#ifdef CONFIG_LINUX
+        if (sa->has_abstract && sa->abstract) {
+            abstract = ",abstract";
+            if (sa->has_tight && sa->tight) {
+                tight = ",tight";
+            }
+        }
+#endif
+
+        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
+                               abstract, tight,
                                s->is_listen ? ",server" : "");
         break;
     }
@@ -1394,8 +1400,10 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     const char *host = qemu_opt_get(opts, "host");
     const char *port = qemu_opt_get(opts, "port");
     const char *fd = qemu_opt_get(opts, "fd");
+#ifdef CONFIG_LINUX
     bool tight = qemu_opt_get_bool(opts, "tight", true);
     bool abstract = qemu_opt_get_bool(opts, "abstract", false);
+#endif
     SocketAddressLegacy *addr;
     ChardevSocket *sock;
 
@@ -1447,10 +1455,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
         q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
         q_unix->path = g_strdup(path);
+#ifdef CONFIG_LINUX
         q_unix->has_tight = true;
         q_unix->tight = tight;
         q_unix->has_abstract = true;
         q_unix->abstract = abstract;
+#endif
     } else if (host) {
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
         addr->u.inet.data = g_new(InetSocketAddress, 1);
diff --git a/chardev/char.c b/chardev/char.c
index 78553125d3..aa4282164a 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -928,6 +928,7 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "logappend",
             .type = QEMU_OPT_BOOL,
+#ifdef CONFIG_LINUX
         },{
             .name = "tight",
             .type = QEMU_OPT_BOOL,
@@ -935,6 +936,7 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "abstract",
             .type = QEMU_OPT_BOOL,
+#endif
         },
         { /* end of list */ }
     },
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 7ecf95579b..67486055ed 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -229,7 +229,7 @@ static void test_socket_fd_pass_num_nocli(void)
 }
 #endif
 
-#ifdef __linux__
+#ifdef CONFIG_LINUX
 
 #define ABSTRACT_SOCKET_VARIANTS 3
 
@@ -326,7 +326,8 @@ static void test_socket_unix_abstract(void)
 
     g_free(addr.u.q_unix.path);
 }
-#endif
+
+#endif  /* CONFIG_LINUX */
 
 int main(int argc, char **argv)
 {
@@ -368,7 +369,7 @@ int main(int argc, char **argv)
 #endif
     }
 
-#ifdef __linux__
+#ifdef CONFIG_LINUX
     g_test_add_func("/util/socket/unix-abstract",
                     test_socket_unix_abstract);
 #endif
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 671717499f..8af0278f15 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -860,10 +860,29 @@ static int vsock_parse(VsockSocketAddress *addr, const char *str,
 
 #ifndef _WIN32
 
+static bool saddr_is_abstract(UnixSocketAddress *saddr)
+{
+#ifdef CONFIG_LINUX
+    return saddr->abstract;
+#else
+    return false;
+#endif
+}
+
+static bool saddr_is_tight(UnixSocketAddress *saddr)
+{
+#ifdef CONFIG_LINUX
+    return !saddr->has_tight || saddr->tight;
+#else
+    return false;
+#endif
+}
+
 static int unix_listen_saddr(UnixSocketAddress *saddr,
                              int num,
                              Error **errp)
 {
+    bool abstract = saddr_is_abstract(saddr);
     struct sockaddr_un un;
     int sock, fd;
     char *pathbuf = NULL;
@@ -877,7 +896,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         return -1;
     }
 
-    if (saddr->path[0] || saddr->abstract) {
+    if (saddr->path[0] || abstract) {
         path = saddr->path;
     } else {
         const char *tmpdir = getenv("TMPDIR");
@@ -887,10 +906,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 
     pathlen = strlen(path);
     if (pathlen > sizeof(un.sun_path) ||
-        (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) {
+        (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
         error_setg(errp, "UNIX socket path '%s' is too long", path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
-                          saddr->abstract ? sizeof(un.sun_path) - 1 :
+                          abstract ? sizeof(un.sun_path) - 1 :
                           sizeof(un.sun_path));
         goto err;
     }
@@ -912,7 +931,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         close(fd);
     }
 
-    if (!saddr->abstract && unlink(path) < 0 && errno != ENOENT) {
+    if (!abstract && unlink(path) < 0 && errno != ENOENT) {
         error_setg_errno(errp, errno,
                          "Failed to unlink socket %s", path);
         goto err;
@@ -922,10 +941,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     un.sun_family = AF_UNIX;
     addrlen = sizeof(un);
 
-    if (saddr->abstract) {
+    if (abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], path, pathlen);
-        if (!saddr->has_tight || saddr->tight) {
+        if (saddr_is_tight(saddr)) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {
@@ -952,6 +971,7 @@ err:
 
 static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 {
+    bool abstract = saddr_is_abstract(saddr);
     struct sockaddr_un un;
     int sock, rc;
     size_t pathlen;
@@ -970,10 +990,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 
     pathlen = strlen(saddr->path);
     if (pathlen > sizeof(un.sun_path) ||
-        (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) {
+        (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
         error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
-                          saddr->abstract ? sizeof(un.sun_path) - 1 :
+                          abstract ? sizeof(un.sun_path) - 1 :
                           sizeof(un.sun_path));
         goto err;
     }
@@ -982,10 +1002,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
     un.sun_family = AF_UNIX;
     addrlen = sizeof(un);
 
-    if (saddr->abstract) {
+    if (abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], saddr->path, pathlen);
-        if (!saddr->has_tight || saddr->tight) {
+        if (saddr_is_tight(saddr)) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {
-- 
2.26.2

Re: [PATCH v2 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

[Eric Blake]

On 11/2/20 3:44 AM, Markus Armbruster wrote:
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix().  The
> function returns a non-abstract socket address for abstract
> sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
> member must never be null).
> 
> The null @path is due to confused code going back all the way to
> commit 17c55decec "sockets: add helpers for creating SocketAddress
> from a socket".
> 
> Add the required special case, and simplify the confused code.
> 
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-sockets.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Eric Blake]

On 11/2/20 3:44 AM, Markus Armbruster wrote:
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update qemu_chr_socket_address().  It shows
> shows neither @abstract nor @tight.  Fix that.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  chardev/char-socket.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1ee5a8c295..dc1cf86ecf 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>                                 s->is_listen ? ",server" : "");
>          break;
>      case SOCKET_ADDRESS_TYPE_UNIX:
> -        return g_strdup_printf("%sunix:%s%s", prefix,
> +    {
> +        UnixSocketAddress *sa = &s->addr->u.q_unix;
> +
> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>                                 s->addr->u.q_unix.path,
> +                               sa->has_abstract && sa->abstract
> +                               ? ",abstract" : "",
> +                               sa->has_tight && sa->tight
> +                               ? ",tight" : "",
>                                 s->is_listen ? ",server" : "");

Gets modified again in 11/11, so I can accept this as a strict
improvement, even if it is not the final form.

Reviewed-by: Eric Blake <eblake@redhat.com>

>          break;
> +    }
>      case SOCKET_ADDRESS_TYPE_FD:
>          return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str,
>                                 s->is_listen ? ",server" : "");
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 01/11] test-util-sockets: Plug file descriptor leak

[Philippe Mathieu-Daudé]

On 11/2/20 10:44 AM, Markus Armbruster wrote:
> Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 11/11] sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX

[Eric Blake]

On 11/2/20 3:44 AM, Markus Armbruster wrote:
> The abstract socket namespace is a non-portable Linux extension.  An
> attempt to use it elsewhere should fail with ENOENT (the abstract
> address looks like a "" pathname, which does not resolve).  We report
> this failure like
> 
>     Failed to connect socket abc: No such file or directory
> 
> Tolerable, although ENOTSUP would be better.
> 
> However, introspection lies: it has @abstract regardless of host
> support.  Easy enough to fix: since Linux provides them since 2.2,
> 'if': 'defined(CONFIG_LINUX)' should do.
> 
> The above failure becomes
> 
>     Parameter 'backend.data.addr.data.abstract' is unexpected
> 
> I consider this an improvement.
> 

Commit message lacks mention of the fact that we are now explicitly not
outputting 'strict' for non-abstract sockets (in fact, that change could
be squashed in 9/11 if you wanted to do it there).  But as this cleans
up the code I mentioned in 9/11, I'll leave it up to Dan if the commit
message needs a tweak; the end result is fine if we don't feel like a v3
spin just for moving hunks around.

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/chardev/char-socket.c
> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>          break;
>      case SOCKET_ADDRESS_TYPE_UNIX:
>      {
> +        const char *tight = "", *abstract = "";
>          UnixSocketAddress *sa = &s->addr->u.q_unix;
>  
> -        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
> -                               s->addr->u.q_unix.path,
> -                               sa->has_abstract && sa->abstract
> -                               ? ",abstract" : "",
> -                               sa->has_tight && sa->tight
> -                               ? ",tight" : "",

Unconditional output if tight is true (which is its stated default)...

> +#ifdef CONFIG_LINUX
> +        if (sa->has_abstract && sa->abstract) {
> +            abstract = ",abstract";
> +            if (sa->has_tight && sa->tight) {
> +                tight = ",tight";
> +            }
> +        }
> +#endif
> +
> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
> +                               abstract, tight,

...vs. the now-nicer conditional where tight is only present if abstract.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/2/20 3:44 AM, Markus Armbruster wrote:
>> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
>> support" neglected to update qemu_chr_socket_address().  It shows
>> shows neither @abstract nor @tight.  Fix that.
>> 
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  chardev/char-socket.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index 1ee5a8c295..dc1cf86ecf 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>>                                 s->is_listen ? ",server" : "");
>>          break;
>>      case SOCKET_ADDRESS_TYPE_UNIX:
>> -        return g_strdup_printf("%sunix:%s%s", prefix,
>> +    {
>> +        UnixSocketAddress *sa = &s->addr->u.q_unix;
>> +
>> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>>                                 s->addr->u.q_unix.path,
>> +                               sa->has_abstract && sa->abstract
>> +                               ? ",abstract" : "",
>> +                               sa->has_tight && sa->tight
>> +                               ? ",tight" : "",
>>                                 s->is_listen ? ",server" : "");
>
> Gets modified again in 11/11, so I can accept this as a strict
> improvement, even if it is not the final form.

You're right, PATCH 11's change is better done here already.  Will tidy
up if I need to respin for some other reason.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

>
>>          break;
>> +    }
>>      case SOCKET_ADDRESS_TYPE_FD:
>>          return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str,
>>                                 s->is_listen ? ",server" : "");
>> 

Re: [PATCH v2 11/11] sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/2/20 3:44 AM, Markus Armbruster wrote:
>> The abstract socket namespace is a non-portable Linux extension.  An
>> attempt to use it elsewhere should fail with ENOENT (the abstract
>> address looks like a "" pathname, which does not resolve).  We report
>> this failure like
>> 
>>     Failed to connect socket abc: No such file or directory
>> 
>> Tolerable, although ENOTSUP would be better.
>> 
>> However, introspection lies: it has @abstract regardless of host
>> support.  Easy enough to fix: since Linux provides them since 2.2,
>> 'if': 'defined(CONFIG_LINUX)' should do.
>> 
>> The above failure becomes
>> 
>>     Parameter 'backend.data.addr.data.abstract' is unexpected
>> 
>> I consider this an improvement.
>> 
>
> Commit message lacks mention of the fact that we are now explicitly not
> outputting 'strict' for non-abstract sockets (in fact, that change could

I trust you mean 'tight'.

> be squashed in 9/11 if you wanted to do it there).

Less churn.  I'll do it if I need to respin.

>                                                     But as this cleans
> up the code I mentioned in 9/11, I'll leave it up to Dan if the commit
> message needs a tweak; the end result is fine if we don't feel like a v3
> spin just for moving hunks around.

Neglecting to mention the change in the commit message isn't *too* bad,
because the change "only" corrects something new in this series, which
makes a future question "why did this go away?" relatively unlikely.

That said, I'm happy to respin if you think it's worthwhile.  Just ask.

> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/chardev/char-socket.c
>> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>>          break;
>>      case SOCKET_ADDRESS_TYPE_UNIX:
>>      {
>> +        const char *tight = "", *abstract = "";
>>          UnixSocketAddress *sa = &s->addr->u.q_unix;
>>  
>> -        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>> -                               s->addr->u.q_unix.path,
>> -                               sa->has_abstract && sa->abstract
>> -                               ? ",abstract" : "",
>> -                               sa->has_tight && sa->tight
>> -                               ? ",tight" : "",
>
> Unconditional output if tight is true (which is its stated default)...
>
>> +#ifdef CONFIG_LINUX
>> +        if (sa->has_abstract && sa->abstract) {
>> +            abstract = ",abstract";
>> +            if (sa->has_tight && sa->tight) {
>> +                tight = ",tight";
>> +            }
>> +        }
>> +#endif
>> +
>> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
>> +                               abstract, tight,
>
> ...vs. the now-nicer conditional where tight is only present if abstract.

Re: [PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Daniel P. Berrangé]

On Tue, Nov 03, 2020 at 07:28:20AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 11/2/20 3:44 AM, Markus Armbruster wrote:
> >> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> >> support" neglected to update qemu_chr_socket_address().  It shows
> >> shows neither @abstract nor @tight.  Fix that.
> >> 
> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  chardev/char-socket.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> >> index 1ee5a8c295..dc1cf86ecf 100644
> >> --- a/chardev/char-socket.c
> >> +++ b/chardev/char-socket.c
> >> @@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
> >>                                 s->is_listen ? ",server" : "");
> >>          break;
> >>      case SOCKET_ADDRESS_TYPE_UNIX:
> >> -        return g_strdup_printf("%sunix:%s%s", prefix,
> >> +    {
> >> +        UnixSocketAddress *sa = &s->addr->u.q_unix;
> >> +
> >> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
> >>                                 s->addr->u.q_unix.path,
> >> +                               sa->has_abstract && sa->abstract
> >> +                               ? ",abstract" : "",
> >> +                               sa->has_tight && sa->tight
> >> +                               ? ",tight" : "",
> >>                                 s->is_listen ? ",server" : "");
> >
> > Gets modified again in 11/11, so I can accept this as a strict
> > improvement, even if it is not the final form.
> 
> You're right, PATCH 11's change is better done here already.  Will tidy
> up if I need to respin for some other reason.

I can squash in the following part of patch 11:

@@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
         break;
     case SOCKET_ADDRESS_TYPE_UNIX:
     {
+        const char *tight = "", *abstract = "";
         UnixSocketAddress *sa = &s->addr->u.q_unix;

-        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
-                               s->addr->u.q_unix.path,
-                               sa->has_abstract && sa->abstract
-                               ? ",abstract" : "",
-                               sa->has_tight && sa->tight
-                               ? ",tight" : "",
+#ifdef CONFIG_LINUX
+        if (sa->has_abstract && sa->abstract) {
+            abstract = ",abstract";
+            if (sa->has_tight && sa->tight) {
+                tight = ",tight";
+            }
+        }
+#endif
+
+        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
+                               abstract, tight,
                                s->is_listen ? ",server" : "");
         break;
     }

but leaving out the CONFIG_LINUX ifdef until Patch 11


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Nov 03, 2020 at 07:28:20AM +0100, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 11/2/20 3:44 AM, Markus Armbruster wrote:
>> >> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
>> >> support" neglected to update qemu_chr_socket_address().  It shows
>> >> shows neither @abstract nor @tight.  Fix that.
>> >> 
>> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  chardev/char-socket.c | 10 +++++++++-
>> >>  1 file changed, 9 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> >> index 1ee5a8c295..dc1cf86ecf 100644
>> >> --- a/chardev/char-socket.c
>> >> +++ b/chardev/char-socket.c
>> >> @@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>> >>                                 s->is_listen ? ",server" : "");
>> >>          break;
>> >>      case SOCKET_ADDRESS_TYPE_UNIX:
>> >> -        return g_strdup_printf("%sunix:%s%s", prefix,
>> >> +    {
>> >> +        UnixSocketAddress *sa = &s->addr->u.q_unix;
>> >> +
>> >> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>> >>                                 s->addr->u.q_unix.path,
>> >> +                               sa->has_abstract && sa->abstract
>> >> +                               ? ",abstract" : "",
>> >> +                               sa->has_tight && sa->tight
>> >> +                               ? ",tight" : "",
>> >>                                 s->is_listen ? ",server" : "");
>> >
>> > Gets modified again in 11/11, so I can accept this as a strict
>> > improvement, even if it is not the final form.
>> 
>> You're right, PATCH 11's change is better done here already.  Will tidy
>> up if I need to respin for some other reason.
>
> I can squash in the following part of patch 11:
>
> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>          break;
>      case SOCKET_ADDRESS_TYPE_UNIX:
>      {
> +        const char *tight = "", *abstract = "";
>          UnixSocketAddress *sa = &s->addr->u.q_unix;
>
> -        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
> -                               s->addr->u.q_unix.path,
> -                               sa->has_abstract && sa->abstract
> -                               ? ",abstract" : "",
> -                               sa->has_tight && sa->tight
> -                               ? ",tight" : "",
> +#ifdef CONFIG_LINUX
> +        if (sa->has_abstract && sa->abstract) {
> +            abstract = ",abstract";
> +            if (sa->has_tight && sa->tight) {
> +                tight = ",tight";
> +            }
> +        }
> +#endif
> +
> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
> +                               abstract, tight,
>                                 s->is_listen ? ",server" : "");
>          break;
>      }
>
> but leaving out the CONFIG_LINUX ifdef until Patch 11

Appreciated!