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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/26/2015 11:07 AM, Markus Armbruster wrote:
>> 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.
>> 
>
> It _was_ a part of the previous patch in v9 :)

Actually, it was in "[PATCH v9 08/17] tests: Convert to new qapi union
layout" (i.e. the next patch), not in "[PATCH v9 07/17] qapi: Start
converting to new qapi union layout" (the previous patch).


> [...]
>
>>> +++ 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.
>
> Okay, that works for me.  Reserve additional namespace in 12, then drop
> the reservation in the later patch (now in subset C, see my comments to
> 24/25)...
>
>> 
>>> 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.
>
> ...and see if I can avoid churn in union-clash-type.json up until the
> point in subset C where it disappears.  I'll see how it plays out.
> Expect v11 soon.

Thanks!