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

[Peter Crosthwaite]

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);
>> +}
>> +
>>
>> 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
>