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

[PATCH 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.

Regardless, I *will* make time for ripping the feature out if we
decide to do that.  Quick & easy way to avoid reviewing this series
*hint* *hint*.

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>

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     |  22 +++++-
 chardev/char.c            |   2 +
 tests/test-util-sockets.c | 155 ++++++++++++++++++++------------------
 util/qemu-sockets.c       |  54 ++++++++++---
 5 files changed, 156 insertions(+), 91 deletions(-)

-- 
2.26.2

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

[Markus Armbruster]

Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34
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 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.

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 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().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 62 +++++++++++++++------------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 9d317e73a6..b1b5628bd5 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,25 +230,14 @@ 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;
+    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(user_data, 1, &error_abort);
     g_assert_cmpint(fd, >=, 0);
     g_assert(fd_is_socket(fd));
 
@@ -257,69 +246,66 @@ 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);
+    int fd;
 
+    fd = socket_connect(user_data, &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 04/11] test-util-sockets: Factor out test_socket_unix_abstract_one()

[Markus Armbruster]

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 b1b5628bd5..77fc51d6f5 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -259,6 +259,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;
@@ -270,40 +288,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 05/11] test-util-sockets: Synchronize properly, don't sleep(1)

[Markus Armbruster]

The abstract sockets test spawns a thread to listen and a 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.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 77fc51d6f5..c2802f69ee 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,25 +230,6 @@ static void test_socket_fd_pass_num_nocli(void)
 #endif
 
 #ifdef __linux__
-static gpointer unix_server_thread_func(gpointer user_data)
-{
-    int fd;
-    int connfd;
-    struct sockaddr_un un;
-    socklen_t len = sizeof(un);
-
-    fd = socket_listen(user_data, 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)
 {
     int fd;
@@ -261,20 +242,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 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 */.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 23 deletions(-)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index c2802f69ee..f8b6586e70 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,59 +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)
 {
-    int fd;
+    abstract_socket_matrix_row *row = user_data;
+    Error *err = NULL;
+    int i, fd;
 
-    fd = socket_connect(user_data, &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);
 }
@@ -329,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 07/11] sockets: Fix default of UnixSocketAddress member @tight

[Markus Armbruster]

QMP chardev-add defaults absent member @tight to false instead of
true.  HMP chardev-add and CLI -chardev correctly default to true.

The previous commit demonstrated that socket_listen() and
socket_connect() are broken for absent @tight.  That explains why QMP
is broken, but not why HMP and CLI work.  We need to dig deeper.

An optional bool member of a QAPI struct can be false, true, or
absent.  In C, we have:

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

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

unix_listen_saddr() and unix_connect_saddr() use member @tight without
checking @has_tight.  This is wrong.

When @tight was set to false as it should be, absent @tight defaults
to false.  Wrong, it should default to true.  This is what breaks QMP.

There is one exception: qemu_chr_parse_socket() leaves @has_tight
false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
why HMP and CLI work.  Same for @has_abstract.

Fix unix_listen_saddr() and unix_connect_saddr() to default absent
@tight to true.

Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reported-by: Kevin Wolf <kwolf@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 05e5c73f9d..c802d5aa0a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -919,7 +919,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 {
@@ -979,7 +979,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 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
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 c802d5aa0a..801c5e3957 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1];
+        return addr;
     }
+#endif
 
+    addr->u.q_unix.path = g_strdup(su->sun_path);
     return addr;
 }
 #endif /* WIN32 */
-- 
2.26.2

[PATCH 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.

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 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.

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 801c5e3957..18c8de8cdb 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -871,7 +871,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 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.

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

diff --git a/qapi/sockets.json b/qapi/sockets.json
index c0c640a5b0..db4019306a 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' } }
+    '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' },
+    '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } }
 
 ##
 # @VsockSocketAddress:
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index dc1cf86ecf..1d2b2efb13 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:
     {
+#ifdef CONFIG_LINUX
         UnixSocketAddress *sa = &s->addr->u.q_unix;
+#endif
 
         return g_strdup_printf("%sunix:%s%s%s%s", prefix,
                                s->addr->u.q_unix.path,
+#ifdef CONFIG_LINUX
                                sa->has_abstract && sa->abstract
                                ? ",abstract" : "",
                                sa->has_tight && sa->tight
                                ? ",tight" : "",
+#else
+                               "", "",
+#endif
                                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 18c8de8cdb..f8553976e6 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -854,10 +854,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;
@@ -871,7 +890,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");
@@ -881,10 +900,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;
     }
@@ -906,7 +925,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;
@@ -916,10 +935,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 {
@@ -946,6 +965,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;
@@ -964,10 +984,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;
     }
@@ -976,10 +996,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 00/11] sockets: Attempt to drain the abstract socket swamp

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

Hi Markus,

On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:

> 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.
>

I also failed (as chardev maintainer..) to not only review but weigh in and
discuss the merits or motivations behind it.

I agree with you. Also the commit lacks motivation behind this "feature".


> 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.
>
> Regardless, I *will* make time for ripping the feature out if we
> decide to do that.  Quick & easy way to avoid reviewing this series
> *hint* *hint*.
>

well, fwiw, I would also take that approach too, to the risk of upsetting
the users. But maybe we can get a clear reason behind it before that
happens. (sorry, I didn't dig the ML archive is such evidence is there, it
should have been in the commit message too)


> 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>
>
> 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     |  22 +++++-
>  chardev/char.c            |   2 +
>  tests/test-util-sockets.c | 155 ++++++++++++++++++++------------------
>  util/qemu-sockets.c       |  54 ++++++++++---
>  5 files changed, 156 insertions(+), 91 deletions(-)
>
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3649 bytes --]

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

[Paolo Bonzini]

On 29/10/20 14:38, Markus Armbruster wrote:
> When @tight was set to false as it should be, absent @tight defaults
> to false.  Wrong, it should default to true.  This is what breaks QMP.

When @has_tight...

Paolo

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

[Paolo Bonzini]

On 29/10/20 14:38, Markus Armbruster wrote:
> +        /* 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 = !su->sun_path[sizeof(su->sun_path) - 1];
> +        return addr;

I think this should be

    addr->u.q_unit.tight = salen < sizeof(*su);

Paolo

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

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 1 +
>  1 file changed, 1 insertion(+)

Only a test bug, but a bug nonetheless, so appropriate for 5.2

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

> 
> 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);
>  
> 

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

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

[Paolo Bonzini]

On 29/10/20 18:39, Paolo Bonzini wrote:
>> When @tight was set to false as it should be, absent @tight defaults
>> to false.  Wrong, it should default to true.  This is what breaks QMP.
> When @has_tight...

Ah, I see what you meant here.  Suggested reword:

---------
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	   false
    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.
---------

Apologies if the last sentence is incorrect. :)

Thanks,

Paolo

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

[Paolo Bonzini]

On 29/10/20 14:38, Markus Armbruster wrote:
> 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.
> 
> Regardless, I *will* make time for ripping the feature out if we
> decide to do that.  Quick & easy way to avoid reviewing this series
> *hint* *hint*.

Apart from the nits pointed out in patch 7 (commit message) and 8 (code),

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks, and don't forget to fix the hole that your head has left in the
wall.

Paolo

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

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> The code tested doesn't care, which is a bug I will fix shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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 03/11] test-util-sockets: Clean up SocketAddress construction

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> 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().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 62 +++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 38 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 04/11] test-util-sockets: Factor out test_socket_unix_abstract_one()

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 48 ++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 28 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 05/11] test-util-sockets: Synchronize properly, don't sleep(1)

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> The abstract sockets test spawns a thread to listen and a accept, and

s/and a/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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 39 +++++++++++++--------------------------
>  1 file changed, 13 insertions(+), 26 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 06/11] test-util-sockets: Test the complete abstract socket matrix

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> The test covers only two out of nine combinations.  Test all nine.
> Four turn out to be broken.  Marked /* BUG */.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------
>  1 file changed, 63 insertions(+), 23 deletions(-)
> 

> -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;
>  
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 07/11] sockets: Fix default of UnixSocketAddress member @tight

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> QMP chardev-add defaults absent member @tight to false instead of
> true.  HMP chardev-add and CLI -chardev correctly default to true.
> 
> The previous commit demonstrated that socket_listen() and
> socket_connect() are broken for absent @tight.  That explains why QMP
> is broken, but not why HMP and CLI work.  We need to dig deeper.
> 
> An optional bool member of a QAPI struct can be false, true, or
> absent.  In C, we have:
> 
> 	    has_MEMBER    MEMBER
>     false         true	   false
>     true	  true	   false
>     absent	 false	false/ignore

I'm not sure the TAB in this table made it very legible (it's hard to
tell if has_MEMBER is the label of column 1 or 2).

Row two is wrong: MEMBER (column 3) is set to true when the QMP code
passed true on the wire.

> 
> When has_MEMBER is false, MEMBER should be set to false on write, and
> ignored on read.
> 
> unix_listen_saddr() and unix_connect_saddr() use member @tight without
> checking @has_tight.  This is wrong.

It generally works if addr was constructed by the same way as the
generated QAPI parser code - but as you demonstrated, in this particular
case, because our construction did not obey the rules of the QAPI
parser, our lack of checking bit us.

> 
> When @tight was set to false as it should be, absent @tight defaults
> to false.  Wrong, it should default to true.  This is what breaks QMP.
> 
> There is one exception: qemu_chr_parse_socket() leaves @has_tight
> false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
> why HMP and CLI work.  Same for @has_abstract.
> 
> Fix unix_listen_saddr() and unix_connect_saddr() to default absent
> @tight to true.
> 
> Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.

At any rate, the fix looks correct:
- as producers, anywhere we hand-construct an addr (rather than using
generated QAPI code), we MUST set both has_MEMBER and MEMBER, including
setting MEMBER to false if has_MEMBER is false, if we want to preserve
the assumptions made in the rest of the code;
- as consumers, rather than relying on the QAPI parsers only setting
MEMBER to true when has_MEMBER is true, we can ensure that has_MEMBER
has priority by checking it ourselves

> +++ b/util/qemu-sockets.c
> @@ -919,7 +919,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 {
> @@ -979,7 +979,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 {
> 

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 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

[Eric Blake]

On 10/29/20 8:38 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
> 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 c802d5aa0a..801c5e3957 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1];

This is questionable - how can you tell from the last byte whether the
name was created as tight or not?

> +        return addr;
>      }
> +#endif
>  
> +    addr->u.q_unix.path = g_strdup(su->sun_path);

This is wrong on at least Linux, where su->sun_path need not be
NUL-terminated (allowing file-system Unix sockets to have one more byte
in their name); you need the strndup that you replaced above, in order
avoid reading beyond the end of the array.

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

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

[Eric Blake]

On 10/29/20 8:38 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  chardev/char-socket.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 

> +++ 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" : "",

Why are we appending ',tight' when it is not abstract?  tight only makes
a difference for abstract sockets, so omitting it for normal sockets
makes more sense.

Or put another way, why are we using 2 bools to represent three sensible
states, instead of a single 3-state enum?

>                                 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" : "");
> 

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

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

[Eric Blake]

On 10/29/20 8:38 AM, Markus Armbruster wrote:
> 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-sockets.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

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

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 801c5e3957..18c8de8cdb 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -871,7 +871,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");
> 

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

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

[Eric Blake]

On 10/29/20 8:38 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ 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)

Do we need to mention that @tight is ignored (or even make it an error)
if @abstract is false?

>  #
>  # Since: 1.3
>  ##
>  { 'struct': 'UnixSocketAddress',
>    'data': {
>      'path': 'str',
> -    '*tight': 'bool',
> -    '*abstract': 'bool' } }
> +    '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' },
> +    '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } }

So we document @abstract before @tight, but declare them in reverse
order.  I guess our doc generator doesn't care?

>  
>  ##
>  # @VsockSocketAddress:
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index dc1cf86ecf..1d2b2efb13 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:
>      {
> +#ifdef CONFIG_LINUX
>          UnixSocketAddress *sa = &s->addr->u.q_unix;
> +#endif
>  
>          return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>                                 s->addr->u.q_unix.path,

Why did we need the #ifdef above, which means we can't we use sa here?

> +#ifdef CONFIG_LINUX
>                                 sa->has_abstract && sa->abstract

I hate mid-()-expression #ifdefs.  If g_strdup_printf() were itself a
macro expansion, things break.  Can you come up with a saner way of
writing this?

>                                 ? ",abstract" : "",
>                                 sa->has_tight && sa->tight
>                                 ? ",tight" : "",
> +#else
> +                               "", "",
> +#endif
>                                 s->is_listen ? ",server" : "");

I suggest:

    const char *tight = "", *abstract = "";
    UnixSocketAddress *sa = &s->addr->u.q_unix;

#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" : "");


> +++ b/util/qemu-sockets.c
> @@ -854,10 +854,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;

Should this also look at abstract?

> +#else
> +    return false;
> +#endif
> +}
> +

Is it any easier to split the patch, first into the introduction of
saddr_is_* and adjusting all clients, and second into adding the 'if' to
the QAPI declaration?

But the idea makes sense.

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

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 AM, Markus Armbruster wrote:
>> The abstract sockets test spawns a thread to listen and a accept, and
>
> s/and a/and/

Yes.

>> 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.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/test-util-sockets.c | 39 +++++++++++++--------------------------
>>  1 file changed, 13 insertions(+), 26 deletions(-)
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 AM, Markus Armbruster wrote:
>> QMP chardev-add defaults absent member @tight to false instead of
>> true.  HMP chardev-add and CLI -chardev correctly default to true.
>> 
>> The previous commit demonstrated that socket_listen() and
>> socket_connect() are broken for absent @tight.  That explains why QMP
>> is broken, but not why HMP and CLI work.  We need to dig deeper.
>> 
>> An optional bool member of a QAPI struct can be false, true, or
>> absent.  In C, we have:
>> 
>> 	    has_MEMBER    MEMBER
>>     false         true	   false
>>     true	  true	   false
>>     absent	 false	false/ignore
>
> I'm not sure the TAB in this table made it very legible (it's hard to
> tell if has_MEMBER is the label of column 1 or 2).

Use of TAB is an accident.

> Row two is wrong: MEMBER (column 3) is set to true when the QMP code
> passed true on the wire.

Pasto, fixing...

Result:

            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.
>> 
>> unix_listen_saddr() and unix_connect_saddr() use member @tight without
>> checking @has_tight.  This is wrong.
>
> It generally works if addr was constructed by the same way as the
> generated QAPI parser code - but as you demonstrated, in this particular
> case, because our construction did not obey the rules of the QAPI
> parser, our lack of checking bit us.
>
>> When @tight was set to false as it should be, absent @tight defaults
>> to false.  Wrong, it should default to true.  This is what breaks QMP.
>> 
>> There is one exception: qemu_chr_parse_socket() leaves @has_tight
>> false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
>> why HMP and CLI work.  Same for @has_abstract.
>> 
>> Fix unix_listen_saddr() and unix_connect_saddr() to default absent
>> @tight to true.
>> 
>> Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.
>
> At any rate, the fix looks correct:
> - as producers, anywhere we hand-construct an addr (rather than using
> generated QAPI code), we MUST set both has_MEMBER and MEMBER, including
> setting MEMBER to false if has_MEMBER is false, if we want to preserve
> the assumptions made in the rest of the code;
> - as consumers, rather than relying on the QAPI parsers only setting
> MEMBER to true when has_MEMBER is true, we can ensure that has_MEMBER
> has priority by checking it ourselves

As long as the instance is built according to the rules, you can
contract has_MEMBER && MEMBER to just MEMBER.  Both mean "true".

However, has_MEMBER && !MEMBER cannot be contracted to !MEMBER.  The
former means "false", the latter means "false or absent".

Doubters, see the table above.

Putting defaults in the schema would let us eliminate the "absent"
state, the has_MEMBER flags, and the bugs that come with them.  Sadly,
this project has been crowded out by more urgent or important work since
forever.

>> +++ b/util/qemu-sockets.c
>> @@ -919,7 +919,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 {
>> @@ -979,7 +979,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 {
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/10/20 18:39, Paolo Bonzini wrote:
>>> When @tight was set to false as it should be, absent @tight defaults
>>> to false.  Wrong, it should default to true.  This is what breaks QMP.
>> When @has_tight...
>
> Ah, I see what you meant here.  Suggested reword:
>
> ---------
> 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	   false
>     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.
> ---------
>
> Apologies if the last sentence is incorrect. :)

Sold (with the table fixed as per Eric's review)!

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/10/20 14:38, Markus Armbruster wrote:
>> +        /* 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 = !su->sun_path[sizeof(su->sun_path) - 1];
>> +        return addr;
>
> I think this should be
>
>     addr->u.q_unit.tight = salen < sizeof(*su);
>
> Paolo

You're right, my code is wrong.

The case "@path just fits" is ambiguous: @tight doesn't matter then.
Your code arbitrarily picks tight=false then.  Picking the default
tight=true would perhaps be a bit nicer.  Not worth complicating the
code.

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 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
>> 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 c802d5aa0a..801c5e3957 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1];
>
> This is questionable - how can you tell from the last byte whether the
> name was created as tight or not?

I plead temporary insanity.  See my reply to Paolo.

>> +        return addr;
>>      }
>> +#endif
>>  
>> +    addr->u.q_unix.path = g_strdup(su->sun_path);
>
> This is wrong on at least Linux, where su->sun_path need not be
> NUL-terminated (allowing file-system Unix sockets to have one more byte
> in their name);

Out of curiosity: is this usage portable?  I tried man pages and SUS, no
luck.

>                 you need the strndup that you replaced above, in order
> avoid reading beyond the end of the array.

You're right.  Prone to allocate a bit more than necessary (always
sizeof(su->sun_path) + 1 bytes), but that doesn't matter.

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 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.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  chardev/char-socket.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>
>> +++ 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" : "",
>
> Why are we appending ',tight' when it is not abstract?  tight only makes
> a difference for abstract sockets, so omitting it for normal sockets
> makes more sense.

We don't bother to reject @tight when @abstract is false.  Not bothering
to suppress it here is consistently careless.

I'm trying to make this pig less wrong, I'm not trying to make it less
ugly.

> Or put another way, why are we using 2 bools to represent three sensible
> states, instead of a single 3-state enum?

Because the QAPI interface got merged without proper review by the QAPI
maintainers?

>>                                 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" : "");
>> 

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 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.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ 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)
>
> Do we need to mention that @tight is ignored (or even make it an error)
> if @abstract is false?

We could make presence of @tight an error unless @abstract is true.  But
again, this series aims for less wrong, not for less ugly.

For me, the description "if false, pad an abstract socket address..."
implies "no effect when the socket address is not abstract".  If you'd
like to suggest a clearer phrasing, go right ahead.

>>  #
>>  # Since: 1.3
>>  ##
>>  { 'struct': 'UnixSocketAddress',
>>    'data': {
>>      'path': 'str',
>> -    '*tight': 'bool',
>> -    '*abstract': 'bool' } }
>> +    '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' },
>> +    '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } }
>
> So we document @abstract before @tight, but declare them in reverse
> order.  I guess our doc generator doesn't care?

It doesn't.  I flipped them in the comment for readability, but
neglected to flop the code.  Flipping them now.

>>  
>>  ##
>>  # @VsockSocketAddress:
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index dc1cf86ecf..1d2b2efb13 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:
>>      {
>> +#ifdef CONFIG_LINUX
>>          UnixSocketAddress *sa = &s->addr->u.q_unix;
>> +#endif
>>  
>>          return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>>                                 s->addr->u.q_unix.path,
>
> Why did we need the #ifdef above, which means we can't we use sa here?

Tunnel vision.  I'll simplify.

>> +#ifdef CONFIG_LINUX
>>                                 sa->has_abstract && sa->abstract
>
> I hate mid-()-expression #ifdefs.  If g_strdup_printf() were itself a
> macro expansion, things break.  Can you come up with a saner way of
> writing this?
>
>>                                 ? ",abstract" : "",
>>                                 sa->has_tight && sa->tight
>>                                 ? ",tight" : "",
>> +#else
>> +                               "", "",
>> +#endif
>>                                 s->is_listen ? ",server" : "");
>
> I suggest:
>
>     const char *tight = "", *abstract = "";
>     UnixSocketAddress *sa = &s->addr->u.q_unix;
>
> #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" : "");

I don't care either way, so I'm taking yours.

>> +++ b/util/qemu-sockets.c
>> @@ -854,10 +854,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;
>
> Should this also look at abstract?

It's used in just two places, both guarded by if (abstract).

I added the helpers only because the code creating a struct sockaddr_un
is duplicated, and de-duplication is too hard to bother due to the
(racy) "if path="" pick one" feature.

>> +#else
>> +    return false;
>> +#endif
>> +}
>> +
>
> Is it any easier to split the patch, first into the introduction of
> saddr_is_* and adjusting all clients, and second into adding the 'if' to
> the QAPI declaration?

I doubt it.  But If you guys think it makes the patch easier to
understand, I'll gladly do it.

> But the idea makes sense.

Thanks!

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

[Daniel P. Berrangé]

On Thu, Oct 29, 2020 at 02:38:28PM +0100, Markus Armbruster wrote:
> The test covers only two out of nine combinations.  Test all nine.
> Four turn out to be broken.  Marked /* BUG */.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------
>  1 file changed, 63 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index c2802f69ee..f8b6586e70 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -230,59 +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;

snip

>  
> -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 } }
> +    };

This is overloading multiple separate tests in one, which is bad for
test result reporting.


> @@ -329,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);

We should instead be registering multiple test funcs, passing in
param to say which scenario to test.  This ensures we still see
the test name reflecting which scenario is being run.


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 03/11] test-util-sockets: Clean up SocketAddress construction

[Daniel P. Berrangé]

On Thu, Oct 29, 2020 at 02:38:25PM +0100, Markus Armbruster wrote:
> 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().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-util-sockets.c | 62 +++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 38 deletions(-)
> 
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index 9d317e73a6..b1b5628bd5 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -230,25 +230,14 @@ 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;

Keep this but as a pointer, and initialize it to "user_data",
so that it is clear what data type this parameter is expected
to be.

> -    Error *err = NULL;
> -    int fd = -1;
> -    int connfd = -1;
> +    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(user_data, 1, &error_abort);

Then keep this as passing "addr"


>  
>  static gpointer unix_client_thread_func(gpointer user_data)
>  {
> -    SocketAddress addr;

Same note here

> -    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);
> +    int fd;
>  
> +    fd = socket_connect(user_data, &error_abort);
>      g_assert_cmpint(fd, >=, 0);

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 00/11] sockets: Attempt to drain the abstract socket swamp

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi Markus,
>
> On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>>
>
> I also failed (as chardev maintainer..) to not only review but weigh in and
> discuss the merits or motivations behind it.
>
> I agree with you. Also the commit lacks motivation behind this "feature".
>
>
>> 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.
>>
>> Regardless, I *will* make time for ripping the feature out if we
>> decide to do that.  Quick & easy way to avoid reviewing this series
>> *hint* *hint*.
>>
>
> well, fwiw, I would also take that approach too, to the risk of upsetting
> the users.

Reverting the feature requires rough consensus and a patch.

I can provide a patch, but let's give everybody a chance to object
first.

>            But maybe we can get a clear reason behind it before that
> happens. (sorry, I didn't dig the ML archive is such evidence is there, it
> should have been in the commit message too)

I just did, and found next to nothing.

This is the final cover letter:

    qemu-sockets: add abstract UNIX domain socket support

    qemu does not support abstract UNIX domain
    socket address. Add this ability to make qemu handy
    when abstract address is needed.

Boils down to "$feature is needed because it's handy when it's needed",
which is less than helpful.

Patch history:

v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03799.html

    This version repurposes @path starting with '@' for abstract
    sockets, always tight.  Only connect, no tests, no documentation.

    R-by Marc-André.

v2: https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03803.html

    Minor cleanup.

    Daniel asks why it's needed, points out listen is missing, and
    suggests the two boolean flags abstract, tight.

v3: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02291.html

    Implement interface proposed by Daniel, default of @tight broken,
    tests (which don't catch the broken default), documentation.

    Eric suggests QAPI schema doc improvements (but doesn't challenge
    the interface).

    R-by Daniel for the code.  He asks for randomized @path in tests.

v4: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04036.html

    Daniel points out style nits in tests.

    Eric suggests a few more QAPI schema doc improvements.

v5: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04144.html

    R-by Daniel for the tests.

v6: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04508.html

    No further review comments.

PR: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg05747.html

    Pull request catches my eye.  The interface looks odd, and I
    challenge @tight.  I silently accept Daniel's defense of it without
    digging deeper.

This is a review failure.  I do not blame the patch submitter.

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/10/20 14:38, Markus Armbruster wrote:
>> 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.
>> 
>> Regardless, I *will* make time for ripping the feature out if we
>> decide to do that.  Quick & easy way to avoid reviewing this series
>> *hint* *hint*.
>
> Apart from the nits pointed out in patch 7 (commit message) and 8 (code),
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Thanks, and don't forget to fix the hole that your head has left in the
> wall.

Thanks for the review, and thanks for cheering my up!

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

[Daniel P. Berrangé]

On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi Markus,
> >
> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> 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.
> >>
> >
> > I also failed (as chardev maintainer..) to not only review but weigh in and
> > discuss the merits or motivations behind it.
> >
> > I agree with you. Also the commit lacks motivation behind this "feature".
> >
> >
> >> 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.
> >>
> >> Regardless, I *will* make time for ripping the feature out if we
> >> decide to do that.  Quick & easy way to avoid reviewing this series
> >> *hint* *hint*.
> >>
> >
> > well, fwiw, I would also take that approach too, to the risk of upsetting
> > the users.
> 
> Reverting the feature requires rough consensus and a patch.
> 
> I can provide a patch, but let's give everybody a chance to object
> first.
> 
> >            But maybe we can get a clear reason behind it before that
> > happens. (sorry, I didn't dig the ML archive is such evidence is there, it
> > should have been in the commit message too)
> 
> I just did, and found next to nothing.
> 
> This is the final cover letter:
> 
>     qemu-sockets: add abstract UNIX domain socket support
> 
>     qemu does not support abstract UNIX domain
>     socket address. Add this ability to make qemu handy
>     when abstract address is needed.
> 
> Boils down to "$feature is needed because it's handy when it's needed",
> which is less than helpful.

Well if you have an existing application that uses abstract sockets,
and you want to connect QEMU to it, then QEMU needs to support it,
or you are forced to interject a proxy between your app and QEMU
which is an extra point of failure and lantency. I was interested
in whether there was a specific application they were using, but
that is largely just from a curiosity POV. There's enough usage of
abstract sockets in apps in general that is it clearly a desirable
feature to enable.

Even if no external app is involved and you're just connecting
together 2 QEMU processes' chardevs, abstract sockets are interesting
because of their differing behaviour to non-abstract sockets.

Most notably non-abstract sockets are tied to the filesystem mount
namespace in Linux, where as abstract sockets are tied to the network
namespace. This is a useful distinction in the container world. Ordinarily
you can't connect QEMUs in 2 separate containers together, because they
always have distinct mount namespaces. There is often the ability to
share network namespaces between containers though, and thus unlock
use of abstract sockets for communications. 

> v2: https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03803.html
> 
>     Minor cleanup.
> 
>     Daniel asks why it's needed, points out listen is missing, and
>     suggests the two boolean flags abstract, tight.
> 
> v3: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02291.html
> 
>     Implement interface proposed by Daniel, default of @tight broken,
>     tests (which don't catch the broken default), documentation.
> 
>     Eric suggests QAPI schema doc improvements (but doesn't challenge
>     the interface).
> 
>     R-by Daniel for the code.  He asks for randomized @path in tests.
> 
> v4: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04036.html
> 
>     Daniel points out style nits in tests.
> 
>     Eric suggests a few more QAPI schema doc improvements.
> 
> v5: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04144.html
> 
>     R-by Daniel for the tests.
> 
> v6: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04508.html
> 
>     No further review comments.
> 
> PR: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg05747.html
> 
>     Pull request catches my eye.  The interface looks odd, and I
>     challenge @tight.  I silently accept Daniel's defense of it without
>     digging deeper.
> 
> This is a review failure.  I do not blame the patch submitter.

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 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

[Eric Blake]

On 10/30/20 4:04 AM, Markus Armbruster wrote:

>>> +    addr->u.q_unix.path = g_strdup(su->sun_path);
>>
>> This is wrong on at least Linux, where su->sun_path need not be
>> NUL-terminated (allowing file-system Unix sockets to have one more byte
>> in their name);
> 
> Out of curiosity: is this usage portable?  I tried man pages and SUS, no
> luck.

On Linux, 'man 7 unix' says:

> BUGS
>        When binding a socket to an address, Linux is one  of  the  implementa‐
>        tions  that  appends a null terminator if none is supplied in sun_path.
>        In most cases  this  is  unproblematic:  when  the  socket  address  is
>        retrieved,  it  will  be  one  byte  longer than that supplied when the
>        socket was bound.  However, there is one case where confusing  behavior
>        can  result: if 108 non-null bytes are supplied when a socket is bound,
>        then the addition of the null terminator takes the length of the  path‐
>        name beyond sizeof(sun_path).  Consequently, when retrieving the socket
>        address (for example, via accept(2)), if the input addrlen argument for
>        the  retrieving  call  is specified as sizeof(struct sockaddr_un), then
>        the  returned  address  structure  won't  have  a  null  terminator  in
>        sun_path.
> 
>        In  addition, some implementations don't require a null terminator when
>        binding a socket (the addrlen argument is used to determine the  length
>        of  sun_path)  and when the socket address is retrieved on these imple‐
>        mentations, there is no null terminator in sun_path.

along with advice on using strnlen and/or overallocation to handle
various cases in a cleaner manner, and the caveat that if you always use
a name smaller than sun_path you can avoid the tricky code (at the
expense of one byte less in your namespace).

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

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

[Markus Armbruster]

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

> On Thu, Oct 29, 2020 at 02:38:25PM +0100, Markus Armbruster wrote:
>> 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().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/test-util-sockets.c | 62 +++++++++++++++------------------------
>>  1 file changed, 24 insertions(+), 38 deletions(-)
>> 
>> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
>> index 9d317e73a6..b1b5628bd5 100644
>> --- a/tests/test-util-sockets.c
>> +++ b/tests/test-util-sockets.c
>> @@ -230,25 +230,14 @@ 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;
>
> Keep this but as a pointer, and initialize it to "user_data",
> so that it is clear what data type this parameter is expected
> to be.

Can do (I don't care for it myself).

[...]

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

[Markus Armbruster]

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

> On Thu, Oct 29, 2020 at 02:38:28PM +0100, Markus Armbruster wrote:
>> The test covers only two out of nine combinations.  Test all nine.
>> Four turn out to be broken.  Marked /* BUG */.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 63 insertions(+), 23 deletions(-)
>> 
>> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
>> index c2802f69ee..f8b6586e70 100644
>> --- a/tests/test-util-sockets.c
>> +++ b/tests/test-util-sockets.c
>> @@ -230,59 +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;
>
> snip
>
>>  
>> -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 } }
>> +    };
>
> This is overloading multiple separate tests in one, which is bad for
> test result reporting.
>
>
>> @@ -329,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);
>
> We should instead be registering multiple test funcs, passing in
> param to say which scenario to test.  This ensures we still see
> the test name reflecting which scenario is being run.

There are hundreds of such test functions in tests/ just waiting for you
to create them ;)

Back to serious.  Before the patch, one test function tested two
scenarios.  The patch adds the missing seven.  Feel free to improve on
top.

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

[Markus Armbruster]

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

> On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> 
>> > Hi Markus,
>> >
>> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> 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.
>> >>
>> >
>> > I also failed (as chardev maintainer..) to not only review but weigh in and
>> > discuss the merits or motivations behind it.
>> >
>> > I agree with you. Also the commit lacks motivation behind this "feature".
>> >
>> >
>> >> 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.
>> >>
>> >> Regardless, I *will* make time for ripping the feature out if we
>> >> decide to do that.  Quick & easy way to avoid reviewing this series
>> >> *hint* *hint*.
>> >>
>> >
>> > well, fwiw, I would also take that approach too, to the risk of upsetting
>> > the users.
>> 
>> Reverting the feature requires rough consensus and a patch.
>> 
>> I can provide a patch, but let's give everybody a chance to object
>> first.

Daniel, do you object, yes or no?

I need to know to avoid wasting even more time.

>> >            But maybe we can get a clear reason behind it before that
>> > happens. (sorry, I didn't dig the ML archive is such evidence is there, it
>> > should have been in the commit message too)
>> 
>> I just did, and found next to nothing.
>> 
>> This is the final cover letter:
>> 
>>     qemu-sockets: add abstract UNIX domain socket support
>> 
>>     qemu does not support abstract UNIX domain
>>     socket address. Add this ability to make qemu handy
>>     when abstract address is needed.
>> 
>> Boils down to "$feature is needed because it's handy when it's needed",
>> which is less than helpful.
>
> Well if you have an existing application that uses abstract sockets,
> and you want to connect QEMU to it, then QEMU needs to support it,
> or you are forced to interject a proxy between your app and QEMU
> which is an extra point of failure and lantency. I was interested
> in whether there was a specific application they were using, but
> that is largely just from a curiosity POV. There's enough usage of
> abstract sockets in apps in general that is it clearly a desirable
> feature to enable.
>
> Even if no external app is involved and you're just connecting
> together 2 QEMU processes' chardevs, abstract sockets are interesting
> because of their differing behaviour to non-abstract sockets.
>
> Most notably non-abstract sockets are tied to the filesystem mount
> namespace in Linux, where as abstract sockets are tied to the network
> namespace. This is a useful distinction in the container world. Ordinarily
> you can't connect QEMUs in 2 separate containers together, because they
> always have distinct mount namespaces. There is often the ability to
> share network namespaces between containers though, and thus unlock
> use of abstract sockets for communications. 

If this was patch review, I'd now ask the patch submitter to work it
into the commit message.

Thanks anyway :)

[...]

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

[Paolo Bonzini]

On 02/11/20 09:44, Markus Armbruster wrote:
>>> Reverting the feature requires rough consensus and a patch.
>>>
>>> I can provide a patch, but let's give everybody a chance to object
>>> first.
> Daniel, do you object, yes or no?

I think we should keep the patch, especially since you have cleaned up
everything already.  The interaction with namespaces is interesting.

Abstract sockets also do not have the usual issue with needing to unlink
the socket before bind(), because they are cleaned up automatically when
their last file descriptor is closed, including on SIGKILL.

Paolo

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

[Daniel P. Berrangé]

On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >> 
> >> > Hi Markus,
> >> >
> >> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> 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.
> >> >>
> >> >
> >> > I also failed (as chardev maintainer..) to not only review but weigh in and
> >> > discuss the merits or motivations behind it.
> >> >
> >> > I agree with you. Also the commit lacks motivation behind this "feature".
> >> >
> >> >
> >> >> 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.
> >> >>
> >> >> Regardless, I *will* make time for ripping the feature out if we
> >> >> decide to do that.  Quick & easy way to avoid reviewing this series
> >> >> *hint* *hint*.
> >> >>
> >> >
> >> > well, fwiw, I would also take that approach too, to the risk of upsetting
> >> > the users.
> >> 
> >> Reverting the feature requires rough consensus and a patch.
> >> 
> >> I can provide a patch, but let's give everybody a chance to object
> >> first.
> 
> Daniel, do you object, yes or no?

Yes, I object to removing the feature as it is clearly useful.

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 00/11] sockets: Attempt to drain the abstract socket swamp

[Markus Armbruster]

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

> On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
>> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> >> 
>> >> > Hi Markus,
>> >> >
>> >> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> >> >> Regardless, I *will* make time for ripping the feature out if we
>> >> >> decide to do that.  Quick & easy way to avoid reviewing this series
>> >> >> *hint* *hint*.
>> >> >>
>> >> >
>> >> > well, fwiw, I would also take that approach too, to the risk of upsetting
>> >> > the users.
>> >> 
>> >> Reverting the feature requires rough consensus and a patch.
>> >> 
>> >> I can provide a patch, but let's give everybody a chance to object
>> >> first.
>> 
>> Daniel, do you object, yes or no?
>
> Yes, I object to removing the feature as it is clearly useful.

Thanks.  I sent v2 of my fixes.  Can you take care of getting them
merged, hopefully in time for 5.2?

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

[Daniel P. Berrangé]

On Mon, Nov 02, 2020 at 10:59:43AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
> >> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >> >> 
> >> >> > Hi Markus,
> >> >> >
> >> >> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
> [...]
> >> >> >> Regardless, I *will* make time for ripping the feature out if we
> >> >> >> decide to do that.  Quick & easy way to avoid reviewing this series
> >> >> >> *hint* *hint*.
> >> >> >>
> >> >> >
> >> >> > well, fwiw, I would also take that approach too, to the risk of upsetting
> >> >> > the users.
> >> >> 
> >> >> Reverting the feature requires rough consensus and a patch.
> >> >> 
> >> >> I can provide a patch, but let's give everybody a chance to object
> >> >> first.
> >> 
> >> Daniel, do you object, yes or no?
> >
> > Yes, I object to removing the feature as it is clearly useful.
> 
> Thanks.  I sent v2 of my fixes.  Can you take care of getting them
> merged, hopefully in time for 5.2?

Sure, I'll aim to review & send PR today / tomorrow.

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 00/11] sockets: Attempt to drain the abstract socket swamp

[Markus Armbruster]

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

> On Mon, Nov 02, 2020 at 10:59:43AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
>> >> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> >> >> 
>> >> >> > Hi Markus,
>> >> >> >
>> >> >> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>> [...]
>> >> >> >> Regardless, I *will* make time for ripping the feature out if we
>> >> >> >> decide to do that.  Quick & easy way to avoid reviewing this series
>> >> >> >> *hint* *hint*.
>> >> >> >>
>> >> >> >
>> >> >> > well, fwiw, I would also take that approach too, to the risk of upsetting
>> >> >> > the users.
>> >> >> 
>> >> >> Reverting the feature requires rough consensus and a patch.
>> >> >> 
>> >> >> I can provide a patch, but let's give everybody a chance to object
>> >> >> first.
>> >> 
>> >> Daniel, do you object, yes or no?
>> >
>> > Yes, I object to removing the feature as it is clearly useful.
>> 
>> Thanks.  I sent v2 of my fixes.  Can you take care of getting them
>> merged, hopefully in time for 5.2?
>
> Sure, I'll aim to review & send PR today / tomorrow.

Excellent.  I'll be around to address whatever review comments you may
have.