On 07/09/2013 03:53 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" }
[prior to reading the patch body] I assume that the corresponding qapi-schema.json change that matches the above wire context is: { 'type': 'Base', 'data': { 'read-only': 'bool' } } { 'type': 'File', 'data': { 'filename': 'str' } } { 'union': 'Example', 'base': 'Base', 'discriminator': 'driver', 'data': { 'file': 'File' } } with the new 'discriminator' element stating that an alternate name (rather than the default 'type' name) is in use. Do you allow flattened unions always, or only when 'discriminator' was present? Aren't you really flattening TWO structs? That is, extending the above, we are going from: { 'command': 'test', 'data': { 'driver': 'Example' } } which is previously called as: { "execute": "test", "arguments": { "driver": { "type": "file", "read-only": true, "data": { "filename": "test" } } } } to the shorter: { "execute": "test", "arguments": { "driver": "file", "read-only": true, "filename": "test" } } [...now after reading the patch body] Oh - you ONLY support a discriminator IF you pass a 'base' class, where the discriminator has to be one of the fields named in the base class, necessarily of type 'str'. In other words, the qapi-schema.json needs to look more like: { 'type': 'Base', 'data': { 'read-only': 'bool', 'driver': 'str' } } { 'type': 'File', 'data': { 'filename': 'str' } } { 'union': 'Example', 'base': 'Base', 'discriminator': 'driver', 'data': { 'file': 'File' } } If I'm off (or even if I'm right), more details in the commit message about the qapi description that leads to the final QMP wire contents would be helpful. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > scripts/qapi-types.py | 10 +++--- > scripts/qapi-visit.py | 91 > ++++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 70 insertions(+), 31 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 960065b..db2f533 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -162,10 +162,8 @@ def generate_union(expr): > name = expr['union'] > typeinfo = expr['data'] > > - if expr.has_key('base'): > - base = expr['base'] > - else: > - base = None > + base = expr.get('base') Should this simplification be squashed into patch 2/11? (Patch 4/11 has a similar construct that might also be simplified.) > + discriminator = expr.get('discriminator') > > ret = mcgen(''' > struct %(name)s > @@ -189,7 +187,11 @@ struct %(name)s > > if base: > struct = find_struct(base) > + if discriminator: > + del struct['data'][discriminator] Here my lack of python expertise shows, in what may be a dumb question: Does find_struct(base) return a reference to the original object (oops - modifying 'struct' here means that the next call to find_struct(base) sees a smaller object), or a fully-cloned object (phew - our deletion is to our local copy, but future callers get a fresh clone that still see the discriminator)? Meanwhile, should you assert that the discriminator is of type 'str'? Actually, I suppose it might be nice to support a discriminator of an enum type (to document the set of valid strings); but the point still remains that the discriminator field is probably not going to work as a bool, integer, or struct type. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature