On Sun, Jun 09, 2013 at 09:08:15PM -0500, Anthony Liguori wrote: > Peter Crosthwaite <peter.crosthwa...@xilinx.com> writes: > > > Hi Andreas, > > > > On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaer...@suse.de> wrote: > >> Hi, > >> > >> Am 08.06.2013 04:22, schrieb Peter Crosthwaite: > >>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaer...@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(). > > 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
Nod. Since all realize calls get same parameters not calling parent's constructor automatically seems strange. > >>> + > >>> > >>> 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 > >>