[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 08/17] tests: Convert to new qapi union layou
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 08/17] tests: Convert to new qapi union layout |
Date: |
Thu, 22 Oct 2015 16:57:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 10/22/2015 08:01 AM, Markus Armbruster wrote:
>> 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.
>>>
>>> Make the conversion to the new layout for testsuite code.
>>>
>>> Note that this includes updating an error message regarding a
>>> collision. After the conversion to the new union qapi layout
>>> is complete, there will still be further patches for cleaning
>>> up collision detection, since the use of a named union can
>>> completely eliminate the collision wording changed here.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>> ---
>>> 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.py | 2 +-
>>> tests/qapi-schema/union-clash-type.err | 2 +-
>>> tests/qapi-schema/union-clash-type.json | 6 +--
>>> tests/test-qmp-commands.c | 4 +-
>>> tests/test-qmp-input-visitor.c | 78
>>> ++++++++++++++++-----------------
>>> tests/test-qmp-output-visitor.c | 42 +++++++++---------
>>> 6 files changed, 66 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 1e7e08b..aab2b46 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -546,7 +546,7 @@ def check_union(expr, expr_info):
>>> base = expr.get('base')
>>> discriminator = expr.get('discriminator')
>>> members = expr['data']
>>> - values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
>>> + values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
>>>
>>> # Two types of unions, determined by discriminator.
>>>
>>
>> Umm, does this really belong to this patch?
>
> Yes, because it cleans up the error message in union-clash-type.err, as
> mentioned in the commit message. But since I'm already splitting out the
> qapi-visit parts of 7/17, maybe it belongs better in that patch (all
> changes to the rest of qapi to deal with the qapi-type parts)?
Makes sense, I think.
>>> diff --git a/tests/qapi-schema/union-clash-type.err
>>> b/tests/qapi-schema/union-clash-type.err
>>> index a5dead1..c14bbdd 100644
>>> --- a/tests/qapi-schema/union-clash-type.err
>>> +++ b/tests/qapi-schema/union-clash-type.err
>>> @@ -1 +1 @@
>>> -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion'
>>> member 'kind' clashes with '(automatic)'
>>> +tests/qapi-schema/union-clash-type.json:6: Union 'TestUnion'
>>> member 'type' clashes with '(automatic tag)'
>
> At any rate, this is what improved by adjusting that line of qapi.py.
>
>>> diff --git a/tests/qapi-schema/union-clash-type.json
>>> b/tests/qapi-schema/union-clash-type.json
>>> index cfc256b..641b2d5 100644
>>> --- a/tests/qapi-schema/union-clash-type.json
>>> +++ b/tests/qapi-schema/union-clash-type.json
>>> @@ -1,9 +1,7 @@
>>> # Union branch 'type'
>>> # Reject this, because we would have a clash in generated C, between the
>>> -# simple union's implicit tag member 'kind' and the branch name 'kind'
>>> +# simple union's implicit tag member 'type' and the branch name 'type'
>>> # within the union.
>>> -# TODO: Even when the generated C is switched to use 'type' rather than
>>> -# 'kind', to match the QMP spelling, the collision should still be
>>> detected.
>>> -# Or, we could munge the branch name to allow compilation.
>>> +# TODO: If desired, we could munge the branch name to allow compilation.
>>
>> Let's mark it TODO only if we intend to revisit the idea of munging
>> branch names.
>
> I have a later patch queued that deletes this test altogether, so for
> v10 I'll probably just eliminate changes here for less churn.
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04111.html
>
>>
>>> { 'union': 'TestUnion',
>>> 'data': { 'kind': 'int', 'type': 'str' } }
>> [Mind-numbing mechanical switch to u. and from kind to type...]
>>
>
> Yep. I wish I knew coccinelle well enough to see if it could do the
> conversion for me, but I ended up doing it by hand (basically by
> applying 16/17 early, then seeing what failed to compile, fixing it up,
> then rebasing it into position).
I might be able to sledgehammer it into service here, but since you've
done the job already...
[Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 11/17] net: Convert to new qapi union layout, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members, Eric Blake, 2015/10/16