From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53415) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Umh9U-0004bX-Pn for qemu-devel@nongnu.org; Wed, 12 Jun 2013 05:15:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Umh9O-00051w-TY for qemu-devel@nongnu.org; Wed, 12 Jun 2013 05:15:36 -0400 Received: from cantor2.suse.de ([195.135.220.15]:54988 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Umh9O-00051e-IN for qemu-devel@nongnu.org; Wed, 12 Jun 2013 05:15:30 -0400 Message-ID: <51B83C2E.7040608@suse.de> Date: Wed, 12 Jun 2013 11:15:26 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 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> In-Reply-To: <87a9mypvjk.fsf@codemonkey.ws> 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: Anthony Liguori , "Michael S. Tsirkin" Cc: Kevin Wolf , Peter Crosthwaite , qemu-devel@nongnu.org, jlarrew@linux.vnet.ibm.com, "Aneesh Kumar K.V" , Stefan Hajnoczi , Amit Shah , Paolo Bonzini , fred.konrad@greensocs.com Am 10.06.2013 04:08, schrieb Anthony Liguori: > Peter Crosthwaite writes: >> On Sat, Jun 8, 2013 at 7:55 PM, Andreas F=C3=A4rber = wrote: >>> 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 th= e >>>> 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); >>>> +} >=20 > What's weird about this is that you aren't necessarily calling > Device::realize() here, you're really calling super()::realize(). We can certainly fix that by passing ObjectClass *oc argument instead of DeviceState *dev and using object_class_get_parent(oc) - dc is used uninitialized above. > I really don't know whether it's a better approach or not. It does save LOCs and should work with sane class_inits. > 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. The disadvantage is that we then have no chance to solve Jesse's virtio-net issue this way (cf. cover letter), the only improvement would be Error propagation. Just let me know which path to pursue here. We could start by converting *::init to realize signature and then follow up with either conversion. Would that be acceptable to move forward? Long-term a VirtioDevice::realize() would be blurring semantics though. Partially affects pending ISA series as well (PIT/PIC). Regards, Andreas --=20 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=BC= rnberg