[Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
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. We already ensure that all branches of a flat union are qapi structs with no variants, at which point those members appear in the same JSON object as all non-variant members. And we already have a map 'seen' of all non-variant members. All we need is a new QAPISchemaObjectTypeVariants.check_clash(), which clones the seen map then checks for clashes with each member of the variant's qapi type. Note that the clone of seen inside Variants.check_clash() resembles the one we just removed from Variants.check(); the difference here is that we are now checking for clashes among the qapi members of the variant type, rather than for a single clash with the variant tag name itself. 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). For simple unions, the same code happens to work by design, because of our synthesized wrapper classes (however, the wrapper has a single member 'data' which will never collide with the one non-variant member 'type', so it doesn't really matter). There is no impact to alternates, which intentionally do not need to call variants.check_clash() (there, at most one of the variant's branches will be an ObjectType, and even if one exists, we are not inlining the qapi members of that object into a parent object, the way we do for unions). No change to generated code. Signed-off-by: Eric Blake --- v10: create new Variants.check_clash() rather than piggybacking on .check() v9: new patch, split off from v8 7/17 --- scripts/qapi.py | 12 1 file changed, 12 insertions(+) diff --git a/scripts/qapi.py b/scripts/qapi.py index e057408..4c56935 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -992,6 +992,7 @@ class QAPISchemaObjectType(QAPISchemaType): if self.variants: self.variants.check(schema, seen) assert self.variants.tag_member in self.members +self.variants.check_clash(schema, seen) def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1057,6 +1058,17 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: v.check(schema, self.tag_member.type) +def check_clash(self, schema, seen): +for v in self.variants: +# Reset seen map for each variant, since qapi names from one +# branch do not affect another branch +vseen = dict(seen) +assert isinstance(v.type, QAPISchemaObjectType) +assert not v.type.variants # not implemented +v.type.check(schema) +for m in v.type.members: +m.check_clash(vseen) + class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): -- 2.4.3
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
Eric Blake 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. > > We already ensure that all branches of a flat union are qapi > structs with no variants, at which point those members appear > in the same JSON object as all non-variant members. And we > already have a map 'seen' of all non-variant members. All > we need is a new QAPISchemaObjectTypeVariants.check_clash(), > which clones the seen map then checks for clashes with each > member of the variant's qapi type. > > Note that the clone of seen inside Variants.check_clash() > resembles the one we just removed from Variants.check(); the > difference here is that we are now checking for clashes > among the qapi members of the variant type, rather than for > a single clash with the variant tag name itself. > > 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? > For simple unions, the same code happens to work by design, > because of our synthesized wrapper classes (however, the > wrapper has a single member 'data' which will never collide > with the one non-variant member 'type', so it doesn't really > matter). > > There is no impact to alternates, which intentionally do not > need to call variants.check_clash() (there, at most one of > the variant's branches will be an ObjectType, and even if one > exists, we are not inlining the qapi members of that object > into a parent object, the way we do for unions). > > No change to generated code. > > Signed-off-by: Eric Blake Patch looks good.
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
Markus Armbruster writes: > Eric Blake 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. >> >> We already ensure that all branches of a flat union are qapi >> structs with no variants, at which point those members appear >> in the same JSON object as all non-variant members. And we >> already have a map 'seen' of all non-variant members. All >> we need is a new QAPISchemaObjectTypeVariants.check_clash(), >> which clones the seen map then checks for clashes with each >> member of the variant's qapi type. >> >> Note that the clone of seen inside Variants.check_clash() >> resembles the one we just removed from Variants.check(); the >> difference here is that we are now checking for clashes >> among the qapi members of the variant type, rather than for >> a single clash with the variant tag name itself. >> >> 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? > >> For simple unions, the same code happens to work by design, >> because of our synthesized wrapper classes (however, the >> wrapper has a single member 'data' which will never collide >> with the one non-variant member 'type', so it doesn't really >> matter). >> >> There is no impact to alternates, which intentionally do not >> need to call variants.check_clash() (there, at most one of >> the variant's branches will be an ObjectType, and even if one >> exists, we are not inlining the qapi members of that object >> into a parent object, the way we do for unions). Yes. QAPISchemaObjectTypeVariants.check_clash() checks for each variant's members clashing with other members in the same name space. For alternates, there are no such other members. That said, should we add a comment to QAPISchemaAlternateType.check()? Perhaps: # Not calling self.variant.check_clash(), because there's # nothing to clash with. >> No change to generated code. >> >> Signed-off-by: Eric Blake > > Patch looks good.
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
On 11/09/2015 05:56 AM, Markus Armbruster wrote:
> Eric Blake 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
>
> 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
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
On 11/09/2015 08:13 AM, Markus Armbruster wrote: > Markus Armbruster writes: > >> Eric Blake 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. >>> >>> There is no impact to alternates, which intentionally do not >>> need to call variants.check_clash() (there, at most one of >>> the variant's branches will be an ObjectType, and even if one >>> exists, we are not inlining the qapi members of that object >>> into a parent object, the way we do for unions). > > Yes. QAPISchemaObjectTypeVariants.check_clash() checks for each > variant's members clashing with other members in the same name space. > For alternates, there are no such other members. > > That said, should we add a comment to QAPISchemaAlternateType.check()? > Perhaps: > > # Not calling self.variant.check_clash(), because there's > # nothing to clash with. Sure; if there's another reason for a respin, I can add it here; otherwise, it's already slated to be added in my respin of the patches for redoing how alternates are laid out. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
Eric Blake writes:
> On 11/09/2015 05:56 AM, Markus Armbruster wrote:
>> Eric Blake 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: <1442872682-6523-17-git-send-email-ebl...@redhat.com>
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
>>
>> Patch looks good.
>
> Yay; it's nice to see results after all our mental gymnastics over how
> collision testing should work.
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
On 11/10/2015 01:30 AM, Markus Armbruster wrote:
> Eric Blake writes:
>
>> On 11/09/2015 05:56 AM, Markus Armbruster wrote:
>>> Eric Blake 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: <1442872682-6523-17-git-send-email-ebl...@redhat.com>
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
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
On 11/09/2015 10:16 PM, Eric Blake wrote:
>> 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' } }
Another test I just tried is creating a flat union with:
empty -> base -> union
then use empty as one of the union branches. In that case, there is no
conflict (although base is included twice, neither inclusion adds
members to the JSON object; and the inclusion is not circular so things
compile just fine). So probably not worth adding a test for it.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches
Eric Blake writes:
> On 11/09/2015 10:16 PM, Eric Blake wrote:
>
>>> 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' } }
>
> Another test I just tried is creating a flat union with:
>
> empty -> base -> union
>
> then use empty as one of the union branches. In that case, there is no
> conflict (although base is included twice, neither inclusion adds
> members to the JSON object; and the inclusion is not circular so things
> compile just fine). So probably not worth adding a test for it.
Member name clashes occur because:
1. Two separately defined members happen to have a clashing name.
Whether the members are local or inherited, variant or non-variant
doesn't matter.
2. The same type gets spliced in twice, and conflict.
By "spliced", I mean the members get included. A bit like unboxed,
but also "unwrapped".
Base types and variant types get spliced in. Variant types don't
conflict with each other. Empty type can't conflict (outlawing it
anyway would be pointless). Therefore, the only conflicting splice
is a non-empty type occuring both as (base type of the) base type and
as (base type of a) variant type.
We detect this kind of clash exactly like 1. That's fine, because
the resulting error messages are plainly good enough.
3. Type contains itself.
The type gets spliced into itself, or has itself as unboxed member.
We don't unbox object members so far. That leaves the spliced ones:
base types and variants. Thus, a type contains itself if it's its
own base or one of its own variants, or a direct or indirect base of
either.
We detect this kind of clash separately, when we guard against
infinite check() recursion. Reporting it right there will be
easiest.
We want to cover all this with tests. I guess we're lacking only 3.
