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

[Markus Armbruster]

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.

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

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