Eric Blake <ebl...@redhat.com> writes: > We have two issues with our qapi union layout: > 1) Even though the QMP wire format spells the tag 'type', the > C code spells it 'kind', requiring some hacks in the generator. > 2) The C struct uses an anonymous union, which places all tag > values in the same namespace as all non-variant members. This > leads to spurious collisions if a tag value matches a QMP name. > > Make the conversion to the new layout for qapi-visit.py.
This part is nicely mechanical: ->kind becomes ->type, ->variant becomes ->u.variant, and variants.tag_name or 'kind' becomes variants.tag_member.name. > Also > make a change in qapi.py to quit using the name 'kind', and > the corresponding change in the testsuite. This isn't really part of "qapi-visit: Convert to new qapi union layout". Could be made a separate patch, but I'd simply squash it into the previous one "qapi: Start converting to new qapi union layout". See also my remark inline. > However, many more > changes will be made to the testsuite later, including the > entire deletion of union-clash-type.json, after the completion > of the conversion eliminates collisions between union tag > branch names and non-variant names. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v10: new patch, split from 7/17 an 8/17 > v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC: > http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html > --- > scripts/qapi-visit.py | 24 +++++++++--------------- > scripts/qapi.py | 2 +- > tests/qapi-schema/union-clash-type.err | 2 +- > 3 files changed, 11 insertions(+), 17 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 3c23c53..421fcb6 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -187,18 +187,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s > **obj, const char *name, Error > if (err) { > goto out; > } > - visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, > &err); > + visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, > &err); > if (err) { > goto out_obj; > } > - switch ((*obj)->kind) { > + switch ((*obj)->type) { > ''', > c_name=c_name(name)) > > for var in variants.variants: > ret += mcgen(''' > case %(case)s: > - visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err); > + visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err); > break; > ''', > case=c_enum_const(variants.tag_member.type.name, > @@ -259,22 +259,16 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s > **obj, const char *name, Error > visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); > ''', > c_type=variants.tag_member.type.c_name(), > - # TODO ugly special case for simple union > - # Use same tag name in C as on the wire to get rid of > - # it, then: c_name=c_name(variants.tag_member.name) > - c_name='kind', > + c_name=c_name(variants.tag_member.name), > name=variants.tag_member.name) > ret += gen_err_check(label='out_obj') > ret += mcgen(''' > - if (!visit_start_union(v, !!(*obj)->data, &err) || err) { > + if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { > goto out_obj; > } > switch ((*obj)->%(c_name)s) { > ''', > - # TODO ugly special case for simple union > - # Use same tag name in C as on the wire to get rid of > - # it, then: c_name=c_name(variants.tag_member.name) > - c_name=c_name(variants.tag_name or 'kind')) > + c_name=c_name(variants.tag_member.name)) > > for var in variants.variants: > # TODO ugly special case for simple union > @@ -286,13 +280,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s > **obj, const char *name, Error > var.name)) > if simple_union_type: > ret += mcgen(''' > - visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err); > + visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err); > ''', > c_type=simple_union_type.c_name(), > c_name=c_name(var.name)) > else: > ret += mcgen(''' > - visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err); > + visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err); > ''', > c_type=var.type.c_name(), > c_name=c_name(var.name)) > @@ -308,7 +302,7 @@ out_obj: > error_propagate(errp, err); > err = NULL; > if (*obj) { > - visit_end_union(v, !!(*obj)->data, &err); > + visit_end_union(v, !!(*obj)->u.data, &err); > } > error_propagate(errp, err); > err = NULL; > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 04bcbf7..8b9f5f7 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -548,7 +548,7 @@ def check_union(expr, expr_info): > base = expr.get('base') > discriminator = expr.get('discriminator') > members = expr['data'] > - values = {'MAX': '(automatic)', 'KIND': '(automatic)'} > + values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'} > > # Two types of unions, determined by discriminator. > Took me a minute to remember what's going on here. A simple union's tag values, become an enum type. Here, we catch two differend kinds of clashes: 1. Tag value with the implicitly defined enum member _MAX, i.e. a clash in the enum member name space 2. Member of the (as of yet still) unnamed union holding the variants with the tag variable type, formerly kind (i.e. a clash in the struct member name space). The previous patch added type to the struct member name space without updating clash detection. A future patch will remove the unnamed union, thus clash kind 2. It'll also remove kind from the struct member name space, but that doesn't matter. I believe the following would be slightly cleaner: amend the previous patch to *add* key 'TYPE' to values. Drop both 'KIND' and 'TYPE' when you remove the unnamed union. > diff --git a/tests/qapi-schema/union-clash-type.err > b/tests/qapi-schema/union-clash-type.err > index a5dead1..eca7c1d 100644 > --- a/tests/qapi-schema/union-clash-type.err > +++ b/tests/qapi-schema/union-clash-type.err > @@ -1 +1 @@ > -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' > clashes with '(automatic)' > +tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'type' > clashes with '(automatic tag)' You might be able to avoid this change by adding 'TYPE' in the right spot.