Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> This adds and parses the --monitor option, so that a QMP monitor can be
> used in the storage daemon. The monitor offers commands defined in the
> QAPI schema at storage-daemon/qapi/qapi-schema.json.

I feel we should explain the module sharing between the two QAPI
schemata here.  It's not exactly trivial, as we'll see below.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  storage-daemon/qapi/qapi-schema.json | 15 ++++++++++++
>  qemu-storage-daemon.c                | 34 ++++++++++++++++++++++++++++
>  Makefile                             | 30 ++++++++++++++++++++++++
>  Makefile.objs                        |  4 ++--
>  monitor/Makefile.objs                |  2 ++
>  qapi/Makefile.objs                   |  5 ++++
>  qom/Makefile.objs                    |  1 +
>  scripts/qapi/gen.py                  |  5 ++++
>  storage-daemon/Makefile.objs         |  1 +
>  storage-daemon/qapi/Makefile.objs    |  1 +
>  10 files changed, 96 insertions(+), 2 deletions(-)
>  create mode 100644 storage-daemon/qapi/qapi-schema.json
>  create mode 100644 storage-daemon/Makefile.objs
>  create mode 100644 storage-daemon/qapi/Makefile.objs
>
> diff --git a/storage-daemon/qapi/qapi-schema.json b/storage-daemon/qapi/qapi-schema.json
> new file mode 100644
> index 0000000000..58c561ebea
> --- /dev/null
> +++ b/storage-daemon/qapi/qapi-schema.json
> @@ -0,0 +1,15 @@
> +# -*- Mode: Python -*-
> +
> +{ 'include': '../../qapi/pragma.json' }
> +
> +{ 'include': '../../qapi/block.json' }
> +{ 'include': '../../qapi/block-core.json' }
> +{ 'include': '../../qapi/char.json' }
> +{ 'include': '../../qapi/common.json' }
> +{ 'include': '../../qapi/crypto.json' }
> +{ 'include': '../../qapi/introspect.json' }
> +{ 'include': '../../qapi/job.json' }
> +{ 'include': '../../qapi/monitor.json' }
> +{ 'include': '../../qapi/qom.json' }
> +{ 'include': '../../qapi/sockets.json' }
> +{ 'include': '../../qapi/transaction.json' }
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index 46e0a6ea56..4939e6b41f 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -28,12 +28,16 @@
>  #include "block/nbd.h"
>  #include "chardev/char.h"
>  #include "crypto/init.h"
> +#include "monitor/monitor.h"
> +#include "monitor/monitor-internal.h"
>  
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-block.h"
>  #include "qapi/qapi-commands-block-core.h"
> +#include "qapi/qapi-commands-monitor.h"

Aren't these three redundant with ...

>  #include "qapi/qapi-visit-block.h"
>  #include "qapi/qapi-visit-block-core.h"
> +#include "qapi/qmp/qstring.h"
>  #include "qapi/qobject-input-visitor.h"
>  
>  #include "qemu-common.h"
> @@ -46,6 +50,8 @@
>  #include "qemu/option.h"
>  #include "qom/object_interfaces.h"
>  
> +#include "storage-daemon/qapi/qapi-commands.h"
> +

... this one now?

>  #include "sysemu/runstate.h"
>  #include "trace/control.h"
>  
> @@ -58,6 +64,11 @@ void qemu_system_killed(int signal, pid_t pid)
>      exit_requested = true;
>  }
>  
> +void qmp_quit(Error **errp)
> +{
> +    exit_requested = true;
> +}
> +
>  static void help(void)
>  {
>      printf(
> @@ -101,6 +112,7 @@ enum {
>      OPTION_OBJECT = 256,
>      OPTION_BLOCKDEV,
>      OPTION_CHARDEV,
> +    OPTION_MONITOR,
>      OPTION_NBD_SERVER,
>      OPTION_EXPORT,
>  };

Recommend to keep these sorted alphabetically.

> @@ -116,6 +128,17 @@ static QemuOptsList qemu_object_opts = {
>      },
>  };
>  
> +static void init_qmp_commands(void)
> +{
> +    qmp_init_marshal(&qmp_commands);
> +    qmp_register_command(&qmp_commands, "query-qmp-schema",
> +                         qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
> +
> +    QTAILQ_INIT(&qmp_cap_negotiation_commands);
> +    qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
> +                         qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
> +}

Duplicates monitor_init_qmp_commands() less two 'gen': false commands
that don't exist in the storage daemon.

Hmm, could we let commands.py generate command registration even for
'gen': false commands?

> +
>  static void init_export(BlockExport *export, Error **errp)
>  {
>      switch (export->type) {
> @@ -138,6 +161,7 @@ static int process_options(int argc, char *argv[], Error **errp)
>          {"object", required_argument, 0, OPTION_OBJECT},
>          {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
>          {"chardev", required_argument, 0, OPTION_CHARDEV},
> +        {"monitor", required_argument, 0, OPTION_MONITOR},
>          {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
>          {"export", required_argument, 0, OPTION_EXPORT},
>          {"version", no_argument, 0, 'V'},
> @@ -208,6 +232,14 @@ static int process_options(int argc, char *argv[], Error **errp)
>                  qemu_opts_del(opts);
>                  break;
>              }
> +        case OPTION_MONITOR:
> +            {
> +                QemuOpts *opts = qemu_opts_parse(&qemu_mon_opts,
> +                                                 optarg, true, &error_fatal);
> +                monitor_init_opts(opts, false, &error_fatal);
> +                qemu_opts_del(opts);
> +                break;
> +            }
>          case OPTION_NBD_SERVER:
>              {
>                  Visitor *v;

Recommend to keep the switch cases sorted.  Perhaps put help first.
Order the short options string and the long_options[] array to match.

> @@ -272,6 +304,8 @@ int main(int argc, char *argv[])
>      qemu_add_opts(&qemu_trace_opts);
>      qcrypto_init(&error_fatal);
>      bdrv_init();
> +    monitor_init_globals_core();
> +    init_qmp_commands();
>  
>      if (qemu_init_main_loop(&local_err)) {
>          error_report_err(local_err);
> diff --git a/Makefile b/Makefile
> index 0e3e98582d..e367d2b28a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -121,7 +121,26 @@ GENERATED_QAPI_FILES += $(QAPI_MODULES:%=qapi/qapi-events-%.c)
>  GENERATED_QAPI_FILES += qapi/qapi-introspect.c qapi/qapi-introspect.h
>  GENERATED_QAPI_FILES += qapi/qapi-doc.texi
>  
> +GENERATED_STORAGE_DAEMON_QAPI_FILES = storage-daemon/qapi/qapi-builtin-types.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-builtin-types.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-builtin-visit.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-builtin-visit.c

Any particular reason for not reusing qapi/qapi-builtin-*?  See also the
recipe for storage-daemon/qapi/qapi-gen-timestamp below.

> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-commands.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-commands.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-emit-events.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-emit-events.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-events.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-events.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-introspect.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-introspect.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-types.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-types.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-visit.h
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-visit.c
> +GENERATED_STORAGE_DAEMON_QAPI_FILES += storage-daemon/qapi/qapi-doc.texi

These are the files generated for the storage daemon's main module.  The
files generated for its sub-modules (all shared right now) are absent.
I think this could use a comment.

See also scripts/qapi/gen.py below.

> +
>  generated-files-y += $(GENERATED_QAPI_FILES)
> +generated-files-y += $(GENERATED_STORAGE_DAEMON_QAPI_FILES)
>  
>  generated-files-y += trace/generated-tcg-tracers.h
>  
> @@ -616,6 +635,17 @@ qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
>  		"GEN","$(@:%-timestamp=%)")
>  	@>$@
>  
> +qapi-modules-storage-daemon = \
> +	$(SRC_PATH)/storage-daemon/qapi/qapi-schema.json \
> +    $(QAPI_MODULES_STORAGE_DAEMON:%=$(SRC_PATH)/qapi/%.json)
> +
> +$(GENERATED_STORAGE_DAEMON_QAPI_FILES): storage-daemon/qapi/qapi-gen-timestamp ;
> +storage-daemon/qapi/qapi-gen-timestamp: $(qapi-modules-storage-daemon) $(qapi-py)
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
> +		-o "storage-daemon/qapi" -b $<, \

If we can reuse qapi/qapi-builtin-*, then omit -b to suppress generating
redundant copies.

> +		"GEN","$(@:%-timestamp=%)")
> +	@>$@
> +
>  QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h)
>  $(qga-obj-y): $(QGALIB_GEN)
>  
> diff --git a/Makefile.objs b/Makefile.objs
> index b667d3f07b..d4e0daddee 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -41,8 +41,8 @@ io-obj-y = io/
>  # storage-daemon-obj-y is code used by qemu-storage-daemon (these objects are
>  # used for system emulation, too, but specified separately there)
>  
> -storage-daemon-obj-y = block/
> -storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o
> +storage-daemon-obj-y = block/ monitor/ qapi/ qom/ storage-daemon/
> +storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o job-qmp.o
>  storage-daemon-obj-$(CONFIG_WIN32) += os-win32.o
>  storage-daemon-obj-$(CONFIG_POSIX) += os-posix.o
>  
> diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
> index 15eb6380c5..6e4ef60601 100644
> --- a/monitor/Makefile.objs
> +++ b/monitor/Makefile.objs
> @@ -2,3 +2,5 @@ obj-y += misc.o
>  common-obj-y += monitor.o qmp.o hmp.o
>  common-obj-y += qmp-cmds.o qmp-cmds-monitor.o
>  common-obj-y += hmp-cmds.o
> +
> +storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-monitor.o
> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
> index 3e04e299ed..03d256f0a4 100644
> --- a/qapi/Makefile.objs
> +++ b/qapi/Makefile.objs
> @@ -30,3 +30,8 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
>  obj-y += qapi-events.o
>  obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
>  obj-y += qapi-commands.o
> +
> +QAPI_MODULES_STORAGE_DAEMON = block block-core char common crypto introspect
> +QAPI_MODULES_STORAGE_DAEMON += job monitor qom sockets pragma transaction
> +
> +storage-daemon-obj-y += $(QAPI_MODULES_STORAGE_DAEMON:%=qapi-commands-%.o)
> diff --git a/qom/Makefile.objs b/qom/Makefile.objs
> index f9d77350ac..1b45d104ba 100644
> --- a/qom/Makefile.objs
> +++ b/qom/Makefile.objs
> @@ -2,3 +2,4 @@ qom-obj-y = object.o container.o qom-qobject.o
>  qom-obj-y += object_interfaces.o
>  
>  common-obj-$(CONFIG_SOFTMMU) += qom-hmp-cmds.o qom-qmp-cmds.o
> +storage-daemon-obj-y += qom-qmp-cmds.o
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 796c17c38a..c25634f673 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -44,6 +44,11 @@ class QAPIGen(object):
>          return ''
>  
>      def write(self, output_dir):
> +        # Include paths starting with ../ are used to reuse modules of the main
> +        # schema in specialised schemas. Don't overwrite the files that are
> +        # already generated for the main schema.
> +        if self.fname.startswith('../'):
> +            return
>          pathname = os.path.join(output_dir, self.fname)
>          dir = os.path.dirname(pathname)
>          if dir:

Ugh.

tests/qapi-schema/include/sub-module.json violates the stated assumption
"Include paths starting with ../ are used to reuse modules of the main
schema":

    { 'include': '../sub-sub-module.json' }

Works anyway because it's only used via

    { 'include': 'include/sub-module.json' }

where the ../ is folded with the include/ and vanishes.  self.fname is
relative to the main schema's directory.

Still, baking the assumption into gen.py makes it a part of the QAPI
schema language.  It needs to be documented as such.

Complication: because we generate array types and supporting code code
only when they're actually used, the code generated for a shared module
can depend on how it is used outside the shared module.

Right now, storage-daemon/qapi/qapi-schema.json contains nothing but
modules shared with qapi/qapi-schema.json.  Works.

If storage-daemon/qapi/qapi-schema.json ever acquires an array reference
that is not also in qapi/qapi-schema.json, the generated C won't
compile.

This also needs to be documented because it's anything but obvious.

Instead of suppressing code generated for reused modules outright, we
could somehow avoid clobbering the main schema's output.  I guess we
can't use -p for that, because it makes interferes with reusing the
hand-written QMP code.  Can we make do with -o?

By doing that, we sidestep making ../ special in the QAPI schema
language.  We then have two options for the storage daemon.

One, use the code generated for the storage daemon schema.  Simple,
robust, but we compile more.

Two, we use the code generated for the main schema (same as now).  We
compile less, but expose ourselves to the array problem described
above.

> diff --git a/storage-daemon/Makefile.objs b/storage-daemon/Makefile.objs
> new file mode 100644
> index 0000000000..cfe6beee52
> --- /dev/null
> +++ b/storage-daemon/Makefile.objs
> @@ -0,0 +1 @@
> +storage-daemon-obj-y += qapi/
> diff --git a/storage-daemon/qapi/Makefile.objs b/storage-daemon/qapi/Makefile.objs
> new file mode 100644
> index 0000000000..df8946bdae
> --- /dev/null
> +++ b/storage-daemon/qapi/Makefile.objs
> @@ -0,0 +1 @@
> +storage-daemon-obj-y += qapi-commands.o qapi-introspect.o

Now let's take a step back and examine the problem this patch solves.

The storage daemon needs a control interface.  Instead of inventing a
new one, we want to reuse QMP, both the protocol and the actual
commands, as far as they make sense in the storage daemon.  Makes sense.

To get the QMP protocol, the storage daemon needs a QAPI schema.

To reuse existing QMP commands, the storage daemon's QAPI schema needs
to include the parts of the existing QAPI schema that define them.

The stupidest possible way to do that is copying.  Unworkable; too much
code, too hard to keep it in sync.

Instead, this patch reuses schema modules.  Summary of issues so far:

* The include directive lets us avoid duplicating schema code, but to
  avoid duplicating the generated code, we resort to a gross hack.

  The hack needs a certain amount of rather awkward documentation (still
  to be written).

  Putting non-shared bits into the storage daemon's schema risks array
  trouble.

  Accepting duplicated generated code might be the lesser evil.

* The discussion of how this module reuse thing works, assumptions,
  shortcomings need to go on the permanent record, be it code comments,
  something in docs/, or at least commit messages.

  Commit messages of PATCH 13 and 16 need to hint at where we're going
  to make sense.  I figure that's easier to do once we got this patch
  into shape.

* We get a bunch of commands that make no sense in the storage daemon,
  see my review of PATCH 04 for examples.

  To avoid that, we need to split modules.  May well be a good idea
  anyway; qapi/block-core.json is by far the fattest module: 850
  non-blank, non-comment lines, 4.5 times fatter than the runner-up
  ui.json.

  Keeping unwanted commands out of the storage daemon will be an ongoing
  process: we need to keep unwanted commands from creeping into modules
  shared with the storage daemon.  Hopefully, properly focused modules
  can make such creep unlikely.

A less hacky solution is to extend the QAPI schema language from one set
of QMP commands and events to many.  Hard to justify when we have just
two sets.

I'm afraid my review isn't a straight "do A, B and C to get my
blessings".  I hope it's useful anyway.