[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, a
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments |
Date: |
Mon, 27 Jul 2015 09:50:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 07/21/2015 06:43 AM, Eric Blake wrote:
>> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>>> A command's 'data' must be a struct type, given either as a
>>> dictionary, or as struct type name.
>>>
>>> Existing test case data-int.json covers simple type 'int'. Add test
>>> cases for type names referring to union and alternate types.
>>
>> We could probably relax things to allow a union (which is always a
>> dictionary on the wire), but I agree that allowing an alternate type is
>> not appropriate (the goal here is that we require a dictionary). But
>> it's also easier to be conservative now and relax later.
Yes.
>>> +++ b/tests/qapi-schema/args-alternate.json
>>> @@ -0,0 +1,4 @@
>>> +# we do not allow alternate arguments
>>> +# TODO should we support this?
>>
>> I see no reason to allow a non-dictionary in QMP, so this TODO could be
>> dropped.
>
> Or, to be clear, we document that arguments is always a dictionary, for:
> { "execute":"command", "arguments":{} }. Allowing an alternate would
> break that, so it is a different level of change to allow an alternate
> (change the QMP protocol) than what it would be to allow a union (the
> QMP protocol is unchanged). Not that we can't do it if we ever have a
> reason, it's just that I don't see it being worth a TODO statement.
I'll drop it.
>>> +++ b/tests/qapi-schema/args-union.json
>>> @@ -0,0 +1,4 @@
>>> +# FIXME we should reject union arguments
>>> +# TODO should we support this?
>>> +{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
>>> +{ 'command': 'oops', 'data': 'Uni' }
>>
>> This, on the other hand, seems valid from the wire format (it will
>> always be a dictionary). I guess the problem is that we generate a C
>> function signature based by calling out each member of the dictionary -
>> but how do you do that for a union?
Exactly: the problem is neither conceptual nor the wire API, it's the C
API we generate.
>> So I see what you are doing:
>> marking that this test currently passes the parser, but then causes
>> problems for generating C code, so we should either reject it up front,
>> or fix the generator. The FIXME documents what you will do later in the
>> series (reject it up front)
Yes, in PATCH 15.
>> and the TODO documents what we can do down
>> the road (fix the generator to allow it).
I figure we'd change the C API not to explode the data type into
multiple parameters. We can consider that when we have a use for it.
> See also 32/47 - events have the same problem.
I'm afraid I don't see the connection to PATCH 32.
>> Reviewed-by: Eric Blake <address@hidden>
Thanks!
- Re: [Qemu-devel] [PATCH RFC v2 11/47] tests/qapi-schema: Document alternate's enum lacks visit function, (continued)
[Qemu-devel] [PATCH RFC v2 25/47] qapi: Make generators work on sorted schema expressions, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 15/47] qapi: Fix to reject union command arguments, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 20/47] qapi: Rename class QAPISchema to QAPISchemaParser, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 18/47] qapi-commands: Don't feed output of mcgen() to mcgen() again, Markus Armbruster, 2015/07/01