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

[Eric Blake]

On 11/10/2015 01:30 AM, Markus Armbruster wrote:
> 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': {} }

Okay, I should add that into my pending patch that cleans up base loops,

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

and yes, that one is still in my queue for subset D:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg07001.html

and I may indeed at a test for reusing the base type of a flat union as
one of the branches of the same union, depending on whether it uncovers
anything different.


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

Yep, that's what is pending in my queue, just further out than subset C.
 Doesn't matter if it misses 2.5 (the bug is real, but is only triggered
by bad .json code, and we aren't going to add any bad .json code between
now and 2.5).


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

Still true - I'm still plowing through earlier patches before deciding
if my 'qapi: Detect base class loops' also needs to detect flat union loops.

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

signature.asc
Description: OpenPGP digital signature