| Message ID | 1445576998-2921-14-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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 <eblake@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.
On 10/26/2015 11:07 AM, Markus Armbruster wrote: > Eric Blake <eblake@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. > It _was_ a part of the previous patch in v9 :) [...] >> +++ 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. Okay, that works for me. Reserve additional namespace in 12, then drop the reservation in the later patch (now in subset C, see my comments to 24/25)... > >> 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. ...and see if I can avoid churn in union-clash-type.json up until the point in subset C where it disappears. I'll see how it plays out. Expect v11 soon.
Eric Blake <eblake@redhat.com> writes: > On 10/26/2015 11:07 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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. >> > > It _was_ a part of the previous patch in v9 :) Actually, it was in "[PATCH v9 08/17] tests: Convert to new qapi union layout" (i.e. the next patch), not in "[PATCH v9 07/17] qapi: Start converting to new qapi union layout" (the previous patch). > [...] > >>> +++ 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. > > Okay, that works for me. Reserve additional namespace in 12, then drop > the reservation in the later patch (now in subset C, see my comments to > 24/25)... > >> >>> 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. > > ...and see if I can avoid churn in union-clash-type.json up until the > point in subset C where it disappears. I'll see how it plays out. > Expect v11 soon. Thanks!
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. 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)'
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. Also make a change in qapi.py to quit using the name 'kind', and the corresponding change in the testsuite. 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 <eblake@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(-)