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

["Andreas Färber" <afaerber@suse.de>]

Am 10.06.2013 04:08, schrieb Anthony Liguori:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
>>>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaerber@suse.de> 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);
>>>> +}
> 
> What's weird about this is that you aren't necessarily calling
> Device::realize() here, you're really calling super()::realize().

We can certainly fix that by passing ObjectClass *oc argument instead of
DeviceState *dev and using object_class_get_parent(oc) - dc is used
uninitialized above.

> I really don't know whether it's a better approach or not.

It does save LOCs and should work with sane class_inits.

> Another option is to have a VirtioDevice::realize() that virtio devices
> overload such that you don't have to explicitly call the super()
> version.  The advantage of this approach is that you don't have to
> explicitly call the super version.

The disadvantage is that we then have no chance to solve Jesse's
virtio-net issue this way (cf. cover letter), the only improvement would
be Error propagation.

Just let me know which path to pursue here. We could start by converting
*::init to realize signature and then follow up with either conversion.
Would that be acceptable to move forward?
Long-term a VirtioDevice::realize() would be blurring semantics though.

Partially affects pending ISA series as well (PIT/PIC).

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