Re: [PATCH 07/11] sockets: Fix default of UnixSocketAddress member @tight

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 AM, Markus Armbruster wrote:
>> QMP chardev-add defaults absent member @tight to false instead of
>> true.  HMP chardev-add and CLI -chardev correctly default to true.
>> 
>> The previous commit demonstrated that socket_listen() and
>> socket_connect() are broken for absent @tight.  That explains why QMP
>> is broken, but not why HMP and CLI work.  We need to dig deeper.
>> 
>> An optional bool member of a QAPI struct can be false, true, or
>> absent.  In C, we have:
>> 
>>          has_MEMBER    MEMBER
>>     false         true          false
>>     true       true     false
>>     absent    false  false/ignore
>
> I'm not sure the TAB in this table made it very legible (it's hard to
> tell if has_MEMBER is the label of column 1 or 2).

Use of TAB is an accident.

> Row two is wrong: MEMBER (column 3) is set to true when the QMP code
> passed true on the wire.

Pasto, fixing...

Result:

            has_MEMBER    MEMBER
    false         true     false
    true          true      true
    absent       false  false/ignore

>> When has_MEMBER is false, MEMBER should be set to false on write, and
>> ignored on read.
>> 
>> unix_listen_saddr() and unix_connect_saddr() use member @tight without
>> checking @has_tight.  This is wrong.
>
> It generally works if addr was constructed by the same way as the
> generated QAPI parser code - but as you demonstrated, in this particular
> case, because our construction did not obey the rules of the QAPI
> parser, our lack of checking bit us.
>
>> When @tight was set to false as it should be, absent @tight defaults
>> to false.  Wrong, it should default to true.  This is what breaks QMP.
>> 
>> There is one exception: qemu_chr_parse_socket() leaves @has_tight
>> false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
>> why HMP and CLI work.  Same for @has_abstract.
>> 
>> Fix unix_listen_saddr() and unix_connect_saddr() to default absent
>> @tight to true.
>> 
>> Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.
>
> At any rate, the fix looks correct:
> - as producers, anywhere we hand-construct an addr (rather than using
> generated QAPI code), we MUST set both has_MEMBER and MEMBER, including
> setting MEMBER to false if has_MEMBER is false, if we want to preserve
> the assumptions made in the rest of the code;
> - as consumers, rather than relying on the QAPI parsers only setting
> MEMBER to true when has_MEMBER is true, we can ensure that has_MEMBER
> has priority by checking it ourselves

As long as the instance is built according to the rules, you can
contract has_MEMBER && MEMBER to just MEMBER.  Both mean "true".

However, has_MEMBER && !MEMBER cannot be contracted to !MEMBER.  The
former means "false", the latter means "false or absent".

Doubters, see the table above.

Putting defaults in the schema would let us eliminate the "absent"
state, the has_MEMBER flags, and the bugs that come with them.  Sadly,
this project has been crowded out by more urgent or important work since
forever.

>> +++ b/util/qemu-sockets.c
>> @@ -919,7 +919,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>>      if (saddr->abstract) {
>>          un.sun_path[0] = '\0';
>>          memcpy(&un.sun_path[1], path, pathlen);
>> -        if (saddr->tight) {
>> +        if (!saddr->has_tight || saddr->tight) {
>>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>>          }
>>      } else {
>> @@ -979,7 +979,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, 
>> Error **errp)
>>      if (saddr->abstract) {
>>          un.sun_path[0] = '\0';
>>          memcpy(&un.sun_path[1], saddr->path, pathlen);
>> -        if (saddr->tight) {
>> +        if (!saddr->has_tight || saddr->tight) {
>>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>>          }
>>      } else {
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!