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
