[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discrimi
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator |
Date: |
Fri, 26 Jul 2013 17:01:46 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> > ---
> > 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 <address@hidden>
Thanks, Eric.
Kevin
- [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions, (continued)
- [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH v2 07/17] qapi: Flat unions with arbitrary discriminator, Kevin Wolf, 2013/07/26
- [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object(), Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions, Kevin Wolf, 2013/07/23
- [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level, Kevin Wolf, 2013/07/23