From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38634) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ulvcf-0000Pt-O5 for qemu-devel@nongnu.org; Mon, 10 Jun 2013 02:30:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ulvca-0001WG-Ph for qemu-devel@nongnu.org; Mon, 10 Jun 2013 02:30:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59513) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ulvca-0001W3-Hi for qemu-devel@nongnu.org; Mon, 10 Jun 2013 02:30:28 -0400 Date: Mon, 10 Jun 2013 09:30:36 +0300 From: "Michael S. Tsirkin" Message-ID: <20130610063036.GA26265@redhat.com> References: <1370629140-30841-1-git-send-email-afaerber@suse.de> <1370629140-30841-3-git-send-email-afaerber@suse.de> <51B2FFA0.6020403@suse.de> <87a9mypvjk.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <87a9mypvjk.fsf@codemonkey.ws> 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: Anthony Liguori Cc: Kevin Wolf , Peter Crosthwaite , qemu-devel@nongnu.org, jlarrew@linux.vnet.ibm.com, "Aneesh Kumar K.V" , Stefan Hajnoczi , Amit Shah , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= , fred.konrad@greensocs.com On Sun, Jun 09, 2013 at 09:08:15PM -0500, Anthony Liguori wrote: > Peter Crosthwaite writes: >=20 > > Hi Andreas, > > > > On Sat, Jun 8, 2013 at 7:55 PM, Andreas F=E4rber w= rote: > >> Hi, > >> > >> Am 08.06.2013 04:22, schrieb Peter Crosthwaite: > >>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas F=E4rber = 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 t= he > >>> overridden parent class functions already, so it can be made to wor= k > >>> 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); > >>> +} >=20 > What's weird about this is that you aren't necessarily calling > Device::realize() here, you're really calling super()::realize(). >=20 > I really don't know whether it's a better approach or not. >=20 > 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. >=20 > Regards, >=20 > 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 ne= ed > >>> to define struct FooClass when creating new abstractions (or virtua= l > >>> 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? >=20 >=20 >=20 > > > > Regards, > > Peter > > > >> Regards, > >> Andreas > >> > >> -- > >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > >> GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FC= rnberg > >>