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

From: Kevin Wolf <kwolf@redhat.com>
To: Max Reitz <mreitz@redhat.com>
Cc: pkrempa@redhat.com, armbru@redhat.com, qemu-block@nongnu.org,
	qemu-devel@nongnu.org
Subject: Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option
Date: Thu, 7 Nov 2019 11:12:35 +0100	[thread overview]
Message-ID: <20191107101235.GB5007@linux.fritz.box> (raw)
In-Reply-To: <64d79019-711d-8eb1-da72-a9b95f999ff5@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]