Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize

[Andreas Färber]

Hi,

Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <address@hidden> wrote:
>> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
>> index dc6f4e4..409d315 100644
>> --- a/hw/9pfs/virtio-9p-device.c
>> +++ b/hw/9pfs/virtio-9p-device.c
[...]
>> @@ -136,12 +138,16 @@ static Property virtio_9p_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> -static void virtio_9p_class_init(ObjectClass *klass, void *data)
>> +static void virtio_9p_class_init(ObjectClass *oc, void *data)
>>  {
>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>> -    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(oc);
>> +    V9fsClass *v9c = VIRTIO_9P_CLASS(oc);
>> +
>> +    v9c->parent_realize = dc->realize;
>> +    dc->realize = virtio_9p_device_realize;
>> +
>>      dc->props = virtio_9p_properties;
>> -    vdc->init = virtio_9p_device_init;
>>      vdc->get_features = virtio_9p_get_features;
>>      vdc->get_config = virtio_9p_get_config;
>>  }
>> @@ -151,6 +157,7 @@ static const TypeInfo virtio_device_info = {
>>      .parent = TYPE_VIRTIO_DEVICE,
>>      .instance_size = sizeof(V9fsState),
>>      .class_init = virtio_9p_class_init,
>> +    .class_size = sizeof(V9fsClass),
>>  };
>>
>>  static void virtio_9p_register_types(void)
>> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
>> index 1d6eedb..85699a7 100644
>> --- a/hw/9pfs/virtio-9p.h
>> +++ b/hw/9pfs/virtio-9p.h
>> @@ -227,6 +227,15 @@ typedef struct V9fsState
>>      V9fsConf fsconf;
>>  } V9fsState;
>>
>> +typedef struct V9fsClass {
>> +    /*< private >*/
>> +    VirtioDeviceClass parent_class;
>> +    /*< public >*/
>> +
>> +    DeviceRealize parent_realize;
>> +} V9fsClass;
>> +
>> +
> 
> If applied tree-wide this change pattern is going to add a lot of
> boiler-plate to all devices. There is capability in QOM to access the
> overridden parent class functions already, so it can be made to work
> without every class having to do this save-and-call trick with
> overridden realize (and friends). How about this:
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 9190a7e..696702a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,6 +37,18 @@ int qdev_hotplug = 0;
>  static bool qdev_hot_added = false;
>  static bool qdev_hot_removed = false;
> 
> +void device_parent_realize(DeviceState *dev, Error **errp)
> +{
> +    ObjectClass *class = object_get_class(dev);
> +    DeviceClass *dc;
> +
> +    class = object_class_get_parent(dc);
> +    assert(class);
> +    dc = DEVICE_CLASS(class);
> +
> +    dc->realize(dev, errp);
> +}
> +
> 
> And child class realize fns can call this to realize themselves as the
> parent would. Ditto for reset and unrealize. Then you would only need
> to define struct FooClass when creating new abstractions (or virtual
> functions if your C++).

Indeed, if you check the archives you will find that I suggested the
same in the context of ISA i8254/i8259 or CPUState. ;) Yet Anthony
specifically instructed me to do it this way, referring to GObject.
I then documented the expected process in qdev-core.h and object.h.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg