From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJdBG-0001oC-Nf for qemu-devel@nongnu.org; Mon, 27 Jul 2015 03:50:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJdBE-00027w-0V for qemu-devel@nongnu.org; Mon, 27 Jul 2015 03:50:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41986) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJdBD-00027D-NE for qemu-devel@nongnu.org; Mon, 27 Jul 2015 03:50:35 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-15-git-send-email-armbru@redhat.com> <55AE3E6D.4000008@redhat.com> <55B1013F.1030704@redhat.com> Date: Mon, 27 Jul 2015 09:50:32 +0200 In-Reply-To: <55B1013F.1030704@redhat.com> (Eric Blake's message of "Thu, 23 Jul 2015 08:59:11 -0600") Message-ID: <87fv4at4xj.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake 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 Thanks!