[Qemu-devel] Re: [PATCH RFC 00/48] Convert device_add to QObject / QError

["Michael S. Tsirkin" <mst@redhat.com>]

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