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

[Eric Blake]

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/

> 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
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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature