Re: [Qemu-devel] [PATCH v5 07/10] qmp: add rocker device support

[Scott Feldman <sfeldma@gmail.com>]

On Tue, Feb 3, 2015 at 7:10 AM, Eric Blake <eblake@redhat.com> wrote:
> On 01/22/2015 01:03 AM, sfeldma@gmail.com wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Add QMP/HMP support for rocker devices.  This is mostly for debugging purposes
>> to see inside the device's tables and port configurations.  Some examples:
>>
>
> QMP interface review:
>
>> +++ b/qapi-schema.json
>> @@ -3523,3 +3523,6 @@
>>  # Since: 2.1
>>  ##
>>  { 'command': 'rtc-reset-reinjection' }
>> +
>> +# Rocker ethernet network switch
>> +{ 'include': 'qapi/rocker.json' }
>> diff --git a/qapi/rocker.json b/qapi/rocker.json
>> new file mode 100644
>> index 0000000..326c6c7
>> --- /dev/null
>> +++ b/qapi/rocker.json
>> @@ -0,0 +1,259 @@
>> +##
>> +# @Rocker:
>> +#
>> +# Rocker switch information.
>> +#
>> +# @name: switch name
>> +#
>> +# @id: switch ID
>> +#
>> +# @ports: number of front-panel ports
>> +##
>
> Missing a 'Since: 2.3' designation.

fixed all of these

>
>> +{ 'type': 'RockerSwitch',
>> +  'data': { 'name': 'str', 'id': 'uint64', 'ports': 'uint32' } }
>> +
>> +##
>> +# @rocker:
>> +#
>> +# Return rocker switch information.
>> +#
>> +# Returns: @Rocker information
>> +#
>> +# Since: 2.3
>> +##
>> +{ 'command': 'rocker',
>> +  'data': { 'name': 'str' },
>> +  'returns': 'RockerSwitch' }
>
> Should this command be named 'query-rocker', as it is used for queries?

added "query-" to all cmds.

>  Should the 'name' argument be optional, and the output be an array (all
> rocker devices, rather than just a given rocker name lookup)?

we can add a query-rockers cmds later to get list of rocker names (or
list of RockerSwitch'es).

>
> Missing '#optional' tags on the various optional fields.  Why are
> certain fields optional?  Does it mean they have a default value, or
> that they don't make sense in some configurations?  The docs could be
> more clear on that.

Added #optional for the optional fields and added comment for each
section that these are fields that may or may not be present depending
on context of object being returned.