| Message ID | 1445576998-2921-23-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. "a non-variant member name" Could use an example. Pointer to flat-union-clash-branch.json would do. > This patch is the back end for a series that converts to a > saner qapi union layout. Now that all clients have been > converted to use 'type' and 'obj->u.value', we can drop the > temporary parallel support for 'kind' and 'obj->value'. > > Given a simple union qapi type: > > { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } } > > this is the overall effect, when compared to the state before > this series of patches: > > | struct Foo { > |- FooKind kind; > |- union { /* union tag is @kind */ > |+ FooKind type; > |+ union { /* union tag is @type */ > | void *data; > | int64_t a; > | bool b; > |- }; > |+ } u; > | }; > > Note, however, that we do not rename the generated enum, which > is still 'FooKind'. A further patch could generate implicit > enums as 'FooType', but while the generator already reserved > the '*Kind' namespace (commit 4dc2e69), there are already QMP > constructs with '*Type' naming, which means changing our > reservation namespace would have lots of churn to C code to > deal with a forced name change. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: rebase to earlier changes, match commit wording > 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-types.py | 26 +++++--------------------- > 1 file changed, 5 insertions(+), 21 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 579532d..d131430 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -148,23 +148,10 @@ struct %(c_name)s { > if base: > ret += gen_struct_fields([], base) > else: > - # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we > - # want to use only 'type', but the conversion is large enough to > - # require staging over several commits. > - ret += mcgen(''' > - union { > - %(c_type)s kind; > - %(c_type)s type; > - }; > -''', > - c_type=c_name(variants.tag_member.type.name)) > + ret += gen_struct_field(variants.tag_member.name, > + variants.tag_member.type, > + False) > > - # TODO As a hack, we emit the union twice, once as an anonymous union > - # and once as a named union. Ultimately, we want to use only the > - # named union version (as it avoids conflicts between tag values as > - # branch names competing with non-variant QMP names), but the conversion > - # is large enough to require staging over several commits. > - tmp = '' > # FIXME: What purpose does data serve, besides preventing a union that > # has a branch named 'data'? We use it in qapi-visit.py to decide > # whether to bypass the switch statement if visiting the discriminator > @@ -173,7 +160,7 @@ struct %(c_name)s { > # should not be any data leaks even without a data pointer. Or, if > # 'data' is merely added to guarantee we don't have an empty union, > # shouldn't we enforce that at .json parse time? > - tmp += mcgen(''' > + ret += mcgen(''' > union { /* union tag is @%(c_name)s */ > void *data; > ''', > @@ -182,17 +169,14 @@ struct %(c_name)s { > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > typ = var.simple_union_type() or var.type > - tmp += mcgen(''' > + ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > c_type=typ.c_type(), > c_name=c_name(var.name)) > > - ret += tmp > - ret += ' ' + '\n '.join(tmp.split('\n')) > ret += mcgen(''' > } u; > - }; > }; > ''') Tag values can no longer clash with non-variant member names, but we still have a bunch of restrictions stemming from that clash, notably the one you updated in PATCH 12 (outlaw 'TYPE' in addition to 'KIND'). You previously cleaned them up in "[PATCH v10 24/25] qapi: Remove outdated tests related to QMP/branch collisions", but that one's now delayed until later. I haven't tried to understand why, because I think delaying it a bit is okay, but I'd like to have it noted in the commit message. If you supply a suitably updated one, I'll touch it up in my tree.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 579532d..d131430 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -148,23 +148,10 @@ struct %(c_name)s { if base: ret += gen_struct_fields([], base) else: - # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we - # want to use only 'type', but the conversion is large enough to - # require staging over several commits. - ret += mcgen(''' - union { - %(c_type)s kind; - %(c_type)s type; - }; -''', - c_type=c_name(variants.tag_member.type.name)) + ret += gen_struct_field(variants.tag_member.name, + variants.tag_member.type, + False) - # TODO As a hack, we emit the union twice, once as an anonymous union - # and once as a named union. Ultimately, we want to use only the - # named union version (as it avoids conflicts between tag values as - # branch names competing with non-variant QMP names), but the conversion - # is large enough to require staging over several commits. - tmp = '' # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -173,7 +160,7 @@ struct %(c_name)s { # should not be any data leaks even without a data pointer. Or, if # 'data' is merely added to guarantee we don't have an empty union, # shouldn't we enforce that at .json parse time? - tmp += mcgen(''' + ret += mcgen(''' union { /* union tag is @%(c_name)s */ void *data; ''', @@ -182,17 +169,14 @@ struct %(c_name)s { for var in variants.variants: # Ugly special case for simple union TODO get rid of it typ = var.simple_union_type() or var.type - tmp += mcgen(''' + ret += mcgen(''' %(c_type)s %(c_name)s; ''', c_type=typ.c_type(), c_name=c_name(var.name)) - ret += tmp - ret += ' ' + '\n '.join(tmp.split('\n')) ret += mcgen(''' } u; - }; }; ''')
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. This patch is the back end for a series that converts to a saner qapi union layout. Now that all clients have been converted to use 'type' and 'obj->u.value', we can drop the temporary parallel support for 'kind' and 'obj->value'. Given a simple union qapi type: { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } } this is the overall effect, when compared to the state before this series of patches: | struct Foo { |- FooKind kind; |- union { /* union tag is @kind */ |+ FooKind type; |+ union { /* union tag is @type */ | void *data; | int64_t a; | bool b; |- }; |+ } u; | }; Note, however, that we do not rename the generated enum, which is still 'FooKind'. A further patch could generate implicit enums as 'FooType', but while the generator already reserved the '*Kind' namespace (commit 4dc2e69), there are already QMP constructs with '*Type' naming, which means changing our reservation namespace would have lots of churn to C code to deal with a forced name change. Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: rebase to earlier changes, match commit wording 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-types.py | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)