Re: [Qemu-devel] [PATCH v1 1/2] qmp.c: (re)implement qmp_cpu

[Eric Blake <eblake@redhat.com>]

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

On 12/13/2017 12:15 PM, Daniel Henrique Barboza wrote:
> Commit 755f196898 ("qapi: Convert the cpu command") added the qmp_cpu
> function in qmp.c, leaving it blank. It the same commit, a working
> hmp_cpu was implemented. Since then, no further changes were made in
> qmp_cpu, resulting now in a working 'cpu' command that works in HMP
> and a 'cpu' command in QMP that does nothing.
> 
> Regardless of what constraints were involved that time in not implemeting
> qmp_cpu, at this moment it is possible to have both. This patch brings
> the logic of hmp_cpu to qmp_cpu and converts the HMP function to use its
> QMP counterpart.

I'm not sure I like this. HMP is stateful (you have to remember what
previous 'cpu' commands have been run to tell what the current command
will do).  That may be convenient (if not confusing) to humans, but is
lousy for machine interfaces.  QMP should be stateless as much as
possible - for any command that would behave differently according to
what CPU is selected, THAT command (and not a different 'cpu' command
executed previously) should have a cpu argument alongside all its other
parameters.

So unless you have a really strong use case for this, I don't think we
want it.


> +++ b/qapi-schema.json
> @@ -1048,11 +1048,19 @@
>  ##
>  # @cpu:
>  #
> -# This command is a nop that is only provided for the purposes of compatibility.
> +# Set the default CPU.
>  #
> -# Since: 0.14.0
> +# @index: The index of the virtual CPU to be set as default
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.12.0
> +#
> +# Example:
> +#
> +# -> { "execute": "cpu", "arguments": { "index": 2 } }
> +# <- { "return": {} }
>  #
> -# Notes: Do not use this command.
>  ##
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
> diff --git a/qmp.c b/qmp.c
> index e8c303116a..c482225d5c 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -115,7 +115,9 @@ void qmp_system_powerdown(Error **erp)
>  
>  void qmp_cpu(int64_t index, Error **errp)
>  {
> -    /* Just do nothing */
> +    if (monitor_set_cpu(index) < 0) {
> +        error_setg(errp, "Invalid CPU index");
> +    }
>  }
>  
>  void qmp_cpu_add(int64_t id, Error **errp)
> 

Better yet, let's document that 'cpu' is deprecated, so that we can
remove it from QMP altogether in a couple of releases.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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