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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

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

I trust you mean 'tight'.

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

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

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

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

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

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