* [PATCH] qapi: allow unions to contain further unions @ 2023-02-14 18:44 Daniel P. Berrangé 2023-02-23 7:24 ` Markus Armbruster 0 siblings, 1 reply; 3+ messages in thread From: Daniel P. Berrangé @ 2023-02-14 18:44 UTC (permalink / raw) To: qemu-devel Cc: Michael Roth, Eric Blake, Markus Armbruster, Het Gala, Daniel P. Berrangé This extends the QAPI schema validation to permit unions inside unions, provided the checks for clashing fields pass. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- This patch comes out of the discussion on Het's migration series starting at this patch: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02111.html Markus had described his desired improved architecture https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02719.html but I don't think I have enough knowledge of the QAPI code to attempt to fuse the handling of structs/unions as mentioned. This patch does what looks to be the bare minimum to permit unions in unions, while keeping validation checks for clashing fields. I've not tested beyond the unit tests, but if this is acceptable from Markus' POV, I'd expect Het to insert this patch at the start of his migration series and thus test it more fully. scripts/qapi/schema.py | 6 +-- .../union-invalid-union-subfield.err | 2 + .../union-invalid-union-subfield.json | 27 +++++++++++++ .../union-invalid-union-subfield.out | 0 .../union-invalid-union-subtype.err | 2 + .../union-invalid-union-subtype.json | 26 +++++++++++++ .../union-invalid-union-subtype.out | 0 tests/qapi-schema/union-union-branch.err | 0 tests/qapi-schema/union-union-branch.json | 26 +++++++++++++ tests/qapi-schema/union-union-branch.out | 38 +++++++++++++++++++ 10 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 tests/qapi-schema/union-invalid-union-subfield.err create mode 100644 tests/qapi-schema/union-invalid-union-subfield.json create mode 100644 tests/qapi-schema/union-invalid-union-subfield.out create mode 100644 tests/qapi-schema/union-invalid-union-subtype.err create mode 100644 tests/qapi-schema/union-invalid-union-subtype.json create mode 100644 tests/qapi-schema/union-invalid-union-subtype.out create mode 100644 tests/qapi-schema/union-union-branch.err create mode 100644 tests/qapi-schema/union-union-branch.json create mode 100644 tests/qapi-schema/union-union-branch.out diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index cd8661125c..062c6bbb00 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -465,9 +465,10 @@ def check(self, schema): # on behalf of info, which is not necessarily self.info def check_clash(self, info, seen): assert self._checked - assert not self.variants # not implemented for m in self.members: m.check_clash(info, seen) + if self.variants: + self.variants.check_clash(info, seen) def connect_doc(self, doc=None): super().connect_doc(doc) @@ -652,8 +653,7 @@ def check(self, schema, seen): self.info, "branch '%s' is not a value of %s" % (v.name, self.tag_member.type.describe())) - if (not isinstance(v.type, QAPISchemaObjectType) - or v.type.variants): + if not isinstance(v.type, QAPISchemaObjectType): raise QAPISemError( self.info, "%s cannot use %s" diff --git a/tests/qapi-schema/union-invalid-union-subfield.err b/tests/qapi-schema/union-invalid-union-subfield.err new file mode 100644 index 0000000000..d3a2e31aff --- /dev/null +++ b/tests/qapi-schema/union-invalid-union-subfield.err @@ -0,0 +1,2 @@ +union-invalid-union-subfield.json: In union 'TestUnion': +union-invalid-union-subfield.json:22: member 'teeth' of type 'TestTypeFish' collides with base member 'teeth' diff --git a/tests/qapi-schema/union-invalid-union-subfield.json b/tests/qapi-schema/union-invalid-union-subfield.json new file mode 100644 index 0000000000..235f76d7da --- /dev/null +++ b/tests/qapi-schema/union-invalid-union-subfield.json @@ -0,0 +1,27 @@ +{ 'enum': 'TestEnum', + 'data': [ 'animals', 'plants' ] } + +{ 'enum': 'TestAnimals', + 'data': [ 'fish', 'birds'] } + +{ 'struct': 'TestTypeFish', + 'data': { 'scales': 'int', 'teeth': 'int' } } + +{ 'struct': 'TestTypeBirds', + 'data': { 'feathers': 'int' } } + +{ 'union': 'TestTypeAnimals', + 'base': { 'atype': 'TestAnimals' }, + 'discriminator': 'atype', + 'data': { 'fish': 'TestTypeFish', + 'birds': 'TestTypeBirds' } } + +{ 'struct': 'TestTypePlants', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': { 'type': 'TestEnum', + 'teeth': 'int' }, + 'discriminator': 'type', + 'data': { 'animals': 'TestTypeAnimals', + 'plants': 'TestTypePlants' } } diff --git a/tests/qapi-schema/union-invalid-union-subfield.out b/tests/qapi-schema/union-invalid-union-subfield.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/union-invalid-union-subtype.err b/tests/qapi-schema/union-invalid-union-subtype.err new file mode 100644 index 0000000000..7b8679c08f --- /dev/null +++ b/tests/qapi-schema/union-invalid-union-subtype.err @@ -0,0 +1,2 @@ +union-invalid-union-subtype.json: In union 'TestUnion': +union-invalid-union-subtype.json:22: base member 'type' collides with base member 'type' diff --git a/tests/qapi-schema/union-invalid-union-subtype.json b/tests/qapi-schema/union-invalid-union-subtype.json new file mode 100644 index 0000000000..59ca4b0385 --- /dev/null +++ b/tests/qapi-schema/union-invalid-union-subtype.json @@ -0,0 +1,26 @@ +{ 'enum': 'TestEnum', + 'data': [ 'value-a', 'value-b' ] } + +{ 'enum': 'TestEnumA', + 'data': [ 'value-a1', 'value-a2' ] } + +{ 'struct': 'TestTypeA1', + 'data': { 'integer': 'int' } } + +{ 'struct': 'TestTypeA2', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestTypeA', + 'base': { 'type': 'TestEnumA' }, + 'discriminator': 'type', + 'data': { 'value-a1': 'TestTypeA1', + 'value-a2': 'TestTypeA2' } } + +{ 'struct': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': { 'type': 'TestEnum' }, + 'discriminator': 'type', + 'data': { 'value-a': 'TestTypeA', + 'value-b': 'TestTypeB' } } diff --git a/tests/qapi-schema/union-invalid-union-subtype.out b/tests/qapi-schema/union-invalid-union-subtype.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/union-union-branch.err b/tests/qapi-schema/union-union-branch.err new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/union-union-branch.json b/tests/qapi-schema/union-union-branch.json new file mode 100644 index 0000000000..d3d7ce57c6 --- /dev/null +++ b/tests/qapi-schema/union-union-branch.json @@ -0,0 +1,26 @@ +{ 'enum': 'TestEnum', + 'data': [ 'value-a', 'value-b' ] } + +{ 'enum': 'TestEnumA', + 'data': [ 'value-a1', 'value-a2' ] } + +{ 'struct': 'TestTypeA1', + 'data': { 'integer': 'int' } } + +{ 'struct': 'TestTypeA2', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestTypeA', + 'base': { 'type-a': 'TestEnumA' }, + 'discriminator': 'type-a', + 'data': { 'value-a1': 'TestTypeA1', + 'value-a2': 'TestTypeA2' } } + +{ 'struct': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': { 'type': 'TestEnum' }, + 'discriminator': 'type', + 'data': { 'value-a': 'TestTypeA', + 'value-b': 'TestTypeB' } } diff --git a/tests/qapi-schema/union-union-branch.out b/tests/qapi-schema/union-union-branch.out new file mode 100644 index 0000000000..d0c37495c2 --- /dev/null +++ b/tests/qapi-schema/union-union-branch.out @@ -0,0 +1,38 @@ +module ./builtin +object q_empty +enum QType + prefix QTYPE + member none + member qnull + member qnum + member qstring + member qdict + member qlist + member qbool +module union-union-branch.json +enum TestEnum + member value-a + member value-b +enum TestEnumA + member value-a1 + member value-a2 +object TestTypeA1 + member integer: int optional=False +object TestTypeA2 + member integer: int optional=False +object q_obj_TestTypeA-base + member type-a: TestEnumA optional=False +object TestTypeA + base q_obj_TestTypeA-base + tag type-a + case value-a1: TestTypeA1 + case value-a2: TestTypeA2 +object TestTypeB + member integer: int optional=False +object q_obj_TestUnion-base + member type: TestEnum optional=False +object TestUnion + base q_obj_TestUnion-base + tag type + case value-a: TestTypeA + case value-b: TestTypeB -- 2.39.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] qapi: allow unions to contain further unions 2023-02-14 18:44 [PATCH] qapi: allow unions to contain further unions Daniel P. Berrangé @ 2023-02-23 7:24 ` Markus Armbruster 2023-02-23 13:40 ` Daniel P. Berrangé 0 siblings, 1 reply; 3+ messages in thread From: Markus Armbruster @ 2023-02-23 7:24 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: qemu-devel, Michael Roth, Eric Blake, Het Gala Daniel P. Berrangé <berrange@redhat.com> writes: > This extends the QAPI schema validation to permit unions inside unions, > provided the checks for clashing fields pass. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > > This patch comes out of the discussion on Het's migration series > starting at this patch: > > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02111.html > > Markus had described his desired improved architecture > > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02719.html > > but I don't think I have enough knowledge of the QAPI code to attempt > to fuse the handling of structs/unions as mentioned. This patch does > what looks to be the bare minimum to permit unions in unions, while > keeping validation checks for clashing fields. > > I've not tested beyond the unit tests, but if this is acceptable > from Markus' POV, I'd expect Het to insert this patch at the > start of his migration series and thus test it more fully. > > scripts/qapi/schema.py | 6 +-- > .../union-invalid-union-subfield.err | 2 + > .../union-invalid-union-subfield.json | 27 +++++++++++++ > .../union-invalid-union-subfield.out | 0 > .../union-invalid-union-subtype.err | 2 + > .../union-invalid-union-subtype.json | 26 +++++++++++++ > .../union-invalid-union-subtype.out | 0 > tests/qapi-schema/union-union-branch.err | 0 > tests/qapi-schema/union-union-branch.json | 26 +++++++++++++ > tests/qapi-schema/union-union-branch.out | 38 +++++++++++++++++++ > 10 files changed, 124 insertions(+), 3 deletions(-) > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.err > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.json > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.out > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.err > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.json > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.out > create mode 100644 tests/qapi-schema/union-union-branch.err > create mode 100644 tests/qapi-schema/union-union-branch.json > create mode 100644 tests/qapi-schema/union-union-branch.out > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index cd8661125c..062c6bbb00 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -465,9 +465,10 @@ def check(self, schema): > # on behalf of info, which is not necessarily self.info > def check_clash(self, info, seen): > assert self._checked > - assert not self.variants # not implemented > for m in self.members: > m.check_clash(info, seen) > + if self.variants: > + self.variants.check_clash(info, seen) Note for later: the .check_clash() methods are responsible for rejecting clashing members, with an error message of the form "X collides with Y". Fine print 1: members clash when their names both map to the same C name. For instance, 'a-b' collides with 'a_b'. Fine print 2: the special case of identical keys in a single JSON-ish object is already rejected by the parser, with an error message of the form "duplicate key 'KEY'". > > def connect_doc(self, doc=None): > super().connect_doc(doc) > @@ -652,8 +653,7 @@ def check(self, schema, seen): > self.info, > "branch '%s' is not a value of %s" > % (v.name, self.tag_member.type.describe())) > - if (not isinstance(v.type, QAPISchemaObjectType) > - or v.type.variants): > + if not isinstance(v.type, QAPISchemaObjectType): > raise QAPISemError( > self.info, > "%s cannot use %s" This lifts the restriction; an object type's variant type may now have variants. Could affect any code that deals with object type members. Best case: the code just works. Okay case: the code asserts there are no variants. This patch needs to make it work instead. One known instance: check_clash() above. I looked for more, and there are a few "no variants" assertions, but they are all unrelated. Worst case: the code doesn't work. This patch needs to make it work. No known instances. Two complementary ways to convince ourselves everything works: systematic code inspection, systematic tests. The former looks at every place where we do something with object type members. I may try that later. For systematic tests, we need to understand what can go wrong, and what needs to work. I tried to work out a detailed argument, but it didn't come together. Best I can do is to simply propose that the additional variant members of a union's branch may clash with the union's common members, but not with any other branch's members, and that's all. We need to test the clash is rejected (negative test), and we need to test the non-clash works (positive test). Additionally, we should at least inspect the code generated for the positive test. > diff --git a/tests/qapi-schema/union-invalid-union-subfield.err b/tests/qapi-schema/union-invalid-union-subfield.err > new file mode 100644 > index 0000000000..d3a2e31aff > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-union-subfield.err > @@ -0,0 +1,2 @@ > +union-invalid-union-subfield.json: In union 'TestUnion': > +union-invalid-union-subfield.json:22: member 'teeth' of type 'TestTypeFish' collides with base member 'teeth' > diff --git a/tests/qapi-schema/union-invalid-union-subfield.json b/tests/qapi-schema/union-invalid-union-subfield.json > new file mode 100644 > index 0000000000..235f76d7da > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-union-subfield.json > @@ -0,0 +1,27 @@ This is the negative test I asked for above. Good. We commonly start with a comment on what is being tested. Suggest # Clash between common member and union variant's variant member # Base's member 'teeth' clashes with TestTypeFish's > +{ 'enum': 'TestEnum', > + 'data': [ 'animals', 'plants' ] } > + > +{ 'enum': 'TestAnimals', > + 'data': [ 'fish', 'birds'] } > + > +{ 'struct': 'TestTypeFish', > + 'data': { 'scales': 'int', 'teeth': 'int' } } > + > +{ 'struct': 'TestTypeBirds', > + 'data': { 'feathers': 'int' } } > + > +{ 'union': 'TestTypeAnimals', > + 'base': { 'atype': 'TestAnimals' }, > + 'discriminator': 'atype', > + 'data': { 'fish': 'TestTypeFish', > + 'birds': 'TestTypeBirds' } } > + > +{ 'struct': 'TestTypePlants', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': { 'type': 'TestEnum', > + 'teeth': 'int' }, Indentation is off. > + 'discriminator': 'type', > + 'data': { 'animals': 'TestTypeAnimals', > + 'plants': 'TestTypePlants' } } > diff --git a/tests/qapi-schema/union-invalid-union-subfield.out b/tests/qapi-schema/union-invalid-union-subfield.out > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/union-invalid-union-subtype.err b/tests/qapi-schema/union-invalid-union-subtype.err > new file mode 100644 > index 0000000000..7b8679c08f > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-union-subtype.err > @@ -0,0 +1,2 @@ > +union-invalid-union-subtype.json: In union 'TestUnion': > +union-invalid-union-subtype.json:22: base member 'type' collides with base member 'type' The error message is crap. See below. > diff --git a/tests/qapi-schema/union-invalid-union-subtype.json b/tests/qapi-schema/union-invalid-union-subtype.json > new file mode 100644 > index 0000000000..59ca4b0385 > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-union-subtype.json > @@ -0,0 +1,26 @@ Suggest # Clash between common member and union variant's common member # Base's member 'type' clashes with TestTypeA's > +{ 'enum': 'TestEnum', > + 'data': [ 'value-a', 'value-b' ] } > + > +{ 'enum': 'TestEnumA', > + 'data': [ 'value-a1', 'value-a2' ] } > + > +{ 'struct': 'TestTypeA1', > + 'data': { 'integer': 'int' } } > + > +{ 'struct': 'TestTypeA2', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestTypeA', > + 'base': { 'type': 'TestEnumA' }, > + 'discriminator': 'type', > + 'data': { 'value-a1': 'TestTypeA1', > + 'value-a2': 'TestTypeA2' } } > + > +{ 'struct': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': { 'type': 'TestEnum' }, > + 'discriminator': 'type', > + 'data': { 'value-a': 'TestTypeA', > + 'value-b': 'TestTypeB' } } This new test is structurally similar to existing test union-clash-member. Synopsis: { 'enum': 'TestEnum', { 'enum': 'TestEnum', 'data': [ 'value-a', 'value-b' ] } 'data': [ 'value1', 'value2' ] } { 'struct': 'Base', 'data': { 'enum1': 'TestEnum', '*name': 'str' } } { 'enum': 'TestEnumA', 'data': [ 'value-a1', 'value-a2' ] } { 'struct': 'TestTypeA1', 'data': { 'integer': 'int' } } { 'struct': 'TestTypeA2', 'data': { 'integer': 'int' } } { 'union': 'TestTypeA', { 'struct': 'Branch1', 'base': { 'type': 'TestEnumA' }, 'data': { 'name': 'str' } } 'discriminator': 'type', 'data': { 'value-a1': 'TestTypeA1', 'value-a2': 'TestTypeA2' } } { 'struct': 'TestTypeB', { 'struct': 'Branch2', 'data': { 'integer': 'int' } } 'data': { 'value': 'int' } } { 'union': 'TestUnion', { 'union': 'TestUnion', 'base': { 'type': 'TestEnum' }, 'base': 'Base', 'discriminator': 'type', 'discriminator': 'enum1', 'data': { 'value-a': 'TestTypeA', 'data': { 'value1': 'Branch1', 'value-b': 'TestTypeB' } } 'value2': 'Branch2' } } Differences: * Names and scalar types, which shouldn't matter. * Base is inline vs. explicit type; shouldn't matter. * First branch is a union vs. a struct. Since the error is about a common union member, this shouldn't matter, either. But it does: the error message is much worse, namely base member 'type' collides with base member 'type' vs. member 'name' of type 'Branch1' collides with member 'name' of type 'Base' Something's off here. > diff --git a/tests/qapi-schema/union-invalid-union-subtype.out b/tests/qapi-schema/union-invalid-union-subtype.out > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/union-union-branch.err b/tests/qapi-schema/union-union-branch.err > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/union-union-branch.json b/tests/qapi-schema/union-union-branch.json > new file mode 100644 > index 0000000000..d3d7ce57c6 > --- /dev/null > +++ b/tests/qapi-schema/union-union-branch.json > @@ -0,0 +1,26 @@ > +{ 'enum': 'TestEnum', > + 'data': [ 'value-a', 'value-b' ] } > + > +{ 'enum': 'TestEnumA', > + 'data': [ 'value-a1', 'value-a2' ] } > + > +{ 'struct': 'TestTypeA1', > + 'data': { 'integer': 'int' } } > + > +{ 'struct': 'TestTypeA2', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestTypeA', > + 'base': { 'type-a': 'TestEnumA' }, > + 'discriminator': 'type-a', > + 'data': { 'value-a1': 'TestTypeA1', > + 'value-a2': 'TestTypeA2' } } > + > +{ 'struct': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': { 'type': 'TestEnum' }, > + 'discriminator': 'type', > + 'data': { 'value-a': 'TestTypeA', > + 'value-b': 'TestTypeB' } } This is union-invalid-union-subtype.json with the clash fixed by renaming TestTypeA's member 'type' to 'type-a'. We have three non-clashing members called 'integer': 1. TestUnion branch value-a branch value-a1 common member 2. TestUnion branch value-a branch value-a2 common member 3. TestUnion branch value-b common member They all map to the same member on the wire. This is the positive test I asked for above. Good. Except it needs to go into tests/qapi-schema/qapi-schema-test.json, so we actually generate code and compile it. Let me explain. All the test schemas are fed to test-qapi.py, which tests the QAPI generator's frontend. qapi-schema-test.json and doc-good.json we additionally feed to qapi-gen.py. The code generated for the former gets compiled into a number of unit tests. To find them, use $ git-grep -l '#include.*test-qapi-' Would you mind extending them so the code is actually exercised? > diff --git a/tests/qapi-schema/union-union-branch.out b/tests/qapi-schema/union-union-branch.out > new file mode 100644 > index 0000000000..d0c37495c2 > --- /dev/null > +++ b/tests/qapi-schema/union-union-branch.out > @@ -0,0 +1,38 @@ > +module ./builtin > +object q_empty > +enum QType > + prefix QTYPE > + member none > + member qnull > + member qnum > + member qstring > + member qdict > + member qlist > + member qbool > +module union-union-branch.json > +enum TestEnum > + member value-a > + member value-b > +enum TestEnumA > + member value-a1 > + member value-a2 > +object TestTypeA1 > + member integer: int optional=False > +object TestTypeA2 > + member integer: int optional=False > +object q_obj_TestTypeA-base > + member type-a: TestEnumA optional=False > +object TestTypeA > + base q_obj_TestTypeA-base > + tag type-a > + case value-a1: TestTypeA1 > + case value-a2: TestTypeA2 > +object TestTypeB > + member integer: int optional=False > +object q_obj_TestUnion-base > + member type: TestEnum optional=False > +object TestUnion > + base q_obj_TestUnion-base > + tag type > + case value-a: TestTypeA > + case value-b: TestTypeB ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] qapi: allow unions to contain further unions 2023-02-23 7:24 ` Markus Armbruster @ 2023-02-23 13:40 ` Daniel P. Berrangé 0 siblings, 0 replies; 3+ messages in thread From: Daniel P. Berrangé @ 2023-02-23 13:40 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, Michael Roth, Eric Blake, Het Gala On Thu, Feb 23, 2023 at 08:24:33AM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > This extends the QAPI schema validation to permit unions inside unions, > > provided the checks for clashing fields pass. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > > > This patch comes out of the discussion on Het's migration series > > starting at this patch: > > > > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02111.html > > > > Markus had described his desired improved architecture > > > > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02719.html > > > > but I don't think I have enough knowledge of the QAPI code to attempt > > to fuse the handling of structs/unions as mentioned. This patch does > > what looks to be the bare minimum to permit unions in unions, while > > keeping validation checks for clashing fields. > > > > I've not tested beyond the unit tests, but if this is acceptable > > from Markus' POV, I'd expect Het to insert this patch at the > > start of his migration series and thus test it more fully. > > > > scripts/qapi/schema.py | 6 +-- > > .../union-invalid-union-subfield.err | 2 + > > .../union-invalid-union-subfield.json | 27 +++++++++++++ > > .../union-invalid-union-subfield.out | 0 > > .../union-invalid-union-subtype.err | 2 + > > .../union-invalid-union-subtype.json | 26 +++++++++++++ > > .../union-invalid-union-subtype.out | 0 > > tests/qapi-schema/union-union-branch.err | 0 > > tests/qapi-schema/union-union-branch.json | 26 +++++++++++++ > > tests/qapi-schema/union-union-branch.out | 38 +++++++++++++++++++ > > 10 files changed, 124 insertions(+), 3 deletions(-) > > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.err > > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.json > > create mode 100644 tests/qapi-schema/union-invalid-union-subfield.out > > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.err > > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.json > > create mode 100644 tests/qapi-schema/union-invalid-union-subtype.out > > create mode 100644 tests/qapi-schema/union-union-branch.err > > create mode 100644 tests/qapi-schema/union-union-branch.json > > create mode 100644 tests/qapi-schema/union-union-branch.out > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index cd8661125c..062c6bbb00 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > diff --git a/tests/qapi-schema/union-invalid-union-subfield.err b/tests/qapi-schema/union-invalid-union-subfield.err > > new file mode 100644 > > index 0000000000..d3a2e31aff > > --- /dev/null > > +++ b/tests/qapi-schema/union-invalid-union-subfield.err > > @@ -0,0 +1,2 @@ > > +union-invalid-union-subfield.json: In union 'TestUnion': > > +union-invalid-union-subfield.json:22: member 'teeth' of type 'TestTypeFish' collides with base member 'teeth' > > diff --git a/tests/qapi-schema/union-invalid-union-subfield.json b/tests/qapi-schema/union-invalid-union-subfield.json > > new file mode 100644 > > index 0000000000..235f76d7da > > --- /dev/null > > +++ b/tests/qapi-schema/union-invalid-union-subfield.json > > @@ -0,0 +1,27 @@ > > This is the negative test I asked for above. Good. > > We commonly start with a comment on what is being tested. Suggest > > # Clash between common member and union variant's variant member > # Base's member 'teeth' clashes with TestTypeFish's Yes, added. > > +{ 'union': 'TestUnion', > > + 'base': { 'type': 'TestEnum', > > + 'teeth': 'int' }, > > Indentation is off. Opps, a <tab> appeared. > > diff --git a/tests/qapi-schema/union-invalid-union-subtype.err b/tests/qapi-schema/union-invalid-union-subtype.err > > new file mode 100644 > > index 0000000000..7b8679c08f > > --- /dev/null > > +++ b/tests/qapi-schema/union-invalid-union-subtype.err > > @@ -0,0 +1,2 @@ > > +union-invalid-union-subtype.json: In union 'TestUnion': > > +union-invalid-union-subtype.json:22: base member 'type' collides with base member 'type' > > The error message is crap. See below. Agreed, and now fixed. > > diff --git a/tests/qapi-schema/union-invalid-union-subtype.json b/tests/qapi-schema/union-invalid-union-subtype.json > > new file mode 100644 > > index 0000000000..59ca4b0385 > > --- /dev/null > > +++ b/tests/qapi-schema/union-invalid-union-subtype.json > > @@ -0,0 +1,26 @@ > > Suggest > > # Clash between common member and union variant's common member > # Base's member 'type' clashes with TestTypeA's Yes, added. > > +{ 'union': 'TestUnion', > > + 'base': { 'type': 'TestEnum' }, > > + 'discriminator': 'type', > > + 'data': { 'value-a': 'TestTypeA', > > + 'value-b': 'TestTypeB' } } > > This new test is structurally similar to existing test > union-clash-member. Synopsis: > > { 'enum': 'TestEnum', { 'enum': 'TestEnum', > 'data': [ 'value-a', 'value-b' ] } 'data': [ 'value1', 'value2' ] } > > { 'struct': 'Base', > 'data': { 'enum1': 'TestEnum', > '*name': 'str' } } > > { 'enum': 'TestEnumA', > 'data': [ 'value-a1', 'value-a2' ] } > > { 'struct': 'TestTypeA1', > 'data': { 'integer': 'int' } } > > { 'struct': 'TestTypeA2', > 'data': { 'integer': 'int' } } > > { 'union': 'TestTypeA', { 'struct': 'Branch1', > 'base': { 'type': 'TestEnumA' }, 'data': { 'name': 'str' } } > 'discriminator': 'type', > 'data': { 'value-a1': 'TestTypeA1', > 'value-a2': 'TestTypeA2' } } > > { 'struct': 'TestTypeB', { 'struct': 'Branch2', > 'data': { 'integer': 'int' } } 'data': { 'value': 'int' } } > > { 'union': 'TestUnion', { 'union': 'TestUnion', > 'base': { 'type': 'TestEnum' }, 'base': 'Base', > 'discriminator': 'type', 'discriminator': 'enum1', > 'data': { 'value-a': 'TestTypeA', 'data': { 'value1': 'Branch1', > 'value-b': 'TestTypeB' } } 'value2': 'Branch2' } } > > Differences: > > * Names and scalar types, which shouldn't matter. > > * Base is inline vs. explicit type; shouldn't matter. But does matter - this is what results in the bad error messages mentioned in the next point. > * First branch is a union vs. a struct. Since the error is about a > common union member, this shouldn't matter, either. But it does: the > error message is much worse, namely > > base member 'type' collides with base member 'type' > > vs. > > member 'name' of type 'Branch1' collides with member 'name' of type 'Base' > > Something's off here. When 'describe' formats a name for the base member it is insufficiently specific. It had intentionally skipped added info about the type containing the base, because it (rightly) assumed there was only one applicable containing type in that context. THis assumption is not valid once we allow unions in unions, so we need to be explicit about the containing type. > > diff --git a/tests/qapi-schema/union-union-branch.json b/tests/qapi-schema/union-union-branch.json > > new file mode 100644 > > index 0000000000..d3d7ce57c6 > > --- /dev/null > > +++ b/tests/qapi-schema/union-union-branch.json > > @@ -0,0 +1,26 @@ snip > > +{ 'union': 'TestUnion', > > + 'base': { 'type': 'TestEnum' }, > > + 'discriminator': 'type', > > + 'data': { 'value-a': 'TestTypeA', > > + 'value-b': 'TestTypeB' } } > > This is union-invalid-union-subtype.json with the clash fixed by > renaming TestTypeA's member 'type' to 'type-a'. > > We have three non-clashing members called 'integer': > > 1. TestUnion branch value-a branch value-a1 common member > > 2. TestUnion branch value-a branch value-a2 common member > > 3. TestUnion branch value-b common member > > They all map to the same member on the wire. This is the positive test > I asked for above. Good. > > Except it needs to go into tests/qapi-schema/qapi-schema-test.json, so > we actually generate code and compile it. Let me explain. > > All the test schemas are fed to test-qapi.py, which tests the QAPI > generator's frontend. > > qapi-schema-test.json and doc-good.json we additionally feed to > qapi-gen.py. The code generated for the former gets compiled into a > number of unit tests. To find them, use > > $ git-grep -l '#include.*test-qapi-' > > Would you mind extending them so the code is actually exercised? Ok, yes, I missed that aspect. So 99% of the qapi-schema/*.json files are only for validating bad scenarios. The good scenarios should (nearly) all be in qapi-schema-test.json. I'm adding to test-qobject-{input,output}-visitor.c to validate this new union-in-union handling. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-23 13:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-14 18:44 [PATCH] qapi: allow unions to contain further unions Daniel P. Berrangé 2023-02-23 7:24 ` Markus Armbruster 2023-02-23 13:40 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).