| Message ID | 20171218184428.15074-2-danielhb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | deprecate qmp_cpu | expand |
On Mon, Dec 18, 2017 at 04:44:28PM -0200, Daniel Henrique Barboza wrote: > 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. > > 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 > 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 <danielhb@linux.vnet.ibm.com> Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel
Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> 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. > 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 <danielhb@linux.vnet.ibm.com> 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? 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... > --- > 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. > > +@section System emulator machine protocol commands > + > +@subsection 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)
On 12/19/2017 08:12 AM, Markus Armbruster wrote: > Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> 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 <danielhb@linux.vnet.ibm.com> > 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. >> >> +@section System emulator machine protocol commands >> + >> +@subsection 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)
On 12/19/2017 06:19 AM, Daniel Henrique Barboza wrote: >> 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. > My vote: (3). No one was using qmp_cpu for anything, while deprecation exists to allow a user a chance to fix their workflow to adjust from one thing that works (but badly) to its replacement that also works (and will be supported long term). Since our replacement is nothing, we are admitting that no one was relying on the command, and I don't see the point in a deprecation delay. Just nuke it! qemu-doc.texi doesn't even need to mention the command; the commit message is enough to document our reasons for complete removal.
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. +@section System emulator machine protocol commands + +@subsection 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)
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. 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 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 <danielhb@linux.vnet.ibm.com> --- qapi-schema.json | 5 ++++- qemu-doc.texi | 10 ++++++++++ qmp.c | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-)