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

[Eric Blake]

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

[...]

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

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

signature.asc
Description: OpenPGP digital signature