Re: [Qemu-devel] [PATCH 11/12] qmp: add cpu_set qmp command

[Eric Blake <eblake@redhat.com>]

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

On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -1385,6 +1385,15 @@
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
>  ##
> +# @cpu_set
> +#

I already mentioned naming this cpu-set on your cover letter.  Additionally:

> +# Sets specified cpu to online/ofline mode

s/ofline/offline/

> +#
> +# Notes: semantics is : cpu_set id=x status=online|offline
> +##
> +{ 'command': 'cpu_set', 'data': {'id': 'int', 'status': 'str'} }

Don't open-code a 'str'.  My preference would be a bool, with slightly
different naming.

Also, this is not the typical documentation style used in this file.
Rather, it would be more like:

# Sets specified cpu to online/offline mode
#
# @id: cpu id to be updated
#
# @online: true to put the cpu online, false to take it offline
#
##
{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }


> +void qmp_cpu_set(int64_t id, const char *status, Error **errp)
> +{
> +    if (!strcmp(status, "online")) {
> +        do_cpu_hot_add(id, errp);
> +    } else if (!strcmp(status, "offline")) {
> +        error_setg(errp, "Unplug is not implemented");
> +    } else {
> +        error_setg(errp, "Invalid parameter '%s'", status);
> +        return;

This return could be omitted, with no change in behavior.  But again, I
think a bool rather than an open-coded string parser would make this
simpler:

void qmp_cpu_set(int64_t id, bool online, Error **errp)
{
    if (online) {
        do_cpu_hot_add(id, errp);
    } else {
        error_setg(errp, "Unplug is not implemented");
    }
}

-- 
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: 621 bytes --]