| Message ID | 1445898903-12082-23-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/26/2015 04:35 PM, Eric Blake wrote: > 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. You asked for an updated commit message (but that request was made against v10: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06216.html). There, you suggested "a non-variant member name" rather than "QMP name" - it works for me, but you'd want to make that change for all of 13-22/25 (since I copy-pasted the same intro to each). Or decide which ones you want to squash together. For your other comment in that mail (mentioning an example test), I think I already got close to what you asked for, but have one tweak below: > > 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; > | }; > > There are still some further changes that can be made to the > testsuite now that there is no longer a collision between > enum tag values and QMP names, as well as adding a reservation > of 'u' to avoid a collision between QMP and our choice of union > naming, but those will be later patches. Change this paragraph to something along these lines: The testsuite still contains some examples of artificial restrictions (see flat-union-clash-type.json, for example) that are no longer technically necessary, now that there is no longer a collision between enum tag values and non-variant member names; but fixing this will be done in later patches, in part because some further changes are required to keep QAPISchema*.check() from asserting. Also, a later patch will add a reservation for the member name 'u' to avoid a collision between a user's non-variant names and our internal choice of C union name. > > 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> > > ---
Eric Blake <eblake@redhat.com> writes: > On 10/26/2015 04:35 PM, Eric Blake wrote: >> 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. > > You asked for an updated commit message (but that request was made > against v10: > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06216.html). > > There, you suggested "a non-variant member name" rather than "QMP name" > - it works for me, but you'd want to make that change for all of > 13-22/25 (since I copy-pasted the same intro to each). Or decide which > ones you want to squash together. I went through all commit messages and tweaked around occurences of QMP. I pushed the result to qapi-next; please have a look. > For your other comment in that mail (mentioning an example test), I > think I already got close to what you asked for, but have one tweak below: > >> >> 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; >> | }; >> >> There are still some further changes that can be made to the >> testsuite now that there is no longer a collision between >> enum tag values and QMP names, as well as adding a reservation >> of 'u' to avoid a collision between QMP and our choice of union >> naming, but those will be later patches. > > Change this paragraph to something along these lines: > > The testsuite still contains some examples of artificial restrictions > (see flat-union-clash-type.json, for example) that are no longer > technically necessary, now that there is no longer a collision between > enum tag values and non-variant member names; but fixing this will be > done in later patches, in part because some further changes are required > to keep QAPISchema*.check() from asserting. Also, a later patch will > add a reservation for the member name 'u' to avoid a collision between a > user's non-variant names and our internal choice of C union name. Done. >> >> 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> >> >> ---
On 10/27/2015 10:01 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > I went through all commit messages and tweaked around occurences of QMP. > I pushed the result to qapi-next; please have a look. Commit 2858a50 (Prefer typesafe upcasts...) on that branch has too much text (you pasted in corrected text, but forgot to delete the old). Otherwise looks good. Thanks for the touchups. Now on to subset C :)
Eric Blake <eblake@redhat.com> writes: > On 10/27/2015 10:01 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: > >> I went through all commit messages and tweaked around occurences of QMP. >> I pushed the result to qapi-next; please have a look. > > Commit 2858a50 (Prefer typesafe upcasts...) on that branch has too much > text (you pasted in corrected text, but forgot to delete the old). > Otherwise looks good. Thanks for the touchups. Now on to subset C :) Fixed up & pushed.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 1420e00..7e35bb6 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -149,23 +149,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 @@ -174,7 +161,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; ''', @@ -183,17 +170,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; | }; There are still some further changes that can be made to the testsuite now that there is no longer a collision between enum tag values and QMP names, as well as adding a reservation of 'u' to avoid a collision between QMP and our choice of union naming, but those will be later patches. 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> --- v11: enhance commit message 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(-)