[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi u
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi union layout |
Date: |
Tue, 27 Oct 2015 09:33:48 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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.
- [Qemu-devel] [PATCH v10 14/25] tests: Convert to new qapi union layout, (continued)
- [Qemu-devel] [PATCH v10 14/25] tests: Convert to new qapi union layout, Eric Blake, 2015/10/23
- [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Eric Blake, 2015/10/23
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Markus Armbruster, 2015/10/23
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Eric Blake, 2015/10/23
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Markus Armbruster, 2015/10/26
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Eric Blake, 2015/10/26
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Markus Armbruster, 2015/10/26
- Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes, Eric Blake, 2015/10/26
[Qemu-devel] [PATCH v10 20/25] memory: Convert to new qapi union layout, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi union layout, Eric Blake, 2015/10/23
- Re: [Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi union layout,
Markus Armbruster <=
[Qemu-devel] [PATCH v10 18/25] char: Convert to new qapi union layout, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 23/25] qapi: Reserve 'u' member name, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 11/25] qapi-visit: Remove redundant functions for flat union base, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 16/25] sockets: Convert to new qapi union layout, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 19/25] input: Convert to new qapi union layout, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 24/25] qapi: Remove outdated tests related to QMP/branch collisions, Eric Blake, 2015/10/23
Re: [Qemu-devel] [PATCH v10 00/25] qapi collision reduction (post-introspection subset B'), Markus Armbruster, 2015/10/26