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