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

[Eric Blake]

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).

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

> 
> 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

> +++ 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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org