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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> 
>> ---