Re: [Qemu-devel] [PATCH v10 13/25] qapi-visit: Convert to new qapi union layout

[Markus Armbruster]

Eric Blake <address@hidden> 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.
>
> Make the conversion to the new layout for qapi-visit.py.

This part is nicely mechanical: ->kind becomes ->type, ->variant becomes
->u.variant, and variants.tag_name or 'kind' becomes
variants.tag_member.name.

>                                                           Also
> make a change in qapi.py to quit using the name 'kind', and
> the corresponding change in the testsuite.

This isn't really part of "qapi-visit: Convert to new qapi union
layout".  Could be made a separate patch, but I'd simply squash it into
the previous one "qapi: Start converting to new qapi union layout".  See
also my remark inline.

>                                             However, many more
> changes will be made to the testsuite later, including the
> entire deletion of union-clash-type.json, after the completion
> of the conversion eliminates collisions between union tag
> branch names and non-variant names.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch, split from 7/17 an 8/17
> 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-visit.py                  | 24 +++++++++---------------
>  scripts/qapi.py                        |  2 +-
>  tests/qapi-schema/union-clash-type.err |  2 +-
>  3 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3c23c53..421fcb6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -187,18 +187,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      if (err) {
>          goto out;
>      }
> -    visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, 
> &err);
> +    visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, 
> &err);
>      if (err) {
>          goto out_obj;
>      }
> -    switch ((*obj)->kind) {
> +    switch ((*obj)->type) {
>  ''',
>                  c_name=c_name(name))
>
>      for var in variants.variants:
>          ret += mcgen('''
>      case %(case)s:
> -        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
> +        visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
>          break;
>  ''',
>                       case=c_enum_const(variants.tag_member.type.name,
> @@ -259,22 +259,16 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
>  ''',
>                       c_type=variants.tag_member.type.c_name(),
> -                     # TODO ugly special case for simple union
> -                     # Use same tag name in C as on the wire to get rid of
> -                     # it, then: c_name=c_name(variants.tag_member.name)
> -                     c_name='kind',
> +                     c_name=c_name(variants.tag_member.name),
>                       name=variants.tag_member.name)
>      ret += gen_err_check(label='out_obj')
>      ret += mcgen('''
> -    if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> +    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
>          goto out_obj;
>      }
>      switch ((*obj)->%(c_name)s) {
>  ''',
> -                 # TODO ugly special case for simple union
> -                 # Use same tag name in C as on the wire to get rid of
> -                 # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'))
> +                 c_name=c_name(variants.tag_member.name))
>
>      for var in variants.variants:
>          # TODO ugly special case for simple union
> @@ -286,13 +280,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>                                         var.name))
>          if simple_union_type:
>              ret += mcgen('''
> -        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
> +        visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err);
>  ''',
>                           c_type=simple_union_type.c_name(),
>                           c_name=c_name(var.name))
>          else:
>              ret += mcgen('''
> -        visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
> +        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
>  ''',
>                           c_type=var.type.c_name(),
>                           c_name=c_name(var.name))
> @@ -308,7 +302,7 @@ out_obj:
>      error_propagate(errp, err);
>      err = NULL;
>      if (*obj) {
> -        visit_end_union(v, !!(*obj)->data, &err);
> +        visit_end_union(v, !!(*obj)->u.data, &err);
>      }
>      error_propagate(errp, err);
>      err = NULL;
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 04bcbf7..8b9f5f7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -548,7 +548,7 @@ def check_union(expr, expr_info):
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
>      members = expr['data']
> -    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
> +    values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
>
>      # Two types of unions, determined by discriminator.
>

Took me a minute to remember what's going on here.  A simple union's tag
values, become an enum type.  Here, we catch two differend kinds of
clashes:

1. Tag value with the implicitly defined enum member _MAX, i.e. a clash
in the enum member name space

2. Member of the (as of yet still) unnamed union holding the variants
with the tag variable type, formerly kind (i.e. a clash in the struct
member name space).

The previous patch added type to the struct member name space without
updating clash detection.

A future patch will remove the unnamed union, thus clash kind 2.  It'll
also remove kind from the struct member name space, but that doesn't
matter.

I believe the following would be slightly cleaner: amend the previous
patch to *add* key 'TYPE' to values.  Drop both 'KIND' and 'TYPE' when
you remove the unnamed union.

> diff --git a/tests/qapi-schema/union-clash-type.err 
> b/tests/qapi-schema/union-clash-type.err
> index a5dead1..eca7c1d 100644
> --- a/tests/qapi-schema/union-clash-type.err
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' 
> clashes with '(automatic)'
> +tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'type' 
> clashes with '(automatic tag)'

You might be able to avoid this change by adding 'TYPE' in the right
spot.