| Message ID | 1374584606-5615-8-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > +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... > ret += generate_struct_fields(struct['data']) > + else: > + assert not discriminator > > ret += mcgen(''' > }; > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index db6fa44..cd33e44 100644 > --- a/scripts/qapi-visit.py > @@ -157,9 +178,17 @@ def generate_visit_union(expr): > members = expr['data'] > > base = expr.get('base') > + discriminator = expr.get('discriminator') > > ret = generate_visit_enum('%sKind' % name, members.keys()) > > + if base: > + base_fields = find_struct(base)['data'] > + if discriminator: > + base_fields = base_fields.copy() > + del base_fields[discriminator] ...here, you explicitly took a copy to be safe. So I suspect you are missing a .copy() above. 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>
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
On 07/26/2013 09:01 AM, Kevin Wolf wrote: >>> 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. Ah, so this is a case of the blind leading the blind... > > 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 Yes, that makes more sense. >> 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> Which means this is indeed a valid review.
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. +The above example can then be modified as follows: + + { 'type': 'BlockdevCommonOptions', + 'data': { 'driver': 'str' 'readonly': 'bool' } } + { 'union': 'BlockdevOptions', + 'base': 'BlockdevCommonOptions', + 'discriminator': 'driver', + 'data': { 'raw': 'RawOptions', + 'qcow2': 'Qcow2Options' } } + +Resulting in this JSON object: + + { "driver": "qcow2", + "readonly": false, + "backing-file": "/some/place/my-image", + "lazy-refcounts": true } + + === Commands === Commands are defined by using a list containing three members. The first diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 9882b95..db2f533 100644 --- a/scripts/qapi-types.py +++ 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] ret += generate_struct_fields(struct['data']) + else: + assert not discriminator ret += mcgen(''' }; diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index db6fa44..cd33e44 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -17,8 +17,26 @@ import os import getopt import errno -def generate_visit_struct_fields(field_prefix, members): +def generate_visit_struct_fields(name, field_prefix, members): + substructs = [] ret = '' + full_name = name if not field_prefix else "%s_%s" % (name, field_prefix) + + for argname, argentry, optional, structured in parse_args(members): + if structured: + ret += generate_visit_struct_fields(name, field_prefix + argname, argentry) + + ret += mcgen(''' + +static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp) +{ + Error *err = NULL; +''', + name=name, full_name=full_name) + push_indent() + + if len(field_prefix): + field_prefix = field_prefix + "." for argname, argentry, optional, structured in parse_args(members): if optional: @@ -31,7 +49,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) { push_indent() if structured: - ret += generate_visit_struct_body(field_prefix + argname, argname, argentry) + ret += generate_visit_struct_body(full_name, argname, argentry) else: ret += mcgen(''' visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err); @@ -47,6 +65,12 @@ visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", visit_end_optional(m, &err); ''') + pop_indent() + ret += mcgen(''' + + error_propagate(errp, err); +} +''') return ret @@ -56,8 +80,9 @@ if (!error_is_set(errp)) { ''') push_indent() + full_name = name if not field_prefix else "%s_%s" % (field_prefix, name) + if len(field_prefix): - field_prefix = field_prefix + "." ret += mcgen(''' Error **errp = &err; /* from outer scope */ Error *err = NULL; @@ -74,20 +99,14 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); ret += mcgen(''' if (!err) { if (!obj || *obj) { -''') - push_indent() - push_indent() - - ret += generate_visit_struct_fields(field_prefix, members) - pop_indent() - ret += mcgen(''' - error_propagate(errp, err); - err = NULL; -} -''') + visit_type_%(name)s_fields(m, obj, &err); + error_propagate(errp, err); + err = NULL; + } +''', + name=full_name) pop_indent() - pop_indent() ret += mcgen(''' /* Always call end_struct if start_struct succeeded. */ visit_end_struct(m, &err); @@ -98,7 +117,9 @@ if (!err) { return ret def generate_visit_struct(name, members): - ret = mcgen(''' + ret = generate_visit_struct_fields(name, "", members) + + ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) { @@ -157,9 +178,17 @@ def generate_visit_union(expr): members = expr['data'] base = expr.get('base') + discriminator = expr.get('discriminator') ret = generate_visit_enum('%sKind' % name, members.keys()) + if base: + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator] + ret += generate_visit_struct_fields(name, "", base_fields) + ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) @@ -179,23 +208,34 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** push_indent() if base: - struct = find_struct(base) - push_indent() - ret += generate_visit_struct_fields("", struct['data']) - pop_indent() + ret += mcgen(''' + visit_type_%(name)s_fields(m, obj, &err); +''', + name=name) pop_indent() ret += mcgen(''' - visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err); + visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err); if (!err) { switch ((*obj)->kind) { ''', - name=name) + name=name, type="type" if not discriminator else discriminator) for key in members: + if not discriminator: + fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);' + else: + fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err); + if (!err) { + visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err); + error_propagate(errp, err); + err = NULL; + visit_end_implicit_struct(m, &err); + }''' + ret += mcgen(''' case %(abbrev)s_KIND_%(enum)s: - visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err); + ''' + fmt + ''' break; ''', abbrev = de_camel_case(name).upper(),
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" } 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(-)