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

[Daniel Henrique Barboza]

On 12/14/2017 01:21 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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.
Speaking of QMP and HMP 'entanglement', is the content of the wiki
still valid?

https://wiki.qemu.org/Features/QAPI

And under "HMP Conversion" we have:

"For HMP commands that don't have QMP equivalents today, new QMP functions
will be added to support these commands."

This in particular gave me the motivation to go ahead and implement qmp_cpu.
But then again, the last entry on Status is "3/6/2011" so yeah, I should 
have
asked here first whether the info from this wiki was relevant before sending
the patch.

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

My case was simply "HMP has it, QMP doesn't". I wasn't aware that QMP
must be as stateless as possible but HMP can retain state.

Now, is there any command that actually is impacted or makes use of the
current monitor CPU? I've searched a bit in qapi-schema.json and
hmp-commands.hx and haven't found any (although this does not
mean necessarily that no command is making use of it). Supposing
that no command makes good use of it, perhaps it's a good exercise
to evaluate if both qmp_cpu and hmp_cpu should be deprecated.

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

I'll give a try.


Daniel

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