Re: [Qemu-devel] [PATCH v9 08/17] tests: 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 testsuite code.
>
> Note that this includes updating an error message regarding a
> collision.  After the conversion to the new union qapi layout
> is complete, there will still be further patches for cleaning
> up collision detection, since the use of a named union can
> completely eliminate the collision wording changed here.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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.py                         |  2 +-
>  tests/qapi-schema/union-clash-type.err  |  2 +-
>  tests/qapi-schema/union-clash-type.json |  6 +--
>  tests/test-qmp-commands.c               |  4 +-
>  tests/test-qmp-input-visitor.c          | 78 
> ++++++++++++++++-----------------
>  tests/test-qmp-output-visitor.c         | 42 +++++++++---------
>  6 files changed, 66 insertions(+), 68 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1e7e08b..aab2b46 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -546,7 +546,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.
>

Umm, does this really belong to this patch?

> diff --git a/tests/qapi-schema/union-clash-type.err 
> b/tests/qapi-schema/union-clash-type.err
> index a5dead1..c14bbdd 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:6: Union 'TestUnion' member 'type' 
> clashes with '(automatic tag)'
> diff --git a/tests/qapi-schema/union-clash-type.json 
> b/tests/qapi-schema/union-clash-type.json
> index cfc256b..641b2d5 100644
> --- a/tests/qapi-schema/union-clash-type.json
> +++ b/tests/qapi-schema/union-clash-type.json
> @@ -1,9 +1,7 @@
>  # Union branch 'type'
>  # Reject this, because we would have a clash in generated C, between the
> -# simple union's implicit tag member 'kind' and the branch name 'kind'
> +# simple union's implicit tag member 'type' and the branch name 'type'
>  # within the union.
> -# TODO: Even when the generated C is switched to use 'type' rather than
> -# 'kind', to match the QMP spelling, the collision should still be detected.
> -# Or, we could munge the branch name to allow compilation.
> +# TODO: If desired, we could munge the branch name to allow compilation.

Let's mark it TODO only if we intend to revisit the idea of munging
branch names.

>  { 'union': 'TestUnion',
>    'data': { 'kind': 'int', 'type': 'str' } }
[Mind-numbing mechanical switch to u. and from kind to type...]