diff mbox

[v3,12/15] monitor: use qmp_dispatch()

Message ID 20160808141439.16908-13-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Aug. 8, 2016, 2:14 p.m. UTC
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>
---
 monitor.c    | 326 +++++++----------------------------------------------------
 trace-events |   1 -
 2 files changed, 34 insertions(+), 293 deletions(-)

Comments

Markus Armbruster Aug. 9, 2016, 12:43 p.m. UTC | #1
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?
Daniel P. Berrangé Aug. 9, 2016, 12:48 p.m. UTC | #2
On Tue, Aug 09, 2016 at 02:43:41PM +0200, Markus Armbruster wrote:
> 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?

Libvirt doesn't use  query-qmp-schema at all - we just blindly invoke
the commands and catch error. That said I agree with your point though.
It seems pretty undesirable for query-qmp-schema to report that the
commands exist when they clearly don't really exist in any usable
manner.

Regards,
Daniel
Marc-Andre Lureau Aug. 9, 2016, 12:50 p.m. UTC | #3
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?
Markus Armbruster Aug. 9, 2016, 2:29 p.m. UTC | #4
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.

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

Feels cleaner, but to avoid -Wundef warnings, we'd have to say
'defined(CONFIG_SPICE)'.

Thoughts?
Marc-Andre Lureau Aug. 9, 2016, 2:41 p.m. UTC | #5
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.
Markus Armbruster Aug. 9, 2016, 4:27 p.m. UTC | #6
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.
Marc-André Lureau Aug. 9, 2016, 7:35 p.m. UTC | #7
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.
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?
Markus Armbruster Aug. 10, 2016, 10:17 a.m. UTC | #8
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"?

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

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

I'm not sure I understand this proposed solution.  qmp_query_uuid()
doesn't "go through QMP" in any sense I'd use.

The (QAPIfied) QMP command handlers are intended to be "natural" C
interfaces.  Their only QMPish part is their use of QAPI types.

For instance, qmp_query_uuid() is almost the obvious C function to
convert the binary qemu_uuid to a string.  "Almost", because the string
is wrapped in a struct, probably due to the rule "don't return simple
types, because that's not extensible".

If you now pointed out that the chance we'll extend UUIDs is remote,
you'd have a point.  But what's done is done.

If the struct wrapping gets bothersome, we can factor out the UUID
formatting into its own function.

> - 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?
Marc-André Lureau Aug. 10, 2016, 3:28 p.m. UTC | #9
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.
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index 5e7ae21..7be4e22 100644
--- a/monitor.c
+++ b/monitor.c
@@ -166,7 +166,6 @@  struct MonFdset {
 };
 
 typedef struct {
-    QObject *id;
     JSONMessageParser parser;
     /*
      * When a client connects, we're in capabilities negotiation mode.
@@ -229,8 +228,6 @@  static int mon_refcount;
 static mon_cmd_t mon_cmds[];
 static mon_cmd_t info_cmds[];
 
-static const mon_cmd_t qmp_cmds[];
-
 Monitor *cur_mon;
 
 static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
@@ -401,49 +398,6 @@  static void monitor_json_emitter(Monitor *mon, const QObject *data)
     QDECREF(json);
 }
 
-static QDict *build_qmp_error_dict(Error *err)
-{
-    QObject *obj;
-
-    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
-                             QapiErrorClass_lookup[error_get_class(err)],
-                             error_get_pretty(err));
-
-    return qobject_to_qdict(obj);
-}
-
-static void monitor_protocol_emitter(Monitor *mon, QObject *data,
-                                     Error *err)
-{
-    QDict *qmp;
-
-    trace_monitor_protocol_emitter(mon);
-
-    if (!err) {
-        /* success response */
-        qmp = qdict_new();
-        if (data) {
-            qobject_incref(data);
-            qdict_put_obj(qmp, "return", data);
-        } else {
-            /* return an empty QDict by default */
-            qdict_put(qmp, "return", qdict_new());
-        }
-    } else {
-        /* error response */
-        qmp = build_qmp_error_dict(err);
-    }
-
-    if (mon->qmp.id) {
-        qdict_put_obj(qmp, "id", mon->qmp.id);
-        mon->qmp.id = NULL;
-    }
-
-    monitor_json_emitter(mon, QOBJECT(qmp));
-    QDECREF(qmp);
-}
-
-
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
@@ -2169,11 +2123,6 @@  static mon_cmd_t mon_cmds[] = {
     { NULL, NULL, },
 };
 
-static const mon_cmd_t qmp_cmds[] = {
-#include "qmp-commands-old.h"
-    { /* NULL */ },
-};
-
 /*******************************************************************/
 
 static const char *pch;
@@ -2524,11 +2473,6 @@  static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
     return NULL;
 }
 
-static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
-{
-    return search_dispatch_table(qmp_cmds, cmdname);
-}
-
 /*
  * Parse command name from @cmdp according to command table @table.
  * If blank, return NULL.
@@ -3679,199 +3623,6 @@  static bool invalid_qmp_mode(const Monitor *mon, const char *cmd,
     return false;
 }
 
-/*
- * Argument validation rules:
- *
- * 1. The argument must exist in cmd_args qdict
- * 2. The argument type must be the expected one
- *
- * Special case: If the argument doesn't exist in cmd_args and
- *               the QMP_ACCEPT_UNKNOWNS flag is set, then the
- *               checking is skipped for it.
- */
-static void check_client_args_type(const QDict *client_args,
-                                   const QDict *cmd_args, int flags,
-                                   Error **errp)
-{
-    const QDictEntry *ent;
-
-    for (ent = qdict_first(client_args); ent;ent = qdict_next(client_args,ent)){
-        QObject *obj;
-        QString *arg_type;
-        const QObject *client_arg = qdict_entry_value(ent);
-        const char *client_arg_name = qdict_entry_key(ent);
-
-        obj = qdict_get(cmd_args, client_arg_name);
-        if (!obj) {
-            if (flags & QMP_ACCEPT_UNKNOWNS) {
-                /* handler accepts unknowns */
-                continue;
-            }
-            /* client arg doesn't exist */
-            error_setg(errp, QERR_INVALID_PARAMETER, client_arg_name);
-            return;
-        }
-
-        arg_type = qobject_to_qstring(obj);
-        assert(arg_type != NULL);
-
-        /* check if argument's type is correct */
-        switch (qstring_get_str(arg_type)[0]) {
-        case 'F':
-        case 'B':
-        case 's':
-            if (qobject_type(client_arg) != QTYPE_QSTRING) {
-                error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                           client_arg_name, "string");
-                return;
-            }
-        break;
-        case 'i':
-        case 'l':
-        case 'M':
-        case 'o':
-            if (qobject_type(client_arg) != QTYPE_QINT) {
-                error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                           client_arg_name, "int");
-                return;
-            }
-            break;
-        case 'T':
-            if (qobject_type(client_arg) != QTYPE_QINT &&
-                qobject_type(client_arg) != QTYPE_QFLOAT) {
-                error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                           client_arg_name, "number");
-                return;
-            }
-            break;
-        case 'b':
-        case '-':
-            if (qobject_type(client_arg) != QTYPE_QBOOL) {
-                error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                           client_arg_name, "bool");
-                return;
-            }
-            break;
-        case 'O':
-            assert(flags & QMP_ACCEPT_UNKNOWNS);
-            break;
-        case 'q':
-            /* Any QObject can be passed.  */
-            break;
-        case '/':
-        case '.':
-            /*
-             * These types are not supported by QMP and thus are not
-             * handled here. Fall through.
-             */
-        default:
-            abort();
-        }
-    }
-}
-
-/*
- * - Check if the client has passed all mandatory args
- * - Set special flags for argument validation
- */
-static void check_mandatory_args(const QDict *cmd_args,
-                                 const QDict *client_args, int *flags,
-                                 Error **errp)
-{
-    const QDictEntry *ent;
-
-    for (ent = qdict_first(cmd_args); ent; ent = qdict_next(cmd_args, ent)) {
-        const char *cmd_arg_name = qdict_entry_key(ent);
-        QString *type = qobject_to_qstring(qdict_entry_value(ent));
-        assert(type != NULL);
-
-        if (qstring_get_str(type)[0] == 'O') {
-            assert((*flags & QMP_ACCEPT_UNKNOWNS) == 0);
-            *flags |= QMP_ACCEPT_UNKNOWNS;
-        } else if (qstring_get_str(type)[0] != '-' &&
-                   qstring_get_str(type)[1] != '?' &&
-                   !qdict_haskey(client_args, cmd_arg_name)) {
-            error_setg(errp, QERR_MISSING_PARAMETER, cmd_arg_name);
-            return;
-        }
-    }
-}
-
-static QDict *qdict_from_args_type(const char *args_type)
-{
-    int i;
-    QDict *qdict;
-    QString *key, *type, *cur_qs;
-
-    assert(args_type != NULL);
-
-    qdict = qdict_new();
-
-    if (args_type == NULL || args_type[0] == '\0') {
-        /* no args, empty qdict */
-        goto out;
-    }
-
-    key = qstring_new();
-    type = qstring_new();
-
-    cur_qs = key;
-
-    for (i = 0;; i++) {
-        switch (args_type[i]) {
-            case ',':
-            case '\0':
-                qdict_put(qdict, qstring_get_str(key), type);
-                QDECREF(key);
-                if (args_type[i] == '\0') {
-                    goto out;
-                }
-                type = qstring_new(); /* qdict has ref */
-                cur_qs = key = qstring_new();
-                break;
-            case ':':
-                cur_qs = type;
-                break;
-            default:
-                qstring_append_chr(cur_qs, args_type[i]);
-                break;
-        }
-    }
-
-out:
-    return qdict;
-}
-
-/*
- * Client argument checking rules:
- *
- * 1. Client must provide all mandatory arguments
- * 2. Each argument provided by the client must be expected
- * 3. Each argument provided by the client must have the type expected
- *    by the command
- */
-static void qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args,
-                                  Error **errp)
-{
-    Error *err = NULL;
-    int flags;
-    QDict *cmd_args;
-
-    cmd_args = qdict_from_args_type(cmd->args_type);
-
-    flags = 0;
-    check_mandatory_args(cmd_args, client_args, &flags, &err);
-    if (err) {
-        goto out;
-    }
-
-    check_client_args_type(client_args, cmd_args, flags, &err);
-
-out:
-    error_propagate(errp, err);
-    QDECREF(cmd_args);
-}
-
 /*
  * Input object checking rules
  *
@@ -3930,67 +3681,58 @@  static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp)
 
 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
-    Error *local_err = NULL;
-    QObject *obj, *data;
-    QDict *input, *args;
-    const mon_cmd_t *cmd;
-    QmpCommand *qcmd;
+    QObject *req, *rsp = NULL, *id = NULL;
+    QDict *qdict = NULL;
     const char *cmd_name;
     Monitor *mon = cur_mon;
+    Error *err = NULL;
 
-    args = input = NULL;
-    data = NULL;
-
-    obj = json_parser_parse(tokens, NULL);
-    if (!obj) {
-        // FIXME: should be triggered in json_parser_parse()
-        error_setg(&local_err, QERR_JSON_PARSING);
+    req = json_parser_parse_err(tokens, NULL, &err);
+    if (err || !req || qobject_type(req) != QTYPE_QDICT) {
+        if (!err) {
+            error_setg(&err, QERR_JSON_PARSING);
+        }
         goto err_out;
     }
 
-    input = qmp_check_input_obj(obj, &local_err);
-    if (!input) {
-        qobject_decref(obj);
+    qdict = qmp_check_input_obj(req, &err);
+    if (!qdict) {
         goto err_out;
     }
 
-    mon->qmp.id = qdict_get(input, "id");
-    qobject_incref(mon->qmp.id);
+    id = qdict_get(qdict, "id");
+    qobject_incref(id);
+    qdict_del(qdict, "id");
 
-    cmd_name = qdict_get_str(input, "execute");
+    cmd_name = qdict_get_str(qdict, "execute");
     trace_handle_qmp_command(mon, cmd_name);
-    cmd = qmp_find_cmd(cmd_name);
-    qcmd = qmp_find_command(cmd_name);
-    if (!qcmd || !cmd) {
-        error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
-                  "The command %s has not been found", cmd_name);
-        goto err_out;
-    }
-    if (invalid_qmp_mode(mon, cmd_name, &local_err)) {
+
+    if (invalid_qmp_mode(mon, cmd_name, &err)) {
         goto err_out;
     }
 
-    obj = qdict_get(input, "arguments");
-    if (!obj) {
-        args = qdict_new();
-    } else {
-        args = qobject_to_qdict(obj);
-        QINCREF(args);
-    }
+    rsp = qmp_dispatch(req);
 
-    qmp_check_client_args(cmd, args, &local_err);
-    if (local_err) {
-        goto err_out;
+err_out:
+    if (err) {
+        qdict = qdict_new();
+        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
+        error_free(err);
+        rsp = QOBJECT(qdict);
     }
 
-    qcmd->fn(args, &data, &local_err);
+    if (rsp) {
+        if (id) {
+            qdict_put_obj(qobject_to_qdict(rsp), "id", id);
+            id = NULL;
+        }
 
-err_out:
-    monitor_protocol_emitter(mon, data, local_err);
-    qobject_decref(data);
-    error_free(local_err);
-    QDECREF(input);
-    QDECREF(args);
+        monitor_json_emitter(mon, rsp);
+    }
+
+    qobject_decref(id);
+    qobject_decref(rsp);
+    qobject_decref(req);
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
diff --git a/trace-events b/trace-events
index 52c6a6c..cdc8653 100644
--- a/trace-events
+++ b/trace-events
@@ -97,7 +97,6 @@  qemu_co_mutex_unlock_return(void *mutex, void *self) "mutex %p self %p"
 
 # monitor.c
 handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
-monitor_protocol_emitter(void *mon) "mon %p"
 monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
 monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
 monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64