qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Date: Sun, 09 Jun 2013 21:08:15 -0500
User-agent: Notmuch/0.15.2+77~g661dcf8 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu)

Peter Crosthwaite <address@hidden> writes:

> Hi Andreas,
>
> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <address@hidden> wrote:
>> 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);
>>> +}

What's weird about this is that you aren't necessarily calling
Device::realize() here, you're really calling super()::realize().

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

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.

Regards,

Anthony Liguori

>>> +
>>>
>>> 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.
>>
>
> Thanks for the history. I found this:
>
> Jan 18 2013 Anthony Liguori wrote:
>
> It's idiomatic from GObject.
>
> I'm not sure I can come up with a concrete example but in the absense of
> a compelling reason to shift from the idiom, I'd strongly suggest not.
>
> Regards,
>
> Anthony Liguori
>
> ------------------------------------
>
> The additive diff on this series would take a massive nosedive - is
> that a compelling reason? It is very unfortunate that pretty much
> every class inheriting from device is going to have to define a class
> struct just for the sake of multi-level realization.
>
> There is roughly 15 or so lines of boiler plate added to every class,
> and in just the cleanup you have done this week you have about 10 or
> so instances of this change pattern. Under the
> child-access-to-parent-version proposal those 15 lines become 1.
>
> I don't see the fundamental reason why child classes shouldnt be able
> to access parent implementations of virtualised functions. Many OO
> oriented languages indeed explicitly build this capability into the
> syntax. Examples include Java with "super.foo()" accesses and C++ via
> the parent class namespace:
>
> class Bar : public Foo {
>   // ...
>
>   void printStuff() {
>     Foo::printStuff(); // calls base class' function
>   }
> };
>
> Anthony is it possible to consider loosening this restriction?



>
> Regards,
> Peter
>
>> 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
>>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]