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

[Markus Armbruster <armbru@redhat.com>]

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

> On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> In the title: "qmp:"
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > 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?
>
> I suspect that neither is very relevant to QMP clients, but I
> thought we had a requirement that HMP interfaces went
> via QMP ones ?

Kind of.  Here's my current boilerplate on the subject:

    HMP commands without a QMP equivalent are okay if their
    functionality makes no sense in QMP, or is of use only for human
    users.

    Example for "makes no sense in QMP": setting the current CPU,
    because a QMP monitor doesn't have a current CPU.

    Examples for "is of use only for human users": HMP command "help",
    the integrated pocket calculator.

    Debugging commands are kind of borderline.  Debugging is commonly a
    human activity, where HMP is just fine.  However, humans create
    tools to assist with their activities, and then QMP is useful.
    While I wouldn't encourage HMP-only for the debugging use case, I
    wouldn't veto it.

When adding an HMP-only command, explain why it is HMP-only in the
commit message.

>                If not, we could just make the HMP query
> interface directly look at the TCG property, the way the
> write interface does.

How useful is it HMP?

> I don't want to add a QMP interface for writing it unless
> there's somebody who actually has a use case for it.
>
>> Use cases for reading it via QMP query-status?
>>
>> Have you considered tacking feature 'unstable' to it?
>
> Nope, because I don't know anything about what that does :-)

docs/devel/qapi-code-gen.rst explains:

    Special features
    ~~~~~~~~~~~~~~~~

    Feature "deprecated" marks a command, event, enum value, or struct
    member as deprecated.  It is not supported elsewhere so far.
    Interfaces so marked may be withdrawn in future releases in accordance
    with QEMU's deprecation policy.

    Feature "unstable" marks a command, event, enum value, or struct
    member as unstable.  It is not supported elsewhere so far.  Interfaces
    so marked may be withdrawn or changed incompatibly in future releases.


We use "unstable" for debugging aids, testing aids, experiments /
unfinished work.

>> >  { '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?
>
> "make check" passed, so I guess not, unless those don't run
> in "make check"...

Plenty of iotests don't run in "make check".  Try

    $ tests/qemu-iotests/check -qcow2 183 234 262 280

> Do those .out files need exact matching output, or can they
> be written to say "we don't care about what value this field
> has or whether it exists" ?

If (hazy) memory serves, there's some normalization.  I doubt it'll
affect this member, i.e. you need to put it there.