Eric Blake <ebl...@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.
