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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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

Actually, these are just local, unpublished tests.  They both make
check_member_clash() recurse infinitely.

    # Direct inheritance loop
    # FIXME triggers infinite recursion
    { 'struct': 'Loopy', 'base': 'Loopy',
      'data': {} }

    # we reject a loop in base classes
    { 'struct': 'Base1', 'base': 'Base2', 'data': {} }
    { 'struct': 'Base2', 'base': 'Base1', 'data': {} }

The latter is actually yours, proposed as base-cycle.json in
Subject: qapi: Detect collisions in C member names
Message-Id: <address@hidden>

If I disable the recursive call, the cycle detection in
QAPISchemaObjectType.check() is reached, and works.

Completing the move of clash detection to check() methods should improve
things from "accidental infinite recursion" to "intentional assertion
failure", because it should get rid of check_member_clash() and should
not break the cycle detection.

Then we can turn the assertion into a proper error message, and add the
tests.

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