diff mbox series

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

Message ID 20201102094422.173975-12-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series sockets: Attempt to drain the abstract socket swamp | expand

Commit Message

Markus Armbruster Nov. 2, 2020, 9:44 a.m. UTC
The abstract socket namespace is a non-portable Linux extension.  An
attempt to use it elsewhere should fail with ENOENT (the abstract
address looks like a "" pathname, which does not resolve).  We report
this failure like

    Failed to connect socket abc: No such file or directory

Tolerable, although ENOTSUP would be better.

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

The above failure becomes

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

I consider this an improvement.

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

Comments

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

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

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

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

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

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

...vs. the now-nicer conditional where tight is only present if abstract.
Markus Armbruster Nov. 3, 2020, 6:35 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

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

I trust you mean 'tight'.

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

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

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

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

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

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

Patch

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