Re: [Qemu-devel] [PATCH v2 1/1] qmp: marking qmp_cpu as deprecated

[Daniel Henrique Barboza]

On 12/19/2017 08:12 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <address@hidden> writes:
>
> > qmp_cpu is a nop that was created a while ago in commit 755f196898
> > ("qapi: Convert the cpu command") for the sake of compatibility,
> > due to the existence of hmp_cpu.
> The compatibility argument makes no sense to me, but it's the reason the
> commit documented.
Reading the thread that added it back in 2011*, I am guessing that the
compatibility argument is based on the premise that every HMP command
should have a QMP counterpart. This also makes sense considering the
QAPI FAQ page I've mentioned a few days ago.
> > Today, there is no need or requirement to keep it as is. QMP is
> > meant to be as stateless as possible, thus any QMP command that
> > needs a specific monitor CPU setup should provide it in its
> s/should/must/
>
> > arguments, instead of relying in the current QMP monitor state.
> >
> > This patch flags qmp_cpu as deprecated in qemu-doc.texi,
> > qapi-schema.json and changes qmp_cpu body to show a deprecation
> > message if used.
> >
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> The "deprecation message" is actually a hard error.  This is unusual.
> When we deprecate an interface, we don't normally make its use fail, we
> just warn.  QMP doesn't provide a good way to warn, though.
>
> Let's compare behavior:
>
> 0. Status quo                       do nothing and succeed
> 1. Your patch                       fail with GenericError
> 2. Your patch less error_setg()     do nothing and succeed
> 3. Immediate removal                fail with CommandNotFound
>
> Does the difference between 1. and 3. matter at all?  Or is it just
> deprecation cargo cult?

Good point. If both the deprecation message and the removal will
result in error for the user ....

> Here's my take on it.  I'd prefer 3. Immediate removal.  I'd also be
> fine with 2. Your patch less error_setg(), followed by removal after the
> customary deprecation period.  1. plus later removal just doesn't make
> sense to me, but it's really no big deal, so if you guys want it...

It all goes down to whether we consider qmp_cpu, a QMP command that does
nothing since its creation and apparently no one ever used it, a 
feature. If it's a
feature, then I vote for (2) and removal after 2 releases following the 
regular
deprecation policy.

If we consider that applying the policy to qmp_cpu is more trouble than 
it's worth,
I'll gladly resend this patch removing it entirely and rewording the 
qemu-doc.texi
entry to mention that it was removed because it did nothing and it was of
no use for anyone.



danielhb

> > ---
> >   qapi-schema.json |  5 ++++-
> >   qemu-doc.texi    | 10 ++++++++++
> >   qmp.c            |  2 +-
> >   3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 18457954a8..b51b89e19b 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1052,7 +1052,10 @@
> >   #
> >   # Since: 0.14.0
> >   #
> > -# Notes: Do not use this command.
> > +# Notes: Do not use this command. It was deprecated in release 2.12.0 and will
> > +#        be removed, with no replacement, at any time in the future. QMP
> > +#        commands that rely on the current CPU monitor should specify it as a
> > +#        parameter.
> >   ##
> >   { 'command': 'cpu', 'data': {'index': 'int'} }
> >   
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index f7317dfc66..09e6c5046a 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -2516,6 +2516,16 @@ subsystem image.
> >   The ``convert -s snapshot_id_or_name'' argument is obsoleted
> >   by the ``convert -l snapshot_param'' argument instead.
> >   
> > address@hidden System emulator machine protocol commands
> > +
> > address@hidden qmp_cpu (since 2.12.0)
> > +
> > +The ``qmp_cpu'' command was implemented in release 0.14.0 as a functional
> > +no-op, remaining as such up to release 2.12.0 when it was deprecated and
> > +flagged for future removal. No replacement will be provided: any QMP command
> > +that uses the current CPU monitor should, instead, specify the CPU in its
> > +parameters.
> > +
> >   @section System emulator human monitor commands
> >   
> >   @subsection host_net_add (since 2.10.0)
> > diff --git a/qmp.c b/qmp.c
> > index e8c303116a..d8543d713d 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -115,7 +115,7 @@ void qmp_system_powerdown(Error **erp)
> >   
> >   void qmp_cpu(int64_t index, Error **errp)
> >   {
> > -    /* Just do nothing */
> > +    error_setg(errp, "qmp_cpu is deprecated, do not use this command");
> >   }
> >   
> >   void qmp_cpu_add(int64_t id, Error **errp)