Re: [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

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    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]