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

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 24 Feb 2010 18:55:12 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Why this is such a big job?  There are two issues with a naive
>> conversion:
>> 
>> * Error message degradation
[...]
>> * 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'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.

Conceptually, device_add is as simple as it gets: add a device.  Any
device.  With properties configured.  Any properties.

>                                We didn't expose 'info', for example, because
> having a command (or 'remote procedure') returning all sorts of possible
> data is no good.

Why?  What's the fundamental difference between having to enumerate all
the possible arguments of "info" and their replies, and having to
enumerate all the possible FOOs valid with "query-FOO", and their
replies?

For what it's worth, "info" returns much richer data than device_add.

>  We mapped each 'info foo' command to a 'query' command instead. This is also
> natural for info commands, as they implemented separately.

Fine with me :)

>  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
> properties.
>
>  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.

Indeed.

>  Two questions:
>
> 1. Don't we need a 'query-devices' command?

There's "info qdm".  It doesn't show properties, but it could.  Its
output is already plenty long, so for humans we better provide a way to
ask for a single driver's properties.

There's also "device_add ?" and "device_add DRIVER,?", but that's for
humans.  For QMP, we want to keep queries and actions neatly separate.

> 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)

We want command self-documentation to enumerate all accepted parameters.
For device_add, I'd make it say "accepts any".  That's a lie, as it
really accepts only device properties.  Thus, Clients can't learn device
properties from the device_add command, they need to query the device
model.  I'm fine with that.