[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH RFC 00/48] Convert device_add to QObject / QErro
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH RFC 00/48] Convert device_add to QObject / QError |
Date: |
Thu, 25 Feb 2010 13:59:09 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
On Wed, Feb 24, 2010 at 06:55:12PM +0100, Markus Armbruster wrote:
> Why this is such a big job? There are two issues with a naive
> conversion:
>
> * 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.
>
> 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
> opinion.
>
> 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
> interfaces.
>
> 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 split this part off into its own patch series if that helps.
> However, the patches before it aren't all that useful without it, so
> I'm not sure splitting buys us much.
>
> A possible alternative is to add the concept of optional named
> arguments to the monitor. Instead of encoding multiple optional
> named arguments in a single positional argument, we encode them as
> multiple named arguments. For instance, "device_add
> ide-drive,drive=hda,bus=ide.0,unit=0" becomes "device_add ide-drive
> drive=hda bus=ide.0 unit=0".
>
> Of course, if you think that adding a second encoding for
> collections of name/value pairs to QMP is fine, then this last part
> can be dropped.
>
> So, the series starts with error locations (part I), and ends with
> keeping QemuOpts out of QMP (part III). Wedged in between is the
> conversion of device_add to QError (part II). In more detail:
>
> Part I: Error Locations
>
> [01-07] Preliminary cleanup & fixes
> [08] Separate "default monitor" and "current monitor" cleanly
> [09-16] More cleanup
> [17-21] Error Locations
>
> Part II: Convert device_add to QError
>
> [22-25] Preliminary qdev cleanup & fixes
> [26-42] Convert device_add to QError
>
> Part III
>
> [43] Conversions between QDict and QemuOpts
> [44-46] New monitor argument type O
> [47-48] Convert device_add to QObject
>
> I cut a few corners clearly marked in commit messages and code. I'll
> fix them up for the non-RFC submit.
I only read the patches in parts I and II, these look very good.
There are some FIXME's there, but you know that yourself.
--
MST
- [Qemu-devel] [PATCH RFC 36/48] error: New error_printf_unless_qmp(), (continued)
- [Qemu-devel] [PATCH RFC 36/48] error: New error_printf_unless_qmp(), Markus Armbruster, 2010/02/24
- [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 <=
- Re: [Qemu-devel] [PATCH RFC 00/48] Convert device_add to QObject / QError, Luiz Capitulino, 2010/02/26