Re: [Qemu-arm] [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Peter Xu <address@hidden> 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 <address@hidden>
>> > ---
>> 
>> > +++ 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