[RFC,18/18] qemu-storage-daemon: Add --monitor option
diff mbox series

Message ID 20191017130204.16131-19-kwolf@redhat.com
State New
Headers show
Series
  • Add qemu-storage-daemon
Related show

Commit Message

Kevin Wolf Oct. 17, 2019, 1:02 p.m. UTC
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.

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

Comments

Max Reitz Nov. 6, 2019, 2:32 p.m. UTC | #1
On 17.10.19 15:02, Kevin Wolf wrote:
> 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.
> 
> 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/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

I took a look into the rest, and I wonder whether query-iothreads from
misc.json would be useful, too.

> 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

Sorry, but I’m about to ask a clueless question: How do we ensure that
the main schema is generated before something else could make sure of
the specialized schemas?

Max

>          pathname = os.path.join(output_dir, self.fname)
>          dir = os.path.dirname(pathname)
>          if dir:
Kevin Wolf Nov. 7, 2019, 10:12 a.m. UTC | #2
Am 06.11.2019 um 15:32 hat Max Reitz geschrieben:
> On 17.10.19 15:02, Kevin Wolf wrote:
> > 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.
> > 
> > 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/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
> 
> I took a look into the rest, and I wonder whether query-iothreads from
> misc.json would be useful, too.

Possibly. It would be a separate patch, but I can add it.

The question is just where to move query-iothreads. Do we have a good
place, or do I need to separate misc.json and a new misc-sysemu.json?

> > 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
> 
> Sorry, but I’m about to ask a clueless question: How do we ensure that
> the main schema is generated before something else could make sure of
> the specialized schemas?

"Make sure"?

I think the order of the generation doesn't matter because generating
the storage daemon files doesn't actually access the main ones.
Generated C files shouldn't be a problem either because if we link an
object file into a binary, we have a make dependency for it.

Maybe the only a bit trickier question is whether we have the
dependencies right so that qemu-storage-daemon.c is only built after the
header files of both the main schema and the specific one have been
generated. If I understand the Makefile correctly, generated-files-y
takes care of this, and this patch adds all new header files to it if I
didn't miss any.

Kevin
Max Reitz Nov. 7, 2019, 10:44 a.m. UTC | #3
On 07.11.19 11:12, Kevin Wolf wrote:
> Am 06.11.2019 um 15:32 hat Max Reitz geschrieben:
>> On 17.10.19 15:02, Kevin Wolf wrote:
>>> 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.
>>>
>>> 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/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
>>
>> I took a look into the rest, and I wonder whether query-iothreads from
>> misc.json would be useful, too.
> 
> Possibly. It would be a separate patch, but I can add it.
> 
> The question is just where to move query-iothreads. Do we have a good
> place, or do I need to separate misc.json and a new misc-sysemu.json?

I’d just put it in block.json because of the “io”... 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
>>
>> Sorry, but I’m about to ask a clueless question: How do we ensure that
>> the main schema is generated before something else could make sure of
>> the specialized schemas?
> 
> "Make sure"?

Oops, s/ sure/ use/.

> I think the order of the generation doesn't matter because generating
> the storage daemon files doesn't actually access the main ones.
> Generated C files shouldn't be a problem either because if we link an
> object file into a binary, we have a make dependency for it.

I was mostly wondering about the fact that make mustn’t try to compile
the “generated files” (which aren’t really generated here) before they
are actually generated when the main schema is processed.

Max

> Maybe the only a bit trickier question is whether we have the
> dependencies right so that qemu-storage-daemon.c is only built after the
> header files of both the main schema and the specific one have been
> generated. If I understand the Makefile correctly, generated-files-y
> takes care of this, and this patch adds all new header files to it if I
> didn't miss any.
> 
> Kevin
>
Markus Armbruster Nov. 8, 2019, 8:59 a.m. UTC | #4
Quick observation: --help fails to mention --monitor.
Markus Armbruster Nov. 12, 2019, 2:25 p.m. UTC | #5
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.
Kevin Wolf Nov. 13, 2019, 10:58 a.m. UTC | #6
Am 12.11.2019 um 15:25 hat Markus Armbruster geschrieben:
> 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?

Looks like it.

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

Possibly. Are you telling me to do it or is it just an idea for a later
cleanup?

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

Hm, I guess not. I'm just tweaking stuff until it seems to work, and
this seemed to work, so...

But as far as I can see, the generated file are never actually used, so
I'll get rid of them.

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

Ok.

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

That's why I asked you before writing it like this. :-)

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

Maybe you'd like it better if we made it explicit?

    { 'include': '../../qapi/block.json', 'gen': false }

But how do I actually get this information down to QAPIGen? This brings
us back to the problem that the QAPI generator doesn't really have a
notion of modules, but only entities have a module name and the first
entity to use a module name causes it to be generated. We don't have
access to the QAPISchemaInclude that created the module.

Even the QAPISchema._modules that I introduced only contains filenames.
Should it contain QAPISchemaInclude objects instead?

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

A comment in the storage daemon main module?

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

I guess we could by artificially generating to a sub-sub-directory, so
that ../../ relative to the output directory specified with -o is still
a directory specific to the storage daemon. Not very nice, though.

It also results in (hopefully!) unused files with duplicate file names
that are supposed to be mostly the same, modulo bugs.

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

Not simple at all: What do you mean by "use"? For linking? For compiling
qemu-storage-daemon.c? For compiling blockdev.c?

The latter would mean that we compile everything that uses QAPI types
twice. Considering that we're trying to get rid of per-target
compilation as much as we can, it doesn't appear desirable to start
compiling shared files multiple times for the storage daemon. (I think
you only meant the generated files, not everything that uses QAPI types,
when you said "we compile more"?)

Only compiling a single file with the headers generated from the storage
daemon schema doesn't sound like a great idea either. The storage daemon
headers _should_ be a subset of the main schema ones, but if there is
ever a bug and data structures differ, I don't want to be the poor guy
to debug this. And anyway - you can compile against different versions
of the header, but you can link in only one version of the generated .c
files. I'm not sure what to call this, but "robust" is probably not in
the selection.

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

I think we can live with this restriction. If we ever need an array
type only in the storage daemon, we can still define the type that uses
it in the main schema, but leave it unused there.

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

Can possibly be solved by making it explicit.

>   Putting non-shared bits into the storage daemon's schema risks array
>   trouble.
> 
>   Accepting duplicated generated code might be the lesser evil.

I disagree on this one.

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

Maybe we can have a bit of a protection against this by moving
qmp_get_blk() and friends into a separate file that isn't linked in the
storage daemon and removing blk_by_qdev_id() from the stubs so that the
build fails rather than just getting an error message back from the
QMP command that makes no sense in the storage daemon.

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

Right, and when the only actual problem is a hypothetical corner case
that can be worked around relatively easily (array types).

Kevin
Markus Armbruster Nov. 13, 2019, 1:53 p.m. UTC | #7
Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.11.2019 um 15:25 hat Markus Armbruster geschrieben:
>> 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?
>
> Looks like it.
>
>> >  #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?
>
> Possibly. Are you telling me to do it or is it just an idea for a later
> cleanup?

Exploring the idea would be nice, but it's definitely not required.

>> > +
>> >  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.
>
> Hm, I guess not. I'm just tweaking stuff until it seems to work, and
> this seemed to work, so...
>
> But as far as I can see, the generated file are never actually used, so
> I'll get rid of them.
>
>> > +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.
>
> Ok.
>
>> 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.
>
> That's why I asked you before writing it like this. :-)
>
>> 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.
>
> Maybe you'd like it better if we made it explicit?
>
>     { 'include': '../../qapi/block.json', 'gen': false }

Maybe.

Another possibility is tightening the stated assumption: "sub-modules
from outside the main module's directory are assumed to be shared, and
no code will be generated for them".  Needs to be documented in
docs/devel/qapi-code-gen.txt.  Which so far neglects to explain modules
properly.  Perhaps I can fix that in parallel, and then we figure out
how to proceed.

> But how do I actually get this information down to QAPIGen? This brings
> us back to the problem that the QAPI generator doesn't really have a
> notion of modules, but only entities have a module name and the first
> entity to use a module name causes it to be generated. We don't have
> access to the QAPISchemaInclude that created the module.
>
> Even the QAPISchema._modules that I introduced only contains filenames.
> Should it contain QAPISchemaInclude objects instead?

The old Python story: it's just a string; oh, it's two, so make it
tuple; oh, this is confusing, so make it a dict; oh, want methods, so
make it an object.

Time for a QAPISchemaModule object, I guess.  I'll have a try and get
back to you.

>> 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.
>
> A comment in the storage daemon main module?

Suffices as long as module sharing is a hack we use in the storage
daemon's QAPI schema.  If it spreads elsewhere, we better cover it in
docs/devel/qapi-code-gen.txt.

However, it looks like module sharing needs a bit of help from the QAPI
generator, which amounts to a schema language tweak, which means we need
to update qapi-code-gen.txt for it.  Depending on the nature of the
tweak, we may want to cover the array restriction in its doc update.

No need to worry about the doc details until we've figured out what to
do in the QAPI generator.

A comment in the storage daemon's main module won't hurt regardless.

>> 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?
>
> I guess we could by artificially generating to a sub-sub-directory, so
> that ../../ relative to the output directory specified with -o is still
> a directory specific to the storage daemon. Not very nice, though.
>
> It also results in (hopefully!) unused files with duplicate file names
> that are supposed to be mostly the same, modulo bugs.
>
>> 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.
>
> Not simple at all: What do you mean by "use"? For linking? For compiling
> qemu-storage-daemon.c? For compiling blockdev.c?
>
> The latter would mean that we compile everything that uses QAPI types
> twice. Considering that we're trying to get rid of per-target
> compilation as much as we can, it doesn't appear desirable to start
> compiling shared files multiple times for the storage daemon. (I think
> you only meant the generated files, not everything that uses QAPI types,
> when you said "we compile more"?)
>
> Only compiling a single file with the headers generated from the storage
> daemon schema doesn't sound like a great idea either. The storage daemon
> headers _should_ be a subset of the main schema ones, but if there is
> ever a bug and data structures differ, I don't want to be the poor guy
> to debug this. And anyway - you can compile against different versions
> of the header, but you can link in only one version of the generated .c
> files. I'm not sure what to call this, but "robust" is probably not in
> the selection.

You're right.

>> 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.
>
> I think we can live with this restriction. If we ever need an array
> type only in the storage daemon,

The guy staring at the gcc error won't be nearly as poor as the one
debugging diverged generated QAPI code, but I'd prefer not to be him
all the same.  Anyway:

>                                  we can still define the type that uses
> it in the main schema, but leave it unused there.

Yes, that's workable.

>> > 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).
>
> Can possibly be solved by making it explicit.

Needs a hopefully smaller amount of less 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.
>
> I disagree on this one.

I wasn't sure when I wrote it (thus "might"), and you've now convinced
me it's not workable.

>> * 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.
>
> Maybe we can have a bit of a protection against this by moving
> qmp_get_blk() and friends into a separate file that isn't linked in the
> storage daemon and removing blk_by_qdev_id() from the stubs so that the
> build fails rather than just getting an error message back from the
> QMP command that makes no sense in the storage daemon.

Yes, please.

>> 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 figure the code change wouldn't be so bad, but the documentation
update to go with it puts me off.

> Right, and when the only actual problem is a hypothetical corner case
> that can be worked around relatively easily (array types).

I consider documenting the hack also an actual problem, albeit a
solvable one.

Patch
diff mbox series

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"
 #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"
+
 #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,
 };
@@ -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);
+}
+
 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;
@@ -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
+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
+
 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 $<, \
+		"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:
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