Re: [Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi union layout

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> We have two issues with our qapi union layout:
> 1) Even though the QMP wire format spells the tag 'type', the
> C code spells it 'kind', requiring some hacks in the generator.
> 2) The C struct uses an anonymous union, which places all tag
> values in the same namespace as all non-variant members. This
> leads to spurious collisions if a tag value matches a QMP name.

"a non-variant member name"

Could use an example.  Pointer to flat-union-clash-branch.json would
do.

> This patch is the back end for a series that converts to a
> saner qapi union layout.  Now that all clients have been
> converted to use 'type' and 'obj->u.value', we can drop the
> temporary parallel support for 'kind' and 'obj->value'.
>
> Given a simple union qapi type:
>
> { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
>
> this is the overall effect, when compared to the state before
> this series of patches:
>
> | struct Foo {
> |-    FooKind kind;
> |-    union { /* union tag is @kind */
> |+    FooKind type;
> |+    union { /* union tag is @type */
> |         void *data;
> |         int64_t a;
> |         bool b;
> |-    };
> |+    } u;
> | };
>
> Note, however, that we do not rename the generated enum, which
> is still 'FooKind'.  A further patch could generate implicit
> enums as 'FooType', but while the generator already reserved
> the '*Kind' namespace (commit 4dc2e69), there are already QMP
> constructs with '*Type' naming, which means changing our
> reservation namespace would have lots of churn to C code to
> deal with a forced name change.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10: rebase to earlier changes, match commit wording
> v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
> http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
> ---
>  scripts/qapi-types.py | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 579532d..d131430 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -148,23 +148,10 @@ struct %(c_name)s {
>      if base:
>          ret += gen_struct_fields([], base)
>      else:
> -        # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
> -        # want to use only 'type', but the conversion is large enough to
> -        # require staging over several commits.
> -        ret += mcgen('''
> -    union {
> -        %(c_type)s kind;
> -        %(c_type)s type;
> -    };
> -''',
> -                     c_type=c_name(variants.tag_member.type.name))
> +        ret += gen_struct_field(variants.tag_member.name,
> +                                variants.tag_member.type,
> +                                False)
>
> -    # TODO As a hack, we emit the union twice, once as an anonymous union
> -    # and once as a named union.  Ultimately, we want to use only the
> -    # named union version (as it avoids conflicts between tag values as
> -    # branch names competing with non-variant QMP names), but the conversion
> -    # is large enough to require staging over several commits.
> -    tmp = ''
>      # FIXME: What purpose does data serve, besides preventing a union that
>      # has a branch named 'data'? We use it in qapi-visit.py to decide
>      # whether to bypass the switch statement if visiting the discriminator
> @@ -173,7 +160,7 @@ struct %(c_name)s {
>      # should not be any data leaks even without a data pointer.  Or, if
>      # 'data' is merely added to guarantee we don't have an empty union,
>      # shouldn't we enforce that at .json parse time?
> -    tmp += mcgen('''
> +    ret += mcgen('''
>      union { /* union tag is @%(c_name)s */
>          void *data;
>  ''',
> @@ -182,17 +169,14 @@ struct %(c_name)s {
>      for var in variants.variants:
>          # Ugly special case for simple union TODO get rid of it
>          typ = var.simple_union_type() or var.type
> -        tmp += mcgen('''
> +        ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
>                       c_type=typ.c_type(),
>                       c_name=c_name(var.name))
>
> -    ret += tmp
> -    ret += '    ' + '\n    '.join(tmp.split('\n'))
>      ret += mcgen('''
>      } u;
> -    };
>  };
>  ''')

Tag values can no longer clash with non-variant member names, but we
still have a bunch of restrictions stemming from that clash, notably the
one you updated in PATCH 12 (outlaw 'TYPE' in addition to 'KIND').  You
previously cleaned them up in "[PATCH v10 24/25] qapi: Remove outdated
tests related to QMP/branch collisions", but that one's now delayed
until later.  I haven't tried to understand why, because I think
delaying it a bit is okay, but I'd like to have it noted in the commit
message.  If you supply a suitably updated one, I'll touch it up in my
tree.