Eric Blake <ebl...@redhat.com> 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. >> > >>> +++ 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. > >> >>> +++ 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? 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) and the TODO documents what we can do down >> the road (fix the generator to allow it). > > See also 32/47 - events have the same problem.
I'll amend the next patch to fix them, too.