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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

If I remember that part of history correctly, implementing the command
in QMP was just as possible back then, but deemed a Bad Idea for the
reason Eric explains below.

What I don't quite remember is why we had to implement it in QMP as a
no-op.  Might have been due to the way QMP and HMP were entangled back
then.

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

Yes.

The standard way to deprecate a feature is to add it to appendix
"Deprecated features" in qemu-doc.texi, and make its use trigger
suitable deprecation messages, pointing to a replacement if any.

Unfortunately, we still lack means to signal "X is deprecated, use Y
instead" to a QMP client.  Not important in this case, because the
command has never worked.