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

[Blue Swirl]

On Wed, Feb 9, 2011 at 4:44 PM, Anthony Liguori <address@hidden> wrote:
> 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);

Do you mean that there should be a generic way of doing that, like
sysbus_create_varargs() for qdev, or just add inline functions which
hide qdev property setup?

I still think that FDT should be used in the future. That would
require that the properties can be set up mechanically, and I don't
see how your proposal would help that.