On 10/22/2015 08:01 AM, Markus Armbruster wrote:
> 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.
>>
>> 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 <eblake@redhat.com>
>>
>> ---
>> 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?

Yes, because it cleans up the error message in union-clash-type.err, as
mentioned in the commit message. But since I'm already splitting out the
qapi-visit parts of 7/17, maybe it belongs better in that patch (all
changes to the rest of qapi to deal with the qapi-type parts)?

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

At any rate, this is what improved by adjusting that line of qapi.py.

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

I have a later patch queued that deletes this test altogether, so for
v10 I'll probably just eliminate changes here for less churn.
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04111.html

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

Yep.  I wish I knew coccinelle well enough to see if it could do the
conversion for me, but I ended up doing it by hand (basically by
applying 16/17 early, then seeing what failed to compile, fixing it up,
then rebasing it into position).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org