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!