[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option
|
From: |
Kevin Wolf |
|
Subject: |
Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option |
|
Date: |
Wed, 13 Nov 2019 11:58:58 +0100 |
|
User-agent: |
Mutt/1.12.1 (2019-06-15) |
Am 12.11.2019 um 15:25 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> 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 <address@hidden>
> > ---
> > 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