From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aifUd-00031g-4j for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:54:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aifUb-00070C-UO for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:54:23 -0400 From: Markus Armbruster References: <1458271654-23706-1-git-send-email-peterx@redhat.com> <1458271654-23706-3-git-send-email-peterx@redhat.com> <56F19222.2050003@redhat.com> <20160323040722.GF28183@pxdev.xzpeter.org> Date: Wed, 23 Mar 2016 10:54:16 +0100 In-Reply-To: <20160323040722.GF28183@pxdev.xzpeter.org> (Peter Xu's message of "Wed, 23 Mar 2016 12:07:22 +0800") Message-ID: <878u19bjuf.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: wei@redhat.com, peter.maydell@linaro.org, drjones@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-arm@nongnu.org, abologna@redhat.com Peter Xu writes: > On Tue, Mar 22, 2016 at 12:42:42PM -0600, Eric Blake wrote: >> On 03/17/2016 09:27 PM, Peter Xu wrote: >> > This patch adds the command "query-gic-capabilities" but not implemnet >> >> s/not implemnet/does not implement/ > > Yep, again. Thanks. > >> >> > it. The command is ARM-only. Return of the command is a list of >> > GICCapability struct that describes all GIC versions that current QEMU >> > and system support. >> > >> > Signed-off-by: Peter Xu >> > --- >> >> > +++ b/qapi-schema.json >> > @@ -4156,3 +4156,14 @@ >> > 'data': { 'version': 'int', >> > 'emulated': 'bool', >> > 'kernel': 'bool' } } >> > + >> > +## >> > +# @query-gic-capabilities: >> > +# >> > +# Return a list of supported GIC version capabilities. >> > +# >> > +# Returns: a list of GICCapability. >> > +# >> > +# Since: 2.6 >> > +## >> > +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] } >> >> On the surface, this seems okay. As mentioned before, I would have >> squashed 1 and 2 into a single patch. The GICCapability type is >> extensible, and introspection is sufficient at seeing what the type is >> currently capable of exposing. >> >> On the other hand... >> >> > +++ b/scripts/qapi.py >> > @@ -46,6 +46,7 @@ returns_whitelist = [ >> > 'query-tpm-models', >> > 'query-tpm-types', >> > 'ringbuf-read', >> > + 'query-gic-capability', >> >> ...it required a whitelist, because you are violating the usual It doesn't :) >> convention of returning a dict. If you DO need the whitelist, your >> addition should have been kept sorted. But you don't need it, if you >> would modify your QAPI to return a dict: >> >> { 'struct': 'GICCapabilitiesReturn', >> 'data': { 'capabilities': ['GICCapability'] } } >> { 'command': 'query-gic-capabilities', >> 'returns': 'GICCapabilitiesReturn' } >> >> Yes, the dict has only a single key, and that key points to the same >> list; but now you have future extensibility: in the future, we could >> return any future global data as a sibling to the array, without having >> to modify every element of the array to repeat redundant information. > > Yes, I think this is better solution. Will adopt this in next version. As explained in my other message, do this only if query-gic-capabilities truly needs it. There's plenty of precedence for returning a list of a structured type. > Thanks for the comments! > > -- peterx