Re: [Qemu-devel] KVM call minutes for Feb 8

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/09/2011 06:28 AM, Markus Armbruster wrote:
>> Except that construction of a device requires initialization from an
>> array of variants (which is then type checked).  The way we store the
>> variants is lossy because we convert back and forth to a string.
>>      
> Yes, there's overlap, but no, a qdev property isn't yet another variant
> type scheme.  Exhibit A of the defense: qdev uses QemuOpts for variant
> types.
>
> Let me elaborate.  qdev_device_add() uses QemuOpts as map from name to
> variant type value, uses the name to look up the property, then uses
> property methods to stuff the variant value it got from QemuOpts into
> the (non-variant) struct member described by the property.
>
> I figure QemuOpts was adopted for this purpose because it was already in
> use with command line and human monitor.  With QMP added to the mix,
> there's friction: QMP uses QDict, not QemuOpts.
>    

I'm going to finish QMP before tackling qdev, but at a high level, 
here's how I think we fix this.

Right now, qdev only really supports construction properties.  In 
GObject parlance, this would be properties with G_PARAM_CONSTRUCT_ONLY.

Instead of the current approach of having the construction properties 
automagically set as part of the object prior to initfn() being invoked, 
we should have an init function that takes the full set of construction 
only properties in the native type.

With a schema description of the device's constructor, we can generate 
code that invokes the native constructor based on a QemuOpts, or based 
on a QDict.

So instead of:

static int serial_isa_initfn(ISADevice *dev);

static ISADeviceInfo serial_isa_info = {
     .init       = serial_isa_initfn,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("index", ISASerialState, index,   -1),
         DEFINE_PROP_HEX32("iobase", ISASerialState, iobase,  -1),
         DEFINE_PROP_UINT32("irq",   ISASerialState, isairq,  -1),
         DEFINE_PROP_CHR("chardev",  ISASerialState, state.chr),
         DEFINE_PROP_END_OF_LIST(),
     },
};

We'd have:

void isa_serial_init(ISASerialState *obj, uint32_t index, uint32_t 
iobase, uint32_t irq, CharDriverState *chardev, Error **errp);

// isa_serial.json
[ 'ISASerialState', {'index': 'uint32_t', 'iobase': 'uint32_t', 'irq': 
'uint32_t', 'chardev': 'CharDriverState *'} ]

 From this definition, we can generate code that handles the -device 
argument doing conversion from string to the appropriate types while 
also doing QObject/GVariant conversion to support the qmp_device_add() 
interface.

Also, having a well typed constructor means that we can do safer 
composition because instead of doing:

DeviceState *dev;

dev = qdev_create(NULL, "isa-serial");
qdev_prop_set_uint32(dev, "iobase", 0x274);
qdev_prop_set_uint32(dev, "irq", 0x07);
qdev_init_nofail(dev);

We can just do:

ISASerialState dev;

isa_serial_init(&dev, 0, 0x274, 0x07, NULL, NULL);

I think one of the reasons qdev has not been thoroughly embraced in much 
of QEMU is that the interface is obnoxious to work with in C.  By 
addressing that, I think it'll be much more successful.

Regards,

Anthony Liguori