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