Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Markus Armbruster <armbru@redhat.com>]

In the title: "qmp:" 

Peter Maydell <peter.maydell@linaro.org> writes:

> The 'singlestep' member of StatusInfo has never done what
> the QMP documentation claims it does. What it actually
> reports is whether TCG is working in "one guest instruction
> per translation block" mode.
>
> Create a new 'one-insn-per-tb' member whose name matches
> what the field actually does and the new command line
> options. Deprecate the old 'singlestep' field.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/about/deprecated.rst   | 10 ++++++++++
>  docs/interop/qmp-intro.txt  |  1 +
>  qapi/run-state.json         | 17 ++++++++++++++---
>  softmmu/runstate-hmp-cmds.c |  2 +-
>  softmmu/runstate.c          |  6 ++++--
>  5 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6f5e689aa45..dd36becdf3b 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -199,6 +199,16 @@ accepted incorrect commands will return an error. Users should make sure that
>  all arguments passed to ``device_add`` are consistent with the documented
>  property types.
>  
> +``StatusInfo`` member ``singlestep`` (since 8.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The ``singlestep`` member of the ``StatusInfo`` returned from
> +the ``query-status`` command is deprecated, because its name
> +is confusing and it never did what the documentation claimed
> +or what its name suggests. Use the ``one-insn-per-tb``
> +member instead, which reports the same information the old
> +``singlestep`` member did but under a clearer name.
> +
>  Human Monitor Protocol (HMP) commands
>  -------------------------------------
>  
> diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt
> index 1c745a7af04..b22916b23df 100644
> --- a/docs/interop/qmp-intro.txt
> +++ b/docs/interop/qmp-intro.txt
> @@ -73,6 +73,7 @@ Escape character is '^]'.
>  { "execute": "query-status" }
>  {
>      "return": {
> +        "one-insn-per-tb": false,
>          "status": "prelaunch", 
>          "singlestep": false, 
>          "running": false
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 9d34afa39e0..1de8c5c55d0 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -104,16 +104,27 @@
>  #
>  # @running: true if all VCPUs are runnable, false if not runnable
>  #
> -# @singlestep: true if VCPUs are in single-step mode
> +# @one-insn-per-tb: true if using TCG with one guest instruction
> +#                   per translation block
> +#
> +# @singlestep: deprecated synonym for @one-insn-per-tb
>  #
>  # @status: the virtual machine @RunState
>  #
> +# Features:
> +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb instead.

Wrap this line, please.

> +#
>  # Since: 0.14
>  #
> -# Notes: @singlestep is enabled through the GDB stub
> +# Notes: @one-insn-per-tb is enabled on the command line with
> +#        '-accel tcg,one-insn-per-tb=on', or with the HMP
> +#        'one-insn-per-tb' command.
>  ##

Hmm.  We report it in query-status, which means it's relevant to QMP
clients.  We provide the command to control it only in HMP, which means
it's not relevant to QMP clients.

Why is reading it relevant to QMP clients, but not writing?

Use cases for reading it via QMP query-status?

Have you considered tacking feature 'unstable' to it?

>  { 'struct': 'StatusInfo',
> -  'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} }
> +  'data': {'running': 'bool',
> +           'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
> +           'one-insn-per-tb': 'bool',
> +           'status': 'RunState'} }
>  
>  ##
>  # @query-status:

I see a bunch of query-status results in
tests/qemu-iotests/{183,234,262,280}.out.  Do they need an update?

[...]