[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 00/48] Convert device_add to QObject / QErro
Re: [Qemu-devel] [PATCH RFC 00/48] Convert device_add to QObject / QError
Fri, 26 Feb 2010 16:43:05 -0300
On Wed, 24 Feb 2010 18:55:12 +0100
Markus Armbruster <address@hidden> wrote:
> Why this is such a big job? There are two issues with a naive
> * Error message degradation
> The error messages are worded for -device. They aren't so hot to
> begin with: we typically have many -device options, and to which one
> a message applies is often not obvious.
> Now, QMP wants relatively generic errors. For instance, "-device:
> no driver specified" becomes "Parameter 'driver' is missing".
> Emitting such an error with our lengthy command lines is just too
> mean to users.
> However, if you know *where* the parameter is missing, the generic
> message is perfectly adequate: "-device a=b: Parameter 'driver' is
> missing". In fact, it's even superior to our current message.
> So the first part of the patch series is about error locations. I
> feel it's very useful all by itself. I can split it off into its
> own patch series. But then the rest of this series depends on it,
> so I'm not sure splitting is all that useful.
This looks reasonable to me.
When we started discussing possible solutions, we considered moving the
description table to its own module and supporting error message override.
The solution in this series is a bit simpler, but it does the job and
adding message override support later shouldn't be difficult, if needed.
> We may still encounter cases where a generic message is not adequate
> even with precise location information. Let's solve that problem
> when we actually encounter it.
> * String argument with option syntax, i.e. NAME=VALUE,...
> QMP uses JSON to encode collections of name/value pairs. Adding a
> second encoding for the same thing would be a mistake, in my
> Note that we already have two competing encodings in our code: QDict
> and QemuOpts. But we should not permit that to leak into an
> external interface like QMP.
> QemuOpts originated in the command line and spread from there into a
> few monitor commands, including device_add, and a few internal
> QDict originated in the monitor. It sits right at the interface
> between monitor core and command handlers.
> My proposed solution is modest and pragmatic:
> * Lift the parsing of arguments into QemuOpts from monitor handlers
> up into the human monitor core. This removes QemuOpts from the
> handler interface, and thus avoids leaking it into QMP. It's
> exactly what we did for other argument types with syntax
> inappropriate for QMP, such as arguments of migrate_set_speed and
> migrate_set_downtime (commits 9da92c49..b0fbf7d3).
> * Monitor handlers that need to pass their arguments in
> QemuOpts-form to internal interfaces use a converter function to
> translate from QDict to QemuOpts.
> This is what the last part of the patch series is about. If you'd
> prefer a different solution, let's talk.
I can't think of anything better, at least not for the short term.
However, I'm wondering if exposing a monster command like device_add is
the right thing to do for QMP. We didn't expose 'info', for example, because
having a command (or 'remote procedure') returning all sorts of possible
data is no good.
We mapped each 'info foo' command to a 'query' command instead. This is also
natural for info commands, as they implemented separately.
So, I'm wondering if the same argument applies for device_add, as we're
going to have a large number of devices, each of them with their own
The other possible solution is to introduce sets of commands to add
specific devices, but this might not be doable at all as we're moving
away from this approach with qdev...
We do need a way to add devices in the system through QMP ASAP, so we'll
have to use device_add, anyway.
1. Don't we need a 'query-devices' command?
2. How are we going to document all the accepted parameters? Note
that as we're exposing them through QMP, they obviusly must not change
(although this probably a requirement for the CLI as well)
- [Qemu-devel] [PATCH RFC 19/48] error: Track locations in configuration files, (continued)
- [Qemu-devel] [PATCH RFC 19/48] error: Track locations in configuration files, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 47/48] monitor: Use argument type 'O' for device_add, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 31/48] error: New QERR_BUS_NOT_FOUND, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 27/48] error: New QERR_PROPERTY_NOT_FOUND, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 29/48] qdev: convert setting device properties to QError, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 03/48] usb: Remove disabled monitor_printf() in usb_read_file(), Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 10/48] error: Simplify error sink setup, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 39/48] error: New QERR_DEVICE_INIT_FAILED, Markus Armbruster, 2010/02/24
- [Qemu-devel] [PATCH RFC 04/48] savevm: Fix -loadvm to report errors to stderr, not the monitor, Markus Armbruster, 2010/02/24
- [Qemu-devel] Re: [PATCH RFC 00/48] Convert device_add to QObject / QError, Michael S. Tsirkin, 2010/02/25
- Re: [Qemu-devel] [PATCH RFC 00/48] Convert device_add to QObject / QError,
Luiz Capitulino <=