Re: [Qemu-devel] [PATCH v3 5/7] qmp: add set-bootindex command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On 07/25/2014 10:45 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> 
> Example QMP command:
> -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
> <- { "return": {} }
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Chenliang <chenliang88@huawei.com>
> ---
>  qapi-schema.json | 16 ++++++++++++++++
>  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>  qmp.c            | 17 +++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b11aad2..a9ef0be 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1704,6 +1704,22 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @set-bootindex:
> +#
> +# set bootindex of a devcie

s/devcie/device/

Just to make sure this command is not write-only, it would be nice to
mention which query-* command can be used to learn a device's current
bootindex.

> +#
> +# @id: the name of the device
> +# @bootindex: the bootindex of the device
> +# @suffix: #optional a suffix of the device
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid device, DeviceNotFound
> +#
> +# Since: 2.2
> +##
> +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex': 'int', '*suffix': 'str'} }

Long line; wrap it to stay in 80 columns.


> +
> +SQMP
> +set-bootindex
> +--------------------

Match the ---- length to the command name.

> +++ b/qmp.c
> @@ -684,6 +684,23 @@ void qmp_object_del(const char *id, Error **errp)
>      object_unparent(obj);
>  }
>  
> +void qmp_set_bootindex(const char *id, int64_t bootindex,
> +                     bool has_suffix, const char *suffix, Error **errp)

Indentation is off.

> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), id);
> +    if (NULL == dev) {

Code like Yoda we do not.  This is more idiomatically written 'if
(!dev)' or 'if (dev == NULL)'.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]