qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marc-André Lureau
Subject: Re: [Qemu-devel] [PATCH v3 12/15] monitor: use qmp_dispatch()
Date: Tue, 09 Aug 2016 19:35:52 +0000

Hi

On Tue, Aug 9, 2016 at 9:35 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> >
> > ----- Original Message -----
> >> Marc-André Lureau <address@hidden> writes:
> >>
> >> > Hi
> >> >
> >> > ----- Original Message -----
> >> >> address@hidden writes:
> >> >>
> >> >> > From: Marc-André Lureau <address@hidden>
> >> >> >
> >> >> > 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 <address@hidden>
> >> >>
> >> >> 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.
We can't easily make qmp per target, as the code is spread everywhere (even
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)

Furthermore, the current query-qmp-schema returns all commands currently,
so this won't be a regression.

I imagine 3 ways to solve this:
- make qmp code per-target (seems to be pretty intrusive change all over,
although I think it's nicer. As a simple ex, calling qmp_query_uuid() just
to get an uuid doesn't make much sense, it doesn't have to go through qmp)
- 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?


-- 
Marc-André Lureau


reply via email to

[Prev in Thread] Current Thread [Next in Thread]