Re: [PATCH 01/10] qapi: expose rtc-reset-reinjection command unconditionally

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This removes the TARGET_I386 condition from the rtc-reset-reinjection
> command. This requires providing a QMP command stub for non-i386 target.
> This in turn requires moving the command out of misc-target.json, since
> that will trigger symbol poisoning errors when built from target
> independent code.
>
> Rather than putting the command into misc.json, it is proposed to create
> misc-$TARGET.json files to hold commands whose impl is conceptually
> only applicable to a single target. This gives an obvious docs hint to
> consumers that the command is only useful in relation a specific target,
> while misc.json is for commands applicable to 2 or more targets.

Starting with this patch, the series structures the manual like this:

    = Machines
    ... contents of machine.json ...
    == Specific to S390
    ... contents of machine-s390.json ...

and

    = Miscellanea
    ... contents of misc.json ...
    == Specific to ARM
    ... contents of misc-arm.json ...
    == Specific to i386
    ... contents of misc-i386.json ...

Except it doesn't add == subsection headers, but that's detail.  The
text I show for them here is crap.

Possible alternative: collect the target-specific stuff in one place
rather than two:

    = Targets
    == ARM
    == i386
    == S390

Again the header text is crap.

Is separating the current contents of misc-<target>.json from
machine-<target>.json useful?

> The current impl of qmp_rtc_reset_reinject() is a no-op if the i386
> RTC is disabled in Kconfig, or if the running machine type lack any
> RTC device. Thus the stub impl for non-i386 targets retains this
> no-op behaviour, instead of reporting a Error which is the more usual
> choice for commands invoked against unsupported configurations.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/i386/monitor.c        |  2 +-
>  qapi/meson.build         |  1 +
>  qapi/misc-i386.json      | 24 ++++++++++++++++++++++++
>  qapi/misc-target.json    | 17 -----------------
>  qapi/qapi-schema.json    |  1 +
>  stubs/meson.build        |  1 +
>  stubs/monitor-i386-rtc.c | 10 ++++++++++
>  7 files changed, 38 insertions(+), 18 deletions(-)
>  create mode 100644 qapi/misc-i386.json
>  create mode 100644 stubs/monitor-i386-rtc.c
>
> diff --git a/hw/i386/monitor.c b/hw/i386/monitor.c
> index 1921e4d52e..79df96562f 100644
> --- a/hw/i386/monitor.c
> +++ b/hw/i386/monitor.c
> @@ -26,7 +26,7 @@
>  #include "monitor/monitor.h"
>  #include "qobject/qdict.h"
>  #include "qapi/error.h"
> -#include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qapi-commands-misc-i386.h"
>  #include "hw/i386/x86.h"
>  #include "hw/rtc/mc146818rtc.h"
>  
> diff --git a/qapi/meson.build b/qapi/meson.build
> index eadde4db30..3a9bd06104 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -64,6 +64,7 @@ if have_system
>      'qdev',
>      'pci',
>      'rocker',
> +    'misc-i386',
>      'tpm',
>      'uefi',
>    ]
> diff --git a/qapi/misc-i386.json b/qapi/misc-i386.json
> new file mode 100644
> index 0000000000..d5bfd91405
> --- /dev/null
> +++ b/qapi/misc-i386.json
> @@ -0,0 +1,24 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later

Might be cleaner to add this to all qapi/*.json first, and in a separate
patch.

> +
> +##
> +# @rtc-reset-reinjection:
> +#
> +# This command will reset the RTC interrupt reinjection backlog.  Can
> +# be used if another mechanism to synchronize guest time is in effect,
> +# for example QEMU guest agent's guest-set-time command.
> +#
> +# Use of this command is only applicable for x86 machines with an RTC,
> +# and on other machines will silently return without performing any
> +# action.

This paragraph replaces ...

> +#
> +# Since: 2.1
> +#
> +# .. qmp-example::
> +#
> +#     -> { "execute": "rtc-reset-reinjection" }
> +#     <- { "return": {} }
> +##
> +{ 'command': 'rtc-reset-reinjection' }
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 42e4a7417d..5d0ffb0164 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -2,23 +2,6 @@
>  # vim: filetype=python
>  #
>  
> -##
> -# @rtc-reset-reinjection:
> -#
> -# This command will reset the RTC interrupt reinjection backlog.  Can
> -# be used if another mechanism to synchronize guest time is in effect,
> -# for example QEMU guest agent's guest-set-time command.
> -#
> -# Since: 2.1
> -#
> -# .. qmp-example::
> -#
> -#     -> { "execute": "rtc-reset-reinjection" }
> -#     <- { "return": {} }
> -##
> -{ 'command': 'rtc-reset-reinjection',
> -  'if': 'TARGET_I386' }

... the conditional.

Before, attempting to execute the command fails with CommandNotFound.

Afterwards it succeeds without doing anything.  I think it should fail
instead.  CommandNotFound would be a lie, so change it to GenericError.

"Specific to target" is no longer machine-readable.  Should
machine-readability be (or become) desirable, we could expose it via
suitable QAPI features, e.g. 'features': ['target-i386'].

> -
>  ##
>  # @SevState:
>  #
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 7bc600bb76..96f6aa4413 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -61,6 +61,7 @@
>  { 'include': 'replay.json' }
>  { 'include': 'yank.json' }
>  { 'include': 'misc.json' }
> +{ 'include': 'misc-i386.json' }
>  { 'include': 'misc-target.json' }
>  { 'include': 'audio.json' }
>  { 'include': 'acpi.json' }
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 63392f5e78..9907b54c1e 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -77,6 +77,7 @@ if have_system
>    stub_ss.add(files('target-monitor-defs.c'))
>    stub_ss.add(files('win32-kbd-hook.c'))
>    stub_ss.add(files('xen-hw-stub.c'))
> +  stub_ss.add(files('monitor-i386-rtc.c'))
>  endif
>  
>  if have_system or have_user
> diff --git a/stubs/monitor-i386-rtc.c b/stubs/monitor-i386-rtc.c
> new file mode 100644
> index 0000000000..ee2e60d95b
> --- /dev/null
> +++ b/stubs/monitor-i386-rtc.c
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-misc-i386.h"
> +
> +void qmp_rtc_reset_reinjection(Error **errp)
> +{
> +    /* Nothing to do since non-x86 machines lack an RTC */
> +}

I think I'd create one stub file per qapi/<foo>-<target>.json.