| Message ID | 1442872682-6523-4-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Expose some weaknesses in the generator: we don't always forbid > the generation of structs that contain multiple members that map Slightly misleading. args-name-clash is a clash between command arguments. These are a struct internally, but we don't currently generate an actual struct for it, only an argument list. > to the same C name. This has already been marked FIXME in > qapi.py, but having more tests will make sure future patches > produce desired behavior. Point to commit d90675f? > Some of these tests will be deleted later, and a positive test > added to qapi-schema-test.json in its place, when the code is "in their place"? > reworked so that the collision no longer occurs. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/Makefile | 6 ++++++ > tests/qapi-schema/args-name-clash.err | 0 > tests/qapi-schema/args-name-clash.exit | 1 + > tests/qapi-schema/args-name-clash.json | 2 ++ > tests/qapi-schema/args-name-clash.out | 6 ++++++ > tests/qapi-schema/flat-union-branch-clash.json | 2 +- > tests/qapi-schema/flat-union-branch-clash2.err | 0 > tests/qapi-schema/flat-union-branch-clash2.exit | 1 + > tests/qapi-schema/flat-union-branch-clash2.json | 14 ++++++++++++++ > tests/qapi-schema/flat-union-branch-clash2.out | 14 ++++++++++++++ > tests/qapi-schema/flat-union-cycle.err | 1 + > tests/qapi-schema/flat-union-cycle.exit | 1 + > tests/qapi-schema/flat-union-cycle.json | 6 ++++++ > tests/qapi-schema/flat-union-cycle.out | 0 > tests/qapi-schema/qapi-schema-test.json | 5 +++-- > tests/qapi-schema/qapi-schema-test.out | 2 ++ > tests/qapi-schema/struct-base-clash2.err | 0 > tests/qapi-schema/struct-base-clash2.exit | 1 + > tests/qapi-schema/struct-base-clash2.json | 5 +++++ > tests/qapi-schema/struct-base-clash2.out | 5 +++++ > tests/qapi-schema/union-clash.err | 1 + > tests/qapi-schema/union-clash.exit | 1 + > tests/qapi-schema/union-clash.json | 3 +++ > tests/qapi-schema/union-clash.out | 0 > tests/qapi-schema/union-clash2.err | 0 > tests/qapi-schema/union-clash2.exit | 1 + > tests/qapi-schema/union-clash2.json | 3 +++ > tests/qapi-schema/union-clash2.out | 6 ++++++ > 28 files changed, 84 insertions(+), 3 deletions(-) > create mode 100644 tests/qapi-schema/args-name-clash.err > create mode 100644 tests/qapi-schema/args-name-clash.exit > create mode 100644 tests/qapi-schema/args-name-clash.json > create mode 100644 tests/qapi-schema/args-name-clash.out > create mode 100644 tests/qapi-schema/flat-union-branch-clash2.err > create mode 100644 tests/qapi-schema/flat-union-branch-clash2.exit > create mode 100644 tests/qapi-schema/flat-union-branch-clash2.json > create mode 100644 tests/qapi-schema/flat-union-branch-clash2.out > create mode 100644 tests/qapi-schema/flat-union-cycle.err > create mode 100644 tests/qapi-schema/flat-union-cycle.exit > create mode 100644 tests/qapi-schema/flat-union-cycle.json > create mode 100644 tests/qapi-schema/flat-union-cycle.out > create mode 100644 tests/qapi-schema/struct-base-clash2.err > create mode 100644 tests/qapi-schema/struct-base-clash2.exit > create mode 100644 tests/qapi-schema/struct-base-clash2.json > create mode 100644 tests/qapi-schema/struct-base-clash2.out > create mode 100644 tests/qapi-schema/union-clash.err > create mode 100644 tests/qapi-schema/union-clash.exit > create mode 100644 tests/qapi-schema/union-clash.json > create mode 100644 tests/qapi-schema/union-clash.out > create mode 100644 tests/qapi-schema/union-clash2.err > create mode 100644 tests/qapi-schema/union-clash2.exit > create mode 100644 tests/qapi-schema/union-clash2.json > create mode 100644 tests/qapi-schema/union-clash2.out > > diff --git a/tests/Makefile b/tests/Makefile > index 6fd5885..97434f6 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -238,6 +238,7 @@ qapi-schema += args-invalid.json > qapi-schema += args-member-array-bad.json > qapi-schema += args-member-array.json > qapi-schema += args-member-unknown.json > +qapi-schema += args-name-clash.json > qapi-schema += args-union.json > qapi-schema += args-unknown.json > qapi-schema += bad-base.json > @@ -274,6 +275,8 @@ qapi-schema += flat-union-bad-discriminator.json > qapi-schema += flat-union-base-any.json > qapi-schema += flat-union-base-union.json > qapi-schema += flat-union-branch-clash.json > +qapi-schema += flat-union-branch-clash2.json > +qapi-schema += flat-union-cycle.json > qapi-schema += flat-union-inline.json > qapi-schema += flat-union-int-branch.json > qapi-schema += flat-union-invalid-branch-key.json > @@ -317,6 +320,7 @@ qapi-schema += returns-unknown.json > qapi-schema += returns-whitelist.json > qapi-schema += struct-base-clash-deep.json > qapi-schema += struct-base-clash.json > +qapi-schema += struct-base-clash2.json > qapi-schema += struct-data-invalid.json > qapi-schema += struct-member-invalid.json > qapi-schema += trailing-comma-list.json > @@ -328,6 +332,8 @@ qapi-schema += unclosed-string.json > qapi-schema += unicode-str.json > qapi-schema += union-bad-branch.json > qapi-schema += union-base-no-discriminator.json > +qapi-schema += union-clash.json > +qapi-schema += union-clash2.json > qapi-schema += union-invalid-base.json > qapi-schema += union-max.json > qapi-schema += union-optional-branch.json > diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json > new file mode 100644 > index 0000000..19bf792 > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.json > @@ -0,0 +1,2 @@ > +# FIXME - we should reject data with members that clash when mapped to C names > +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } > diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out > new file mode 100644 > index 0000000..9b2f6e4 > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.out > @@ -0,0 +1,6 @@ > +object :empty > +object :obj-oops-arg > + member a-b: str optional=False > + member a_b: str optional=False > +command oops :obj-oops-arg -> None > + gen=True success_response=True > diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-branch-clash.json > index 8fb054f..3d7e6c6 100644 > --- a/tests/qapi-schema/flat-union-branch-clash.json > +++ b/tests/qapi-schema/flat-union-branch-clash.json > @@ -1,4 +1,4 @@ > -# we check for no duplicate keys between branches and base > +# we check for no duplicate keys between branch members and base > { 'enum': 'TestEnum', > 'data': [ 'value1', 'value2' ] } > { 'struct': 'Base', This clashing business is awfully confusing as soon as unions come into play. When I'm confused, I need to think in writing. The basic case is clash between local, non-variant members. Needs test coverage. args-name-clash.json provides it, because internally the arguments are just another object type. With a base, the members inherited from base get added to the mix. We need to test a clash betwen local, non-variant member and a member inherited from base. With unions, things get complicated, because we have multiple kinds of clashes. Best explained with an example. Let's use UserDefFlatUnion from qapi-schema-test.json. { 'union': 'UserDefFlatUnion', 'base': 'UserDefUnionBase', # intentional forward reference 'discriminator': 'enum1', 'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } } { 'struct': 'UserDefUnionBase', 'base': 'UserDefZero', 'data': { 'string': 'str', 'enum1': 'EnumOne' } } Generated C looks like this: struct UserDefFlatUnion { /* Members inherited from UserDefUnionBase: */ int64_t integer; char *string; EnumOne enum1; /* Own members: */ // if the schema language supported adding non-variant local // members, they'd go right here union { /* union tag is @enum1 */ void *data; UserDefA *value1; UserDefB *value2; UserDefB *value3; }; }; Thus, what can clash in C is the tag values value1, value2, value3 with the non-variant members integer, string, enum1. On the wire, the union members are unboxed, i.e. we get just "boolean": false instead of "value1": { "boolean": false } Thus what can clash on the wire is the variant members with the non-variant members: boolean with integer, string, enum1 when enum1 is value1, and so forth. This is the clash flat-union-branch-clash.json tests. Its error message is "Member name 'name' of branch 'value1' clashes with base 'Base'". Suboptimal, it should say "with member 'name' of base 'Base'". > diff --git a/tests/qapi-schema/flat-union-branch-clash2.err b/tests/qapi-schema/flat-union-branch-clash2.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-branch-clash2.exit b/tests/qapi-schema/flat-union-branch-clash2.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/flat-union-branch-clash2.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/flat-union-branch-clash2.json b/tests/qapi-schema/flat-union-branch-clash2.json > new file mode 100644 > index 0000000..b3eefb3 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-branch-clash2.json > @@ -0,0 +1,14 @@ > +# FIXME: we should check for no duplicate C names between branches and base > +{ 'enum': 'TestEnum', > + 'data': [ 'base', 'c-d' ] } > +{ 'struct': 'Base', > + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } > +{ 'struct': 'Branch1', > + 'data': { 'string': 'str' } } > +{ 'struct': 'Branch2', > + 'data': { 'value': 'int' } } > +{ 'union': 'TestUnion', > + 'base': 'Base', > + 'discriminator': 'enum1', > + 'data': { 'base': 'Branch1', > + 'c-d': 'Branch2' } } This tests the other kind of clash: tag value 'c-d' clashes with non-variant member name 'c_d'. Please add a comment explaining what clash should be reported here. > diff --git a/tests/qapi-schema/flat-union-branch-clash2.out b/tests/qapi-schema/flat-union-branch-clash2.out > new file mode 100644 > index 0000000..8e0da73 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-branch-clash2.out > @@ -0,0 +1,14 @@ > +object :empty > +object Base > + member enum1: TestEnum optional=False > + member c_d: str optional=True > +object Branch1 > + member string: str optional=False > +object Branch2 > + member value: int optional=False > +enum TestEnum ['base', 'c-d'] > +object TestUnion > + base Base > + tag enum1 > + case base: Branch1 > + case c-d: Branch2 > diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err > new file mode 100644 > index 0000000..152c6f0 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.err > @@ -0,0 +1 @@ > +tests/qapi-schema/flat-union-cycle.json:5: Member name 'switch' of branch 'loop' clashes with base 'Base' > diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json > new file mode 100644 > index 0000000..8f6cd93 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.json > @@ -0,0 +1,6 @@ > +# we reject a loop in flat unions, due to member collision > +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] } > +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } } > +{ 'struct': 'Okay', 'data': { 'int': 'int' } } > +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch', > + 'data': { 'okay': 'Okay', 'loop': 'Base' } } This isn't a loop, it's a fork: we get the members of Base via its use as base, and again via its use as type of a variant case. What does it add over flat-union-branch-clash.json? > diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index 6897a6e..c904db4 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -32,11 +32,12 @@ > 'dict1': 'UserDefTwoDict' } } > > # for testing unions > +# name collisions between branches should not clash > { 'struct': 'UserDefA', > - 'data': { 'boolean': 'bool' } } > + 'data': { 'boolean': 'bool', '*a_b': 'int' } } > > { 'struct': 'UserDefB', > - 'data': { 'intb': 'int' } } > + 'data': { 'intb': 'int', '*a-b': 'bool' } } > > { 'union': 'UserDefFlatUnion', > 'base': 'UserDefUnionBase', # intentional forward reference This tests that different variants may have clashing names. Okay. I'm afraid the comment is a bit too terse. Not sure I'd make the connection from it to member a_b and to UserDefB's a-b a fortnight from now. > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 1f6e858..28a0b3c 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2'] > prefix QENUM_TWO > object UserDefA > member boolean: bool optional=False > + member a_b: int optional=True > alternate UserDefAlternate > case uda: UserDefA > case s: str > @@ -78,6 +79,7 @@ alternate UserDefAlternate > enum UserDefAlternateKind ['uda', 's', 'i'] > object UserDefB > member intb: int optional=False > + member a-b: bool optional=True > object UserDefC > member string1: str optional=False > member string2: str optional=False > diff --git a/tests/qapi-schema/struct-base-clash2.err b/tests/qapi-schema/struct-base-clash2.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/struct-base-clash2.exit b/tests/qapi-schema/struct-base-clash2.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash2.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/struct-base-clash2.json b/tests/qapi-schema/struct-base-clash2.json > new file mode 100644 > index 0000000..56166e0 > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash2.json > @@ -0,0 +1,5 @@ > +# FIXME - a base class collides with a member named base > +{ 'struct': 'Base', 'data': {} } > +{ 'struct': 'Sub', > + 'base': 'Base', > + 'data': { 'base': 'str' } } What's this about? Hmm, I think it's about the way we do a struct type's base. For a union type, we add the base's members, as shown above. For a struct type, we add the base *boxed*, like this: struct Sub { // The base type Base *base; // Own members char *base; }; Therefore, a struct type with a base can't have a member named base. But that's simply daft. As soon as we change it to match union types, this test case goes away. If we change it soon, do we still need this test? Will it be done later in this series? > diff --git a/tests/qapi-schema/struct-base-clash2.out b/tests/qapi-schema/struct-base-clash2.out > new file mode 100644 > index 0000000..e69a416 > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash2.out > @@ -0,0 +1,5 @@ > +object :empty > +object Base > +object Sub > + base Base > + member base: str optional=False > diff --git a/tests/qapi-schema/union-clash.err b/tests/qapi-schema/union-clash.err > new file mode 100644 > index 0000000..64637ed > --- /dev/null > +++ b/tests/qapi-schema/union-clash.err > @@ -0,0 +1 @@ > +tests/qapi-schema/union-clash.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b' > diff --git a/tests/qapi-schema/union-clash.exit b/tests/qapi-schema/union-clash.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/union-clash.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/union-clash.json b/tests/qapi-schema/union-clash.json > new file mode 100644 > index 0000000..0393ed8 > --- /dev/null > +++ b/tests/qapi-schema/union-clash.json > @@ -0,0 +1,3 @@ > +# we reject unions where branch names clash when mapped to C > +{ 'union': 'TestUnion', > + 'data': { 'a-b': 'int', 'a_b': 'str' } } Is it possible for branch names to clash in C when the enumeration (be it implicit or explicit) passes clash checks? > diff --git a/tests/qapi-schema/union-clash.out b/tests/qapi-schema/union-clash.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/union-clash2.err b/tests/qapi-schema/union-clash2.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/union-clash2.exit b/tests/qapi-schema/union-clash2.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/union-clash2.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/union-clash2.json b/tests/qapi-schema/union-clash2.json > new file mode 100644 > index 0000000..b2d45fb > --- /dev/null > +++ b/tests/qapi-schema/union-clash2.json > @@ -0,0 +1,3 @@ > +# FIXME - a union branch named 'data' collides with generated C code > +{ 'union': 'TestUnion', > + 'data': { 'data': 'int' } } This tests another stupid clash: we put a member named data in our generated unions. As soon as we stop doing that, this test will go away. If we stop soon, do we still need this test? Will we stop later in this series? > diff --git a/tests/qapi-schema/union-clash2.out b/tests/qapi-schema/union-clash2.out > new file mode 100644 > index 0000000..6277239 > --- /dev/null > +++ b/tests/qapi-schema/union-clash2.out > @@ -0,0 +1,6 @@ > +object :empty > +object :obj-int-wrapper > + member data: int optional=False > +object TestUnion > + case data: :obj-int-wrapper > +enum TestUnionKind ['data']
On 09/22/2015 09:23 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Expose some weaknesses in the generator: we don't always forbid >> the generation of structs that contain multiple members that map > > Slightly misleading. args-name-clash is a clash between command > arguments. These are a struct internally, but we don't currently > generate an actual struct for it, only an argument list. Maybe struct-member-clash? Renames are easy enough, but only if patch 1/46 is okay to go in first. :) > >> to the same C name. This has already been marked FIXME in >> qapi.py, but having more tests will make sure future patches >> produce desired behavior. > > Point to commit d90675f? Sure, now that it finally landed. > >> Some of these tests will be deleted later, and a positive test >> added to qapi-schema-test.json in its place, when the code is > > "in their place"? > Yep. (Perils of editing, I started with one test, then added more later and merged into one patch) >> reworked so that the collision no longer occurs. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +++ b/tests/qapi-schema/flat-union-branch-clash.json >> @@ -1,4 +1,4 @@ >> -# we check for no duplicate keys between branches and base >> +# we check for no duplicate keys between branch members and base >> { 'enum': 'TestEnum', >> 'data': [ 'value1', 'value2' ] } >> { 'struct': 'Base', > > This clashing business is awfully confusing as soon as unions come into > play. When I'm confused, I need to think in writing. No kidding. We already attempted to detect clashes, and caught some, but not all, types of clashes. And there are indeed two types of member name clashes: those where the generated C struct has duplicate members (either because 2 user names map to the same C name, or because the generated code injects a C name for a purpose other than a "key":value name), and those where the qapi type would specify the same "key":value name more than once in the same {} object on the wire (even if the names would not collide in C because one is accessed through a box pointer). By patch 16/46, we should be catching all cases of member name clashes, but there's still work to do to catch collisions in 'command' and/or 'event' names. Also, by the time 16/46 is in, there are cases where we reject "clashes" where two member names with different spellings would map to the same C name, but where the corresponding C struct does not have a clash because the members are boxed behind different pointers. Technically, we would not have to reject such cases, but the case is still confusing enough that rejecting it forces the qapi writer to consider a naming convention that is less confusing in the first place. > > The basic case is clash between local, non-variant members. Needs test > coverage. args-name-clash.json provides it, because internally the > arguments are just another object type. Correct. The test proves we don't yet catch the clash, and is fixed when later commits add the check. > > With a base, the members inherited from base get added to the mix. We > need to test a clash betwen local, non-variant member and a member > inherited from base. True for both structs and flat unions (the two places where we use 'base'). More on this below. > > With unions, things get complicated, because we have multiple kinds of > clashes. Best explained with an example. Let's use UserDefFlatUnion > from qapi-schema-test.json. > > { 'union': 'UserDefFlatUnion', > 'base': 'UserDefUnionBase', # intentional forward reference > 'discriminator': 'enum1', > 'data': { 'value1' : 'UserDefA', > 'value2' : 'UserDefB', > 'value3' : 'UserDefB' } } > > { 'struct': 'UserDefUnionBase', > 'base': 'UserDefZero', > 'data': { 'string': 'str', 'enum1': 'EnumOne' } } > > Generated C looks like this: > > struct UserDefFlatUnion { > /* Members inherited from UserDefUnionBase: */ > int64_t integer; > char *string; > EnumOne enum1; > /* Own members: */ > // if the schema language supported adding non-variant local > // members, they'd go right here > union { /* union tag is @enum1 */ > void *data; > UserDefA *value1; > UserDefB *value2; > UserDefB *value3; > }; > }; > > Thus, what can clash in C is the tag values value1, value2, value3 with > the non-variant members integer, string, enum1. That is, the tag values now appear as C member names, even though they did not correspond to QMP "key":value names. Likewise, the 'data' C member name can cause a clash. Was even worse before commit 0f61af3e, where we were also burning the C name 'kind'. Commit 1e6c1616 was where we quit burning the C member name 'base'. Prior to that time, members of base classes did not clash with variant names because of the C boxing. If we run into a situation where the enum values collide with base member names (both of which are ABI), we could still solve the collision by renaming the C member names for the enum values to something that don't collide (such as _tag_value1 rather than value1); this is because the C member names are not ABI and can be changed. But we can cross that bridge later if the situation ever arises; for now, it's just easier to patch the generator to reject qapi where such a collision would occur. > > On the wire, the union members are unboxed, i.e. we get just > > "boolean": false > > instead of > > "value1": { "boolean": false } > > Thus what can clash on the wire is the variant members with the > non-variant members: boolean with integer, string, enum1 when enum1 is > value1, and so forth. > > This is the clash flat-union-branch-clash.json tests. Its error message > is "Member name 'name' of branch 'value1' clashes with base 'Base'". > Suboptimal, it should say "with member 'name' of base 'Base'". Indeed, this is the other type of clash (QMP wire clashes, whether or not they cause a C member clash). >> +++ b/tests/qapi-schema/flat-union-branch-clash2.json >> @@ -0,0 +1,14 @@ >> +# FIXME: we should check for no duplicate C names between branches and base >> +{ 'enum': 'TestEnum', >> + 'data': [ 'base', 'c-d' ] } >> +{ 'struct': 'Base', >> + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } >> +{ 'struct': 'Branch1', >> + 'data': { 'string': 'str' } } >> +{ 'struct': 'Branch2', >> + 'data': { 'value': 'int' } } >> +{ 'union': 'TestUnion', >> + 'base': 'Base', >> + 'discriminator': 'enum1', >> + 'data': { 'base': 'Branch1', >> + 'c-d': 'Branch2' } } > > This tests the other kind of clash: tag value 'c-d' clashes with > non-variant member name 'c_d'. > > Please add a comment explaining what clash should be reported here. Will do; and by the end of the series the error is properly reported. >> +++ b/tests/qapi-schema/flat-union-cycle.json >> @@ -0,0 +1,6 @@ >> +# we reject a loop in flat unions, due to member collision >> +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] } >> +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } } >> +{ 'struct': 'Okay', 'data': { 'int': 'int' } } >> +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch', >> + 'data': { 'okay': 'Okay', 'loop': 'Base' } } > > This isn't a loop, it's a fork: we get the members of Base via its use > as base, and again via its use as type of a variant case. > > What does it add over flat-union-branch-clash.json? I wrote this test when I discovered the assertion failure in the parser bug as covered by patch 16/46 (a struct attempting to inherit directly or indirectly from itself is not nice). When I first wrote the test, I was trying to make sure that a flat union cannot inherit from itself, but then ran into the problem that a base class must be a struct and not a union. So I changed the test to make sure that QMP cannot reuse the base class as a variant type, since that would require the members of the base type to occur in QMP more than once, without seeing if any other test already did that. You may have a point that this doesn't cover anything beyond flat-union-branch-clash, and since my later changes to detect self-inheritance didn't change the error message flagged for this case, we can probably safely drop this test as not adding anything. And I guess I should still test that self-inheritance attempts are rejected, even if we later relax things to allow a non-struct as a base class. >> +++ b/tests/qapi-schema/qapi-schema-test.json >> @@ -32,11 +32,12 @@ >> 'dict1': 'UserDefTwoDict' } } >> >> # for testing unions >> +# name collisions between branches should not clash >> { 'struct': 'UserDefA', >> - 'data': { 'boolean': 'bool' } } >> + 'data': { 'boolean': 'bool', '*a_b': 'int' } } >> >> { 'struct': 'UserDefB', >> - 'data': { 'intb': 'int' } } >> + 'data': { 'intb': 'int', '*a-b': 'bool' } } >> >> { 'union': 'UserDefFlatUnion', >> 'base': 'UserDefUnionBase', # intentional forward reference > > This tests that different variants may have clashing names. Okay. That is, even though the variant is accepted at the same QMP {} level, only one variant at a time can be active, so clashes in names between variants is not fatal to either QMP or to the generated C code. > > I'm afraid the comment is a bit too terse. Not sure I'd make the > connection from it to member a_b and to UserDefB's a-b a fortnight from > now. > Then I get to beef it up for the next round :) >> +++ b/tests/qapi-schema/struct-base-clash2.json >> @@ -0,0 +1,5 @@ >> +# FIXME - a base class collides with a member named base >> +{ 'struct': 'Base', 'data': {} } >> +{ 'struct': 'Sub', >> + 'base': 'Base', >> + 'data': { 'base': 'str' } } > > What's this about? Hmm, I think it's about the way we do a struct > type's base. For a union type, we add the base's members, as shown > above. For a struct type, we add the base *boxed*, like this: > > struct Sub { > // The base type > Base *base; > // Own members > char *base; > }; > > Therefore, a struct type with a base can't have a member named base. > But that's simply daft. As soon as we change it to match union types, > this test case goes away. If we change it soon, do we still need this > test? Will it be done later in this series? Yes, we fix it up later in the series, at which point this test disappears. But having the test now makes it easier to see what the later patch is changing. >> +++ b/tests/qapi-schema/union-clash2.json >> @@ -0,0 +1,3 @@ >> +# FIXME - a union branch named 'data' collides with generated C code >> +{ 'union': 'TestUnion', >> + 'data': { 'data': 'int' } } > > This tests another stupid clash: we put a member named data in our > generated unions. As soon as we stop doing that, this test will go > away. If we stop soon, do we still need this test? Will we stop later > in this series? Yes, we fix it up later in the series, at which point this test disappears. But having the test now makes it easier to see what the later patch is changing.
Eric Blake <eblake@redhat.com> writes: > On 09/22/2015 09:23 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Expose some weaknesses in the generator: we don't always forbid >>> the generation of structs that contain multiple members that map >> >> Slightly misleading. args-name-clash is a clash between command >> arguments. These are a struct internally, but we don't currently >> generate an actual struct for it, only an argument list. > > Maybe struct-member-clash? Renames are easy enough, but only if patch > 1/46 is okay to go in first. :) Naming the test args-member-clash is fine, because that's what it tests. It also covers struct member clashes due to they way arguments work. The slightly misleading part is the commit message. >>> to the same C name. This has already been marked FIXME in >>> qapi.py, but having more tests will make sure future patches >>> produce desired behavior. >> >> Point to commit d90675f? > > Sure, now that it finally landed. > >> >>> Some of these tests will be deleted later, and a positive test >>> added to qapi-schema-test.json in its place, when the code is >> >> "in their place"? >> > > Yep. (Perils of editing, I started with one test, then added more later > and merged into one patch) > >>> reworked so that the collision no longer occurs. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- > >>> +++ b/tests/qapi-schema/flat-union-branch-clash.json >>> @@ -1,4 +1,4 @@ >>> -# we check for no duplicate keys between branches and base >>> +# we check for no duplicate keys between branch members and base >>> { 'enum': 'TestEnum', >>> 'data': [ 'value1', 'value2' ] } >>> { 'struct': 'Base', >> >> This clashing business is awfully confusing as soon as unions come into >> play. When I'm confused, I need to think in writing. > > No kidding. We already attempted to detect clashes, and caught some, > but not all, types of clashes. And there are indeed two types of member > name clashes: those where the generated C struct has duplicate members > (either because 2 user names map to the same C name, or because the > generated code injects a C name for a purpose other than a "key":value > name), and those where the qapi type would specify the same "key":value > name more than once in the same {} object on the wire (even if the names > would not collide in C because one is accessed through a box pointer). > By patch 16/46, we should be catching all cases of member name clashes, > but there's still work to do to catch collisions in 'command' and/or > 'event' names. > > Also, by the time 16/46 is in, there are cases where we reject "clashes" > where two member names with different spellings would map to the same C > name, but where the corresponding C struct does not have a clash because > the members are boxed behind different pointers. Technically, we would > not have to reject such cases, but the case is still confusing enough > that rejecting it forces the qapi writer to consider a naming convention > that is less confusing in the first place. That's fair. >> The basic case is clash between local, non-variant members. Needs test >> coverage. args-name-clash.json provides it, because internally the >> arguments are just another object type. > > Correct. The test proves we don't yet catch the clash, and is fixed > when later commits add the check. > >> >> With a base, the members inherited from base get added to the mix. We >> need to test a clash betwen local, non-variant member and a member >> inherited from base. > > True for both structs and flat unions (the two places where we use > 'base'). More on this below. > >> >> With unions, things get complicated, because we have multiple kinds of >> clashes. Best explained with an example. Let's use UserDefFlatUnion >> from qapi-schema-test.json. >> >> { 'union': 'UserDefFlatUnion', >> 'base': 'UserDefUnionBase', # intentional forward reference >> 'discriminator': 'enum1', >> 'data': { 'value1' : 'UserDefA', >> 'value2' : 'UserDefB', >> 'value3' : 'UserDefB' } } >> >> { 'struct': 'UserDefUnionBase', >> 'base': 'UserDefZero', >> 'data': { 'string': 'str', 'enum1': 'EnumOne' } } >> >> Generated C looks like this: >> >> struct UserDefFlatUnion { >> /* Members inherited from UserDefUnionBase: */ >> int64_t integer; >> char *string; >> EnumOne enum1; >> /* Own members: */ >> // if the schema language supported adding non-variant local >> // members, they'd go right here >> union { /* union tag is @enum1 */ >> void *data; >> UserDefA *value1; >> UserDefB *value2; >> UserDefB *value3; >> }; >> }; >> >> Thus, what can clash in C is the tag values value1, value2, value3 with >> the non-variant members integer, string, enum1. > > That is, the tag values now appear as C member names, even though they > did not correspond to QMP "key":value names. Likewise, the 'data' C > member name can cause a clash. > > Was even worse before commit 0f61af3e, where we were also burning the C > name 'kind'. > > Commit 1e6c1616 was where we quit burning the C member name 'base'. > Prior to that time, members of base classes did not clash with variant > names because of the C boxing. For union types. For struct types, we still box the base. I'd like to get rid of that. Even when the base is boxed, the members still clash in QMP. We also box the variants (e.g. UserDefA *value1 in the example above). Would be nice to get rid of that, too. > If we run into a situation where the enum values collide with base > member names (both of which are ABI), we could still solve the collision > by renaming the C member names for the enum values to something that > don't collide (such as _tag_value1 rather than value1); this is because > the C member names are not ABI and can be changed. But we can cross > that bridge later if the situation ever arises; for now, it's just > easier to patch the generator to reject qapi where such a collision > would occur. Agreed. >> On the wire, the union members are unboxed, i.e. we get just >> >> "boolean": false >> >> instead of >> >> "value1": { "boolean": false } >> >> Thus what can clash on the wire is the variant members with the >> non-variant members: boolean with integer, string, enum1 when enum1 is >> value1, and so forth. >> >> This is the clash flat-union-branch-clash.json tests. Its error message >> is "Member name 'name' of branch 'value1' clashes with base 'Base'". >> Suboptimal, it should say "with member 'name' of base 'Base'". > > Indeed, this is the other type of clash (QMP wire clashes, whether or > not they cause a C member clash). > >>> +++ b/tests/qapi-schema/flat-union-branch-clash2.json >>> @@ -0,0 +1,14 @@ >>> +# FIXME: we should check for no duplicate C names between branches and base >>> +{ 'enum': 'TestEnum', >>> + 'data': [ 'base', 'c-d' ] } >>> +{ 'struct': 'Base', >>> + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } >>> +{ 'struct': 'Branch1', >>> + 'data': { 'string': 'str' } } >>> +{ 'struct': 'Branch2', >>> + 'data': { 'value': 'int' } } >>> +{ 'union': 'TestUnion', >>> + 'base': 'Base', >>> + 'discriminator': 'enum1', >>> + 'data': { 'base': 'Branch1', >>> + 'c-d': 'Branch2' } } >> >> This tests the other kind of clash: tag value 'c-d' clashes with >> non-variant member name 'c_d'. >> >> Please add a comment explaining what clash should be reported here. > > Will do; and by the end of the series the error is properly reported. > >>> +++ b/tests/qapi-schema/flat-union-cycle.json >>> @@ -0,0 +1,6 @@ >>> +# we reject a loop in flat unions, due to member collision >>> +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] } >>> +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } } >>> +{ 'struct': 'Okay', 'data': { 'int': 'int' } } >>> +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch', >>> + 'data': { 'okay': 'Okay', 'loop': 'Base' } } >> >> This isn't a loop, it's a fork: we get the members of Base via its use >> as base, and again via its use as type of a variant case. >> >> What does it add over flat-union-branch-clash.json? > > I wrote this test when I discovered the assertion failure in the parser > bug as covered by patch 16/46 (a struct attempting to inherit directly > or indirectly from itself is not nice). Yes, that needs to be caught. > When I first wrote the test, I > was trying to make sure that a flat union cannot inherit from itself, > but then ran into the problem that a base class must be a struct and not > a union. Yes. That means base loops can only consist of struct types right now. > So I changed the test to make sure that QMP cannot reuse the > base class as a variant type, since that would require the members of > the base type to occur in QMP more than once, without seeing if any > other test already did that. > > You may have a point that this doesn't cover anything beyond > flat-union-branch-clash, and since my later changes to detect > self-inheritance didn't change the error message flagged for this case, > we can probably safely drop this test as not adding anything. > > And I guess I should still test that self-inheritance attempts are > rejected, even if we later relax things to allow a non-struct as a base > class. Replacing this test by one that actually tests base loops sounds good to me. If we ever accept other kinds of base types, we'll have to update the loop test, but that's normal. >>> +++ b/tests/qapi-schema/qapi-schema-test.json >>> @@ -32,11 +32,12 @@ >>> 'dict1': 'UserDefTwoDict' } } >>> >>> # for testing unions >>> +# name collisions between branches should not clash >>> { 'struct': 'UserDefA', >>> - 'data': { 'boolean': 'bool' } } >>> + 'data': { 'boolean': 'bool', '*a_b': 'int' } } >>> >>> { 'struct': 'UserDefB', >>> - 'data': { 'intb': 'int' } } >>> + 'data': { 'intb': 'int', '*a-b': 'bool' } } >>> >>> { 'union': 'UserDefFlatUnion', >>> 'base': 'UserDefUnionBase', # intentional forward reference >> >> This tests that different variants may have clashing names. Okay. > > That is, even though the variant is accepted at the same QMP {} level, > only one variant at a time can be active, so clashes in names between > variants is not fatal to either QMP or to the generated C code. Yes. >> I'm afraid the comment is a bit too terse. Not sure I'd make the >> connection from it to member a_b and to UserDefB's a-b a fortnight from >> now. >> > > Then I get to beef it up for the next round :) > > >>> +++ b/tests/qapi-schema/struct-base-clash2.json >>> @@ -0,0 +1,5 @@ >>> +# FIXME - a base class collides with a member named base >>> +{ 'struct': 'Base', 'data': {} } >>> +{ 'struct': 'Sub', >>> + 'base': 'Base', >>> + 'data': { 'base': 'str' } } >> >> What's this about? Hmm, I think it's about the way we do a struct >> type's base. For a union type, we add the base's members, as shown >> above. For a struct type, we add the base *boxed*, like this: >> >> struct Sub { >> // The base type >> Base *base; >> // Own members >> char *base; >> }; >> >> Therefore, a struct type with a base can't have a member named base. >> But that's simply daft. As soon as we change it to match union types, >> this test case goes away. If we change it soon, do we still need this >> test? Will it be done later in this series? > > Yes, we fix it up later in the series, at which point this test > disappears. But having the test now makes it easier to see what the > later patch is changing. I'm not sure I'd bother myself, but I gladly defer to your judgement here. >>> +++ b/tests/qapi-schema/union-clash2.json >>> @@ -0,0 +1,3 @@ >>> +# FIXME - a union branch named 'data' collides with generated C code >>> +{ 'union': 'TestUnion', >>> + 'data': { 'data': 'int' } } >> >> This tests another stupid clash: we put a member named data in our >> generated unions. As soon as we stop doing that, this test will go >> away. If we stop soon, do we still need this test? Will we stop later >> in this series? > > Yes, we fix it up later in the series, at which point this test > disappears. But having the test now makes it easier to see what the > later patch is changing. Likewise.
On 09/23/2015 03:43 AM, Markus Armbruster wrote: >> Commit 1e6c1616 was where we quit burning the C member name 'base'. >> Prior to that time, members of base classes did not clash with variant >> names because of the C boxing. > > For union types. For struct types, we still box the base. I'd like to > get rid of that. Patch 34/46 :) > > Even when the base is boxed, the members still clash in QMP. > > We also box the variants (e.g. UserDefA *value1 in the example above). > Would be nice to get rid of that, too. What do you mean? Here's an example of current boxed code: enum EnumType { ENUM_TYPE_ONE, ENUM_TYPE_TWO, }; struct One { int a; }; struct Two { char *a; }; struct Union { EnumType type; /* union tag is @type */ union { One *one; Two *two; }; }; Is this what you envision for unboxed? Note that we still have to namespace things properly (we have to have union.one.a and union.two.a, and not a direct union.a), so all we'd be saving is the additional allocation of the variant pointers. struct Union { EnumType type; /* union tag is @type */ union { struct { int a; } one; struct { char *a; } two; }; }; However, I'm not sure it would always help. The conversion of netdev_add to full qapi relies on being able to access the variant through a named struct (such as NetdevTapOptions); unboxing the variant would get rid of the convenient access to these named sub-structs.
Eric Blake <eblake@redhat.com> writes: > On 09/23/2015 03:43 AM, Markus Armbruster wrote: > >>> Commit 1e6c1616 was where we quit burning the C member name 'base'. >>> Prior to that time, members of base classes did not clash with variant >>> names because of the C boxing. >> >> For union types. For struct types, we still box the base. I'd like to >> get rid of that. > > Patch 34/46 :) Okay :) >> Even when the base is boxed, the members still clash in QMP. >> >> We also box the variants (e.g. UserDefA *value1 in the example above). >> Would be nice to get rid of that, too. > > What do you mean? Here's an example of current boxed code: > > enum EnumType { > ENUM_TYPE_ONE, > ENUM_TYPE_TWO, > }; > struct One { > int a; > }; > struct Two { > char *a; > }; > struct Union { > EnumType type; > /* union tag is @type */ > union { > One *one; > Two *two; > }; > }; > > Is this what you envision for unboxed? Note that we still have to > namespace things properly (we have to have union.one.a and union.two.a, > and not a direct union.a), so all we'd be saving is the additional > allocation of the variant pointers. > > struct Union { > EnumType type; > /* union tag is @type */ > union { > struct { > int a; > } one; > struct { > char *a; > } two; > }; > }; > > However, I'm not sure it would always help. The conversion of > netdev_add to full qapi relies on being able to access the variant > through a named struct (such as NetdevTapOptions); unboxing the variant > would get rid of the convenient access to these named sub-structs. struct Union { EnumType type; /* union tag is @type */ union { One one; Two two; }; }; For base, we go one step further and peel off the struct, to save some notational overhead. Pointless for unions.
On 09/23/2015 08:02 AM, Markus Armbruster wrote: >> However, I'm not sure it would always help. The conversion of >> netdev_add to full qapi relies on being able to access the variant >> through a named struct (such as NetdevTapOptions); unboxing the variant >> would get rid of the convenient access to these named sub-structs. > > struct Union { > EnumType type; > /* union tag is @type */ > union { > One one; > Two two; > }; > }; > > For base, we go one step further and peel off the struct, to save some > notational overhead. Pointless for unions. Ah, I see. Instead of malloc'ing a sub-struct and calling (roughly) ptr = visit_start_struct(Union) // mallocs subptr = visit_start_implicit_struct(One) // also mallocs visit_type_fields(subptr) visit_end_implicit_struct() we would instead use inline allocation, with: ptr = visit_start_struct(Union) // mallocs visit_type_fields(&ptr->one) seems straightforward enough; I'll play with the idea on top of my series.
Eric Blake <eblake@redhat.com> writes: > On 09/23/2015 08:02 AM, Markus Armbruster wrote: > >>> However, I'm not sure it would always help. The conversion of >>> netdev_add to full qapi relies on being able to access the variant >>> through a named struct (such as NetdevTapOptions); unboxing the variant >>> would get rid of the convenient access to these named sub-structs. >> >> struct Union { >> EnumType type; >> /* union tag is @type */ >> union { >> One one; >> Two two; >> }; >> }; >> >> For base, we go one step further and peel off the struct, to save some >> notational overhead. Pointless for unions. > > Ah, I see. Instead of malloc'ing a sub-struct and calling (roughly) > > ptr = visit_start_struct(Union) // mallocs > subptr = visit_start_implicit_struct(One) // also mallocs > visit_type_fields(subptr) > visit_end_implicit_struct() > > we would instead use inline allocation, with: > > ptr = visit_start_struct(Union) // mallocs > visit_type_fields(&ptr->one) > > seems straightforward enough; I'll play with the idea on top of my series. I should be careful what I wish for lest the series grows faster than I can review it!
diff --git a/tests/Makefile b/tests/Makefile index 6fd5885..97434f6 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -238,6 +238,7 @@ qapi-schema += args-invalid.json qapi-schema += args-member-array-bad.json qapi-schema += args-member-array.json qapi-schema += args-member-unknown.json +qapi-schema += args-name-clash.json qapi-schema += args-union.json qapi-schema += args-unknown.json qapi-schema += bad-base.json @@ -274,6 +275,8 @@ qapi-schema += flat-union-bad-discriminator.json qapi-schema += flat-union-base-any.json qapi-schema += flat-union-base-union.json qapi-schema += flat-union-branch-clash.json +qapi-schema += flat-union-branch-clash2.json +qapi-schema += flat-union-cycle.json qapi-schema += flat-union-inline.json qapi-schema += flat-union-int-branch.json qapi-schema += flat-union-invalid-branch-key.json @@ -317,6 +320,7 @@ qapi-schema += returns-unknown.json qapi-schema += returns-whitelist.json qapi-schema += struct-base-clash-deep.json qapi-schema += struct-base-clash.json +qapi-schema += struct-base-clash2.json qapi-schema += struct-data-invalid.json qapi-schema += struct-member-invalid.json qapi-schema += trailing-comma-list.json @@ -328,6 +332,8 @@ qapi-schema += unclosed-string.json qapi-schema += unicode-str.json qapi-schema += union-bad-branch.json qapi-schema += union-base-no-discriminator.json +qapi-schema += union-clash.json +qapi-schema += union-clash2.json qapi-schema += union-invalid-base.json qapi-schema += union-max.json qapi-schema += union-optional-branch.json diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/args-name-clash.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json new file mode 100644 index 0000000..19bf792 --- /dev/null +++ b/tests/qapi-schema/args-name-clash.json @@ -0,0 +1,2 @@ +# FIXME - we should reject data with members that clash when mapped to C names +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out new file mode 100644 index 0000000..9b2f6e4 --- /dev/null +++ b/tests/qapi-schema/args-name-clash.out @@ -0,0 +1,6 @@ +object :empty +object :obj-oops-arg + member a-b: str optional=False + member a_b: str optional=False +command oops :obj-oops-arg -> None + gen=True success_response=True diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-branch-clash.json index 8fb054f..3d7e6c6 100644 --- a/tests/qapi-schema/flat-union-branch-clash.json +++ b/tests/qapi-schema/flat-union-branch-clash.json @@ -1,4 +1,4 @@ -# we check for no duplicate keys between branches and base +# we check for no duplicate keys between branch members and base { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } { 'struct': 'Base', diff --git a/tests/qapi-schema/flat-union-branch-clash2.err b/tests/qapi-schema/flat-union-branch-clash2.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-branch-clash2.exit b/tests/qapi-schema/flat-union-branch-clash2.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/flat-union-branch-clash2.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/flat-union-branch-clash2.json b/tests/qapi-schema/flat-union-branch-clash2.json new file mode 100644 index 0000000..b3eefb3 --- /dev/null +++ b/tests/qapi-schema/flat-union-branch-clash2.json @@ -0,0 +1,14 @@ +# FIXME: we should check for no duplicate C names between branches and base +{ 'enum': 'TestEnum', + 'data': [ 'base', 'c-d' ] } +{ 'struct': 'Base', + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } +{ 'struct': 'Branch1', + 'data': { 'string': 'str' } } +{ 'struct': 'Branch2', + 'data': { 'value': 'int' } } +{ 'union': 'TestUnion', + 'base': 'Base', + 'discriminator': 'enum1', + 'data': { 'base': 'Branch1', + 'c-d': 'Branch2' } } diff --git a/tests/qapi-schema/flat-union-branch-clash2.out b/tests/qapi-schema/flat-union-branch-clash2.out new file mode 100644 index 0000000..8e0da73 --- /dev/null +++ b/tests/qapi-schema/flat-union-branch-clash2.out @@ -0,0 +1,14 @@ +object :empty +object Base + member enum1: TestEnum optional=False + member c_d: str optional=True +object Branch1 + member string: str optional=False +object Branch2 + member value: int optional=False +enum TestEnum ['base', 'c-d'] +object TestUnion + base Base + tag enum1 + case base: Branch1 + case c-d: Branch2 diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err new file mode 100644 index 0000000..152c6f0 --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-cycle.json:5: Member name 'switch' of branch 'loop' clashes with base 'Base' diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json new file mode 100644 index 0000000..8f6cd93 --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.json @@ -0,0 +1,6 @@ +# we reject a loop in flat unions, due to member collision +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] } +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } } +{ 'struct': 'Okay', 'data': { 'int': 'int' } } +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch', + 'data': { 'okay': 'Okay', 'loop': 'Base' } } diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 6897a6e..c904db4 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -32,11 +32,12 @@ 'dict1': 'UserDefTwoDict' } } # for testing unions +# name collisions between branches should not clash { 'struct': 'UserDefA', - 'data': { 'boolean': 'bool' } } + 'data': { 'boolean': 'bool', '*a_b': 'int' } } { 'struct': 'UserDefB', - 'data': { 'intb': 'int' } } + 'data': { 'intb': 'int', '*a-b': 'bool' } } { 'union': 'UserDefFlatUnion', 'base': 'UserDefUnionBase', # intentional forward reference diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 1f6e858..28a0b3c 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2'] prefix QENUM_TWO object UserDefA member boolean: bool optional=False + member a_b: int optional=True alternate UserDefAlternate case uda: UserDefA case s: str @@ -78,6 +79,7 @@ alternate UserDefAlternate enum UserDefAlternateKind ['uda', 's', 'i'] object UserDefB member intb: int optional=False + member a-b: bool optional=True object UserDefC member string1: str optional=False member string2: str optional=False diff --git a/tests/qapi-schema/struct-base-clash2.err b/tests/qapi-schema/struct-base-clash2.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/struct-base-clash2.exit b/tests/qapi-schema/struct-base-clash2.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/struct-base-clash2.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/struct-base-clash2.json b/tests/qapi-schema/struct-base-clash2.json new file mode 100644 index 0000000..56166e0 --- /dev/null +++ b/tests/qapi-schema/struct-base-clash2.json @@ -0,0 +1,5 @@ +# FIXME - a base class collides with a member named base +{ 'struct': 'Base', 'data': {} } +{ 'struct': 'Sub', + 'base': 'Base', + 'data': { 'base': 'str' } } diff --git a/tests/qapi-schema/struct-base-clash2.out b/tests/qapi-schema/struct-base-clash2.out new file mode 100644 index 0000000..e69a416 --- /dev/null +++ b/tests/qapi-schema/struct-base-clash2.out @@ -0,0 +1,5 @@ +object :empty +object Base +object Sub + base Base + member base: str optional=False diff --git a/tests/qapi-schema/union-clash.err b/tests/qapi-schema/union-clash.err new file mode 100644 index 0000000..64637ed --- /dev/null +++ b/tests/qapi-schema/union-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/union-clash.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b' diff --git a/tests/qapi-schema/union-clash.exit b/tests/qapi-schema/union-clash.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/union-clash.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/union-clash.json b/tests/qapi-schema/union-clash.json new file mode 100644 index 0000000..0393ed8 --- /dev/null +++ b/tests/qapi-schema/union-clash.json @@ -0,0 +1,3 @@ +# we reject unions where branch names clash when mapped to C +{ 'union': 'TestUnion', + 'data': { 'a-b': 'int', 'a_b': 'str' } } diff --git a/tests/qapi-schema/union-clash.out b/tests/qapi-schema/union-clash.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/union-clash2.err b/tests/qapi-schema/union-clash2.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/union-clash2.exit b/tests/qapi-schema/union-clash2.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/union-clash2.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/union-clash2.json b/tests/qapi-schema/union-clash2.json new file mode 100644 index 0000000..b2d45fb --- /dev/null +++ b/tests/qapi-schema/union-clash2.json @@ -0,0 +1,3 @@ +# FIXME - a union branch named 'data' collides with generated C code +{ 'union': 'TestUnion', + 'data': { 'data': 'int' } } diff --git a/tests/qapi-schema/union-clash2.out b/tests/qapi-schema/union-clash2.out new file mode 100644 index 0000000..6277239 --- /dev/null +++ b/tests/qapi-schema/union-clash2.out @@ -0,0 +1,6 @@ +object :empty +object :obj-int-wrapper + member data: int optional=False +object TestUnion + case data: :obj-int-wrapper +enum TestUnionKind ['data']
Expose some weaknesses in the generator: we don't always forbid the generation of structs that contain multiple members that map to the same C name. This has already been marked FIXME in qapi.py, but having more tests will make sure future patches produce desired behavior. Some of these tests will be deleted later, and a positive test added to qapi-schema-test.json in its place, when the code is reworked so that the collision no longer occurs. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/Makefile | 6 ++++++ tests/qapi-schema/args-name-clash.err | 0 tests/qapi-schema/args-name-clash.exit | 1 + tests/qapi-schema/args-name-clash.json | 2 ++ tests/qapi-schema/args-name-clash.out | 6 ++++++ tests/qapi-schema/flat-union-branch-clash.json | 2 +- tests/qapi-schema/flat-union-branch-clash2.err | 0 tests/qapi-schema/flat-union-branch-clash2.exit | 1 + tests/qapi-schema/flat-union-branch-clash2.json | 14 ++++++++++++++ tests/qapi-schema/flat-union-branch-clash2.out | 14 ++++++++++++++ tests/qapi-schema/flat-union-cycle.err | 1 + tests/qapi-schema/flat-union-cycle.exit | 1 + tests/qapi-schema/flat-union-cycle.json | 6 ++++++ tests/qapi-schema/flat-union-cycle.out | 0 tests/qapi-schema/qapi-schema-test.json | 5 +++-- tests/qapi-schema/qapi-schema-test.out | 2 ++ tests/qapi-schema/struct-base-clash2.err | 0 tests/qapi-schema/struct-base-clash2.exit | 1 + tests/qapi-schema/struct-base-clash2.json | 5 +++++ tests/qapi-schema/struct-base-clash2.out | 5 +++++ tests/qapi-schema/union-clash.err | 1 + tests/qapi-schema/union-clash.exit | 1 + tests/qapi-schema/union-clash.json | 3 +++ tests/qapi-schema/union-clash.out | 0 tests/qapi-schema/union-clash2.err | 0 tests/qapi-schema/union-clash2.exit | 1 + tests/qapi-schema/union-clash2.json | 3 +++ tests/qapi-schema/union-clash2.out | 6 ++++++ 28 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 tests/qapi-schema/args-name-clash.err create mode 100644 tests/qapi-schema/args-name-clash.exit create mode 100644 tests/qapi-schema/args-name-clash.json create mode 100644 tests/qapi-schema/args-name-clash.out create mode 100644 tests/qapi-schema/flat-union-branch-clash2.err create mode 100644 tests/qapi-schema/flat-union-branch-clash2.exit create mode 100644 tests/qapi-schema/flat-union-branch-clash2.json create mode 100644 tests/qapi-schema/flat-union-branch-clash2.out create mode 100644 tests/qapi-schema/flat-union-cycle.err create mode 100644 tests/qapi-schema/flat-union-cycle.exit create mode 100644 tests/qapi-schema/flat-union-cycle.json create mode 100644 tests/qapi-schema/flat-union-cycle.out create mode 100644 tests/qapi-schema/struct-base-clash2.err create mode 100644 tests/qapi-schema/struct-base-clash2.exit create mode 100644 tests/qapi-schema/struct-base-clash2.json create mode 100644 tests/qapi-schema/struct-base-clash2.out create mode 100644 tests/qapi-schema/union-clash.err create mode 100644 tests/qapi-schema/union-clash.exit create mode 100644 tests/qapi-schema/union-clash.json create mode 100644 tests/qapi-schema/union-clash.out create mode 100644 tests/qapi-schema/union-clash2.err create mode 100644 tests/qapi-schema/union-clash2.exit create mode 100644 tests/qapi-schema/union-clash2.json create mode 100644 tests/qapi-schema/union-clash2.out