Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator

[Kevin Wolf <kwolf@redhat.com>]

Am 26.07.2013 um 15:40 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > Instead of the rather verbose syntax that distinguishes base and
> > subclass fields...
> > 
> >   { "type": "file",
> >     "read-only": true,
> >     "data": {
> >         "filename": "test"
> >     } }
> > 
> > ...we can now have both in the same namespace, allowing a more direct
> > mapping of the command line, and moving fields between the common base
> > and subclasses without breaking the API:
> > 
> >   { "driver": "file",
> >     "read-only": true,
> >     "filename": "test" }
> 
> MUCH nicer!
> 
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  docs/qapi-code-gen.txt | 22 +++++++++++++
> >  scripts/qapi-types.py  |  6 ++++
> >  scripts/qapi-visit.py  | 86 ++++++++++++++++++++++++++++++++++++--------------
> >  3 files changed, 91 insertions(+), 23 deletions(-)
> 
> > 
> > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> > index 555ca66..c187fda 100644
> > --- a/docs/qapi-code-gen.txt
> > +++ b/docs/qapi-code-gen.txt
> > @@ -103,6 +103,28 @@ And it looks like this on the wire:
> >     "data" : { "backing-file": "/some/place/my-image",
> >                "lazy-refcounts": true } }
> >  
> > +
> > +Flat union types avoid the nesting on the wire. They are used whenever a
> > +specific field of the base type is declared as the discriminator ('type' is
> > +then no longer generated). The discriminator must always be a string field.
> 
> Since an 'enum' is always sent over the wire as a string, is it
> appropriate to allow the discriminator to be an enum field instead of a
> 'str'?  But that could be done in a followup patch; your initial usage
> is just fine with 'str'.  Besides, if we allow the discriminator to have
> 'enum' type, that would imply that we want to guarantee coverage that
> all of the 'data' members of the union type correspond to the members of
> the union.  On the other hand, that would be extra type safety - if we
> extend the enum that backs the discriminator, we'd immediately be
> reminded if we forgot to also extend the union based on that enum.
> Again, food for thought for a future patch, and not something needed for
> this one.

There's nothing to add to this, I would certainly support such a future
patch.

> > +The above example can then be modified as follows:
> > +
> > + { 'type': 'BlockdevCommonOptions',
> > +   'data': { 'driver': 'str' 'readonly': 'bool' } }
> 
> Missing a comma.
> 
> > +++ b/scripts/qapi-types.py
> > @@ -161,7 +161,9 @@ def generate_union(expr):
> >  
> >      name = expr['union']
> >      typeinfo = expr['data']
> > +
> >      base = expr.get('base')
> > +    discriminator = expr.get('discriminator')
> >  
> >      ret = mcgen('''
> >  struct %(name)s
> > @@ -185,7 +187,11 @@ struct %(name)s
> >  
> >      if base:
> >          struct = find_struct(base)
> > +        if discriminator:
> > +            del struct['data'][discriminator]
> 
> I asked before, but didn't get an answer; my question may just show my
> unfamiliarity with python.  Is this modifying the original 'struct',
> such that other uses of the struct after this point will no longer
> contain the discriminator key?  Or is it only modifying a copy of
> 'struct', with the original left intact?  But based on the rest of your
> patch...

Sorry, this is in fact my own unfamiliarity with Python, combined with
failure to fix all cases when you pointed it out. The only reason I
didn't reply to that part of your review was that I thought it would be
obvious when I send a fixed version. Well, except if I don't.

I've changed this hunk now to match the other one:

@@ -184,8 +186,13 @@ struct %(name)s
 ''')
 
     if base:
-        struct = find_struct(base)
-        ret += generate_struct_fields(struct['data'])
+        base_fields = find_struct(base)['data']
+        if discriminator:
+            base_fields = base_fields.copy()
+            del base_fields[discriminator]
+        ret += generate_struct_fields(base_fields)
+    else:
+        assert not discriminator
 
     ret += mcgen('''
 };


> I think my findings are easy fixes; so I'm okay if you fix them and then
> add:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, Eric.

Kevin