Eric Blake <ebl...@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 <ebl...@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.
