Re: [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of flat union branches

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

On 11/04/2015 12:01 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Right now, our ad hoc parser ensures that we cannot have a
>> flat union that introduces any qapi member names that would
>> conflict with the non-variant qapi members already present
>> from the union's base type (see flat-union-clash-member.json).
>> We want QAPISchema*.check() to make the same check, so we can
>> later reduce some of the ad hoc checks.
>>

>> @@ -1068,6 +1069,14 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>      def check(self, schema, tag_type, seen):
>>          QAPISchemaObjectTypeMember.check(self, schema)
>>          assert self.name in tag_type.values
>> +        if seen:
>> +            # This variant is used within a union; ensure each qapi member
>> +            # field does not collide with the union's non-variant members.
>> +            assert isinstance(self.type, QAPISchemaObjectType)
>> +            assert not self.type.variants       # not implemented
>> +            self.type.check(schema)
>> +            for m in self.type.members:
>> +                m.check_clash(seen)
>>
>>      # This function exists to support ugly simple union special cases
>>      # TODO get rid of them, and drop the function
> 
> Two call chains:
> 
> * QAPISchemaObjectType.check() via QAPISchemaObjectTypeVariants.check()
> 
>   In this case, the new conditional executes.
> 
> * QAPISchemaAlternateType.check() via same
> 
>   In this case, it doesn't.
> 
> Why can't we simply add the new code to QAPISchemaObjectType.check()?

We could, but we'd need a nested for-loop:

for v in variants.variants:
    v.type.check(schema)
    assert not v.type.variants
    vseen = dict(seen)
    for m in v.type.members:
        m.check_clash(seen)

> The rest of the clash checking is already there...

You do have a point there.  I guess it wouldn't be the end of the world
to have the loop nesting be explicit rather than indirect through the
intermediate Variants.check().

I'll play with it; and depending on what I do, that may mean I don't
have to munge your other patches quite so heavily.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]