Re: [PATCH] qapi: allow unions to contain further unions

[Daniel P . Berrangé]

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