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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
