Re: [Qemu-devel] [PATCH v3 12/15] monitor: use qmp_dispatch()

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Wed, Aug 10, 2016 at 2:17 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Tue, Aug 9, 2016 at 9:35 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Marc-André Lureau <mlureau@redhat.com> writes:
> >>
> >> > Hi
> >> >
> >> > ----- Original Message -----
> >> >> Marc-André Lureau <mlureau@redhat.com> writes:
> >> >>
> >> >> > Hi
> >> >> >
> >> >> > ----- Original Message -----
> >> >> >> marcandre.lureau@redhat.com writes:
> >> >> >>
> >> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >> >
> >> >> >> > Replace the old manual dispatch and validation code by the
> generic
> >> one
> >> >> >> > provided by qapi common code.
> >> >> >> >
> >> >> >> > Note that it is now possible to call the following commands that
> >> used to
> >> >> >> > be disabled by compile-time conditionals:
> >> >> >> > - dump-skeys
> >> >> >> > - query-spice
> >> >> >> > - rtc-reset-reinjection
> >> >> >> > - query-gic-capabilities
> >> >> >> >
> >> >> >> > Their fallback functions return an appropriate "feature
> disabled"
> >> error.
> >> >> >> >
> >> >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >>
> >> >> >> Means query-qmp-schema no longer shows whether these commands are
> >> >> >> supported, doesn't it?
> >> >> >>
> >> >> >> Eric, could this create difficulties for libvirt or other
> >> introspection
> >> >> >> users?
> >> >> >
> >> >> > Thinking a bit about this, I guess it would be fairly
> straightforward
> >> >> > to have a new key "c-conditional" : "#ifdef CONFIG_SPICE" that
> would
> >> >> > prepend it in C generated files, with a corresponding "#endif".
> Would
> >> >> > that be acceptable?
> >> >>
> >> >> Not exactly pretty, but the only alternative I can think of right now
> >> >> would be conditional qapi generation, i.e. something like
> >> >>
> >> >> { 'if': 'CONFIG_SPICE'
> >> >>   'then': { 'command': 'query-spice', 'returns': 'SpiceInfo' } }
> >> >>
> >> >> More general, but *much* more work.  Let's not go there now.
> >> >
> >> > That looks quite unnecessarily complicated to me, and not so
> declarative.
> >> >
> >> >>
> >> >> The value of key 'c-conditional' must be a preprocessing directive
> that
> >> >> pairs with #endif.  Hmm.
> >> >>
> >> >> Could make it an expression instead, and call the key just
> >> >> 'conditional'.  If given, wrap the code generated for the QAPI
> >> >> definition in
> >> >>
> >> >>     #if <value of conditional>
> >> >>     ...
> >> >>     #endif
> >> >>
> >> >
> >> > Sure, we could make it a preprocessor expression instead, so it would
> >> have to match with the automatically appened "#endif".
> >> >
> >> >> Feels cleaner, but to avoid -Wundef warnings, we'd have to say
> >> >> 'defined(CONFIG_SPICE)'.
> >> >
> >> > Yes, why not? I can work on a patch see how well it fits.
> >>
> >> Yes, please.
> >>
> >
> > After spending some time on this (the generator part is trivial), I think
> > it's not going to be that easy because the conditions are per-target, but
> > qmp is not, so you get poisoned defines errors with the TARGET
> conditions.
>
> Ah, yes.  monitor.c is target-specific for similarly annoying reasons.
> Factoring out the parts that are actually target-specific into a
> separate file would be nice.
>
> > We can't easily make qmp per target, as the code is spread everywhere
> (even
>
> What you mean by "qmp" and ...
>
> > though such qmp code won't be useful for other tools, it would be nice to
> > untangle this - the block code is full of qmp usage and implementation)
>
> ... "qmp usage and implementation"?
>

Building and linking with the generated qapi/qmp code (even though they may
not actually use it..)

>
> > Furthermore, the current query-qmp-schema returns all commands currently,
> > so this won't be a regression.
>
> It returns qmp_schema_json[], generated by qapi-introspect.py.
>
>
Right so it returns everything from the schema. The difference is that with
this series, query-commands will now return everything from the schema. And
the command will return feature-disabled error. Is that an acceptable
regression? or should it be fixed before that series?


> > - unregister functions dynamically? (that could be useful for other
> reasons)
> > - make only qmp_init_marshal() and qmp_introspect() per target.
> >
> > That last option seems the easier. What do you think?
>
> Let me try to elaborate the last option to make sure we're on the same
> page.
>
> Key 'conditional' makes the QAPI generators generate preprocessing
> directives for conditional compilation.  Only commands have this key, at
> least for now.  The preprocessing directives should wrap "everything"
> belonging to the command:
>
> * Command handler declaration in qmp-commands.h
>
>   Problem: this header gets included widely, and we really don't want to
>   make all the .c files including it target-dependent.
>
>   Howeve, since a declaration without an implementation is not an error,
>   we can blindly generate the declarations for now, along with a TODO
>   comment explaining the uncleanliness.
>
> * Command marshaller declaration in qmp-commands.h
>
>   Likewise.
>
> * Command marshaller definition in qmp-marshal.c
>
>   Can make target-dependent.  It's a big file, though.  Perhaps we can
>   split it into a target-dependent and a target-independent part some
>   day.
>
> * Command marshaller use in qmp-marshal.c's qmp_init_marshal()
>
>   Likewise.
>
> * Command introspection data in qmp-introspect.c.
>
>   Can make target-dependent.  Splitting it up doesn't look worth the
>   bother.
>
> Is this basically what you're proposing?
>

Yes, I got it working. But then I started to wonder about the rest of the
events & data types, and I figured it would be probably best  handled by a
c preprocessor (so you could have large #if blocks instead of plenty of
'c-conditional' keys, and a generated file with lot of #ifs that could have
been not there at generation time). Unfortunately, our json files aren't
really 'json', and in particular our #-shellish comments do not play nice
with the C preprocessor.

I don't really fancy changing all the doc format right now, or fixing the
json usage (to be a valid json), since I have a lot of doc patches after,
that would conflict a lot! And it's probably best to automate the move once
all the doc is at the same place..

So I wonder if it's acceptable to have query-commands return commands that
return feature-disabled errors. After all, the command exists, and it
returns a 'valid' reply. Alternatively, I think we could have the commands
removed at run-time for now until we fix this at compile time after the doc
series is merged.
-- 
Marc-André Lureau