2:04 p.m.
Hi,
When adding new QMP commands that are implemented by
arch-specific code, we have been adding stubs that report
QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
an example).
But we are using GenericError for that, and this prevents clients
from reliably checking if the command is really implemented by
the QEMU binary.
What should be the right solution for this? Some of the options I
have considered are:
1) Using CommandNotFound as the error class in the stubs. This
sounds wrong because the command exists (it is present in
query-commands and in the QAPI schema).
2) Creating a CommandNotImplemented error class. Simple to do,
but it would require clients to make two separate checks,
before concluding that the command is available (checking
query-commands or query-qmp-schema, and then checking for
CommandNotImplemented errors).
3.1) Removing the command from query-commands and from the QAPI
schema on binaries that don't implement the command.
Needlessly complex?
3.2) Removing the unimplemented command from query-commands only
(by calling qmp_disable_command()), but keeping it on the QAPI
schema. I am not sure it's OK to do that. If it is, this
sounds like the simplest solution.
Any ideas?
--
Eduardo
2:53 p.m.
On Mon, Oct 03, 2016 at 16:04:42 -0300, Eduardo Habkost wrote:
Hi,
When adding new QMP commands that are implemented by
arch-specific code, we have been adding stubs that report
QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
an example).
But we are using GenericError for that, and this prevents clients
from reliably checking if the command is really implemented by
the QEMU binary.
What should be the right solution for this? Some of the options I
have considered are:
1) Using CommandNotFound as the error class in the stubs. This
sounds wrong because the command exists (it is present in
query-commands and in the QAPI schema).
2) Creating a CommandNotImplemented error class. Simple to do,
but it would require clients to make two separate checks,
before concluding that the command is available (checking
query-commands or query-qmp-schema, and then checking for
CommandNotImplemented errors).
Both options require an extra step to check whether a particular command
is implemented or not. It would be highly appreciated if we didn't have
to go this way since it would seriously complicate the probing process.
We'd need to run each command with some artificial, but sane enough
arguments to bypass arguments checking code:
(QEMU) query-cpu-model-expansion type=full
{"error": {"class": "GenericError", "desc":
"Invalid parameter type for
'model', expected: QDict"}}
(QEMU) query-cpu-model-expansion type=full model={'name':'host'}
{"error": {"class": "GenericError", "desc":
"this feature or command is
not currently supported"}}
(QEMU) query-cpu-model-expansion type=full model={'name':'Penryn'}
{"error": {"class": "GenericError", "desc":
"this feature or command is
not currently supported"}}
3.1) Removing the command from query-commands and from the QAPI
schema on binaries that don't implement the command.
Needlessly complex?
3.2) Removing the unimplemented command from query-commands only
(by calling qmp_disable_command()), but keeping it on the QAPI
schema. I am not sure it's OK to do that. If it is, this
sounds like the simplest solution.
These options are both acceptable to libvirt. So it really depends what
is acceptable to QEMU...
From my POV (which is quite ignorant to QEMU internals in this area),
there are a few other options:
- implementing a new QMP command to list unsupported commands
(e.g., query-unsupported-commands)
- adding a flag to qmp-schema which would indicate whether a command is
supported or not
- even a new stupid command that would take another command as an
argument and return whether it's supported or not would be better than
having to run each command individually with special arguments
I'm not saying these options are clean or doable, I'm just brainstorming
here.
Jirka
3:15 p.m.
On 10/03/2016 02:04 PM, Eduardo Habkost wrote:
Hi,
When adding new QMP commands that are implemented by
arch-specific code, we have been adding stubs that report
QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
an example).
3.1) Removing the command from query-commands and from the QAPI
schema on binaries that don't implement the command.
Needlessly complex?
Ideal goal, but we aren't there yet.
3.2) Removing the unimplemented command from query-commands only
(by calling qmp_disable_command()), but keeping it on the QAPI
schema. I am not sure it's OK to do that. If it is, this
sounds like the simplest solution.
We already have existing commands in this category, and it is
conceptually the easiest (if query-commands doesn't list the command,
then the command doesn't work even if the introspection still shows it).
In fact, that was part of the reason that Marc-Andre's work to remove
middle mode took so many revisions, because we were figuring out if it
was possible to get to the ideal option 3.1 (answer was not yet), then
deciding what the least disgusting hack was for sticking with option 3.2
after we dropped qemu-commands.hx (the hack currently in place, as of
qemu commit 5032a16d, is that all commands are now unconditionally
generated, but the tricky ones are now explicitly unregistered in C code
guarded by negative #ifdefs for the platforms where the command used to
be only conditionally generated from positive ifdefs in the .hx file, so
that the query-commands result is identical).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:22 p.m.
On Mon, Oct 03, 2016 at 03:15:39PM -0500, Eric Blake wrote:
[...]
> 3.2) Removing the unimplemented command from query-commands
only
> (by calling qmp_disable_command()), but keeping it on the QAPI
> schema. I am not sure it's OK to do that. If it is, this
> sounds like the simplest solution.
We already have existing commands in this category, and it is
conceptually the easiest (if query-commands doesn't list the command,
then the command doesn't work even if the introspection still shows it).
[...]
Which commands are already in this category? I don't see any
qmp_disable_command() calls on the tree except for guest agent
code.
--
Eduardo
3:57 p.m.
On 10/04/2016 01:22 PM, Eduardo Habkost wrote:
On Mon, Oct 03, 2016 at 03:15:39PM -0500, Eric Blake wrote:
[...]
>> 3.2) Removing the unimplemented command from query-commands only
>> (by calling qmp_disable_command()), but keeping it on the QAPI
>> schema. I am not sure it's OK to do that. If it is, this
>> sounds like the simplest solution.
>
> We already have existing commands in this category, and it is
> conceptually the easiest (if query-commands doesn't list the command,
> then the command doesn't work even if the introspection still shows it).
[...]
Which commands are already in this category? I don't see any
qmp_disable_command() calls on the tree except for guest agent
code.
Probably a typo on my side; commit 5032a16d adds the use of
qmp_unregister_command(), not qmp_disable_command().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:52 p.m.
On Tue, Oct 04, 2016 at 03:57:10PM -0500, Eric Blake wrote:
On 10/04/2016 01:22 PM, Eduardo Habkost wrote:
> On Mon, Oct 03, 2016 at 03:15:39PM -0500, Eric Blake wrote:
> [...]
>>> 3.2) Removing the unimplemented command from query-commands only
>>> (by calling qmp_disable_command()), but keeping it on the QAPI
>>> schema. I am not sure it's OK to do that. If it is, this
>>> sounds like the simplest solution.
>>
>> We already have existing commands in this category, and it is
>> conceptually the easiest (if query-commands doesn't list the command,
>> then the command doesn't work even if the introspection still shows it).
> [...]
>
> Which commands are already in this category? I don't see any
> qmp_disable_command() calls on the tree except for guest agent
> code.
Probably a typo on my side; commit 5032a16d adds the use of
qmp_unregister_command(), not qmp_disable_command().
Thanks for the pointer. I will try to move the query-cpu-* hack
there.
--
Eduardo
3004
days inactive
3006
days old
5 comments
3 participants
participants (3)
-
Eduardo Habkost -
Eric Blake -
Jiri Denemark