[Qemu-devel] [PATCH v2 0/1] deprecate qmp_cpu

[Qemu-devel] [PATCH v2 0/1] deprecate qmp_cpu

[Daniel Henrique Barboza]

v2:
- added more details about why qmp_cpu is being deprecated
- added a deprecation note in qapi-schema.json

Daniel Henrique Barboza (1):
  qmp: marking qmp_cpu as deprecated

 qapi-schema.json |  5 ++++-
 qemu-doc.texi    | 10 ++++++++++
 qmp.c            |  2 +-
 3 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.13.6

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

[Daniel Henrique Barboza]

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(-)

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)
-- 
2.13.6

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

[Daniel P. Berrange]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

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)

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

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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org