From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlrXI-0005Nz-Vp for qemu-devel@nongnu.org; Sun, 09 Jun 2013 22:08:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UlrXH-0002AY-I8 for qemu-devel@nongnu.org; Sun, 09 Jun 2013 22:08:44 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:42366) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlrXH-0002AS-Dl for qemu-devel@nongnu.org; Sun, 09 Jun 2013 22:08:43 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 9 Jun 2013 22:08:42 -0400 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id CCFD76E8028 for ; Sun, 9 Jun 2013 22:08:34 -0400 (EDT) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r5A28cMn305846 for ; Sun, 9 Jun 2013 22:08:38 -0400 Received: from d01av05.pok.ibm.com (loopback [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r5A28cne029955 for ; Sun, 9 Jun 2013 22:08:38 -0400 From: Anthony Liguori In-Reply-To: References: <1370629140-30841-1-git-send-email-afaerber@suse.de> <1370629140-30841-3-git-send-email-afaerber@suse.de> <51B2FFA0.6020403@suse.de> Date: Sun, 09 Jun 2013 21:08:15 -0500 Message-ID: <87a9mypvjk.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Andreas =?utf-8?Q?F?= =?utf-8?Q?=C3=A4rber?= Cc: Kevin Wolf , "Michael S. Tsirkin" , qemu-devel@nongnu.org, jlarrew@linux.vnet.ibm.com, "Aneesh Kumar K.V" , Stefan Hajnoczi , Amit Shah , Paolo Bonzini , fred.konrad@greensocs.com Peter Crosthwaite writes: > Hi Andreas, > > On Sat, Jun 8, 2013 at 7:55 PM, Andreas F=C3=A4rber wr= ote: >> Hi, >> >> Am 08.06.2013 04:22, schrieb Peter Crosthwaite: >>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas F=C3=A4rber = 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[] =3D { >>>> 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 =3D DEVICE_CLASS(klass); >>>> - VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_CLASS(klass); >>>> + DeviceClass *dc =3D DEVICE_CLASS(oc); >>>> + VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_CLASS(oc); >>>> + V9fsClass *v9c =3D VIRTIO_9P_CLASS(oc); >>>> + >>>> + v9c->parent_realize =3D dc->realize; >>>> + dc->realize =3D virtio_9p_device_realize; >>>> + >>>> dc->props =3D virtio_9p_properties; >>>> - vdc->init =3D virtio_9p_device_init; >>>> vdc->get_features =3D virtio_9p_get_features; >>>> vdc->get_config =3D virtio_9p_get_config; >>>> } >>>> @@ -151,6 +157,7 @@ static const TypeInfo virtio_device_info =3D { >>>> .parent =3D TYPE_VIRTIO_DEVICE, >>>> .instance_size =3D sizeof(V9fsState), >>>> .class_init =3D virtio_9p_class_init, >>>> + .class_size =3D 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 =3D 0; >>> static bool qdev_hot_added =3D false; >>> static bool qdev_hot_removed =3D false; >>> >>> +void device_parent_realize(DeviceState *dev, Error **errp) >>> +{ >>> + ObjectClass *class =3D object_get_class(dev); >>> + DeviceClass *dc; >>> + >>> + class =3D object_class_get_parent(dc); >>> + assert(class); >>> + dc =3D 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=C3=BCrnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg >>