Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches

[Eric Blake]

On 11/09/2015 05:56 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 QAPISchemaObjectType.check() to make the same check,
>> so we can later reduce some of the ad hoc checks.
>>

>> In general, a type used as a branch of a flat union cannot
>> also be the base type of the flat union, so even though we are
>> adding a call to variant.type.check() in order to populate
>> variant.type.members, this is merely a case of gaining
>> topological sorting of how types are visited (and type.check()
>> is already set up to allow multiple calls due to base types).
> 
> Yes, a type cannot contain itself, neither as base nor as variant.
> 
> We have tests covering attempts to do the former
> (struct-cycle-direct.json, struct-cycle-indirect.json).  As far as I can
> see, we don't have tests covering the latter.  Do we catch it?

Yes, at least by virtue of the ad hoc tests: attempting to reuse a base
type of the flat union as a variant member will cause the qapi members
of the base type to appear more than once in the JSON object (that is,
the checks that reject flat-union-clash-member.json would also reject
this scenario). To test:

diff --git i/tests/qapi-schema/qapi-schema-test.json
w/tests/qapi-schema/qapi-schema-test.json
index 44638da..16b2ffb 100644
--- i/tests/qapi-schema/qapi-schema-test.json
+++ w/tests/qapi-schema/qapi-schema-test.json
@@ -67,7 +67,7 @@
   'discriminator': 'enum1',
   'data': { 'value1' : 'UserDefA',
             'value2' : 'UserDefB',
-            'value3' : 'UserDefB' } }
+            'value3' : 'UserDefUnionBase' } }

 { 'struct': 'UserDefUnionBase',
   'base': 'UserDefZero',

  GEN   tests/test-qapi-types.h
/home/eblake/qemu/tests/qapi-schema/qapi-schema-test.json:65: Member
name 'string' of branch 'value3' clashes with base 'UserDefUnionBase'
/home/eblake/qemu/tests/Makefile:415: recipe for target
'tests/test-qapi-types.h' failed

But you have me curious if this collision is still caught when the ad
hoc tests are gone.  If so, great; if not, I'll add a test here.  (I'll
know later when I get through rebasing to all of your comments.)

>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> Patch looks good.

Yay; it's nice to see results after all our mental gymnastics over how
collision testing should work.

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

signature.asc
Description: OpenPGP digital signature