[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM rea
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize |
Date: |
Mon, 10 Jun 2013 09:30:36 +0300 |
On Sun, Jun 09, 2013 at 09:08:15PM -0500, Anthony Liguori wrote:
> Peter Crosthwaite <address@hidden> writes:
>
> > 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);
> >>> +}
>
> 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
> >>
- [Qemu-devel] [PATCH RFT 4/5] virtio-console: Use exitfn for virtserialport, too, (continued)
- [Qemu-devel] [PATCH RFT 4/5] virtio-console: Use exitfn for virtserialport, too, Andreas Färber, 2013/06/07
- [Qemu-devel] [PATCH RFT 3/5] virtio-console: QOM'ify VirtConsole, Andreas Färber, 2013/06/07
- [Qemu-devel] [PATCH RFT 1/5] virtio-blk-dataplane: Improve error reporting, Andreas Färber, 2013/06/07
- [Qemu-devel] [PATCH RFT 5/5] virtio-serial-port: Convert to QOM realize/unrealize, Andreas Färber, 2013/06/07
- [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Andreas Färber, 2013/06/07
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Peter Crosthwaite, 2013/06/07
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Andreas Färber, 2013/06/08
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Peter Crosthwaite, 2013/06/08
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Anthony Liguori, 2013/06/09
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Andreas Färber, 2013/06/12
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Peter Crosthwaite, 2013/06/12
- Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Peter Crosthwaite, 2013/06/18
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize, Michael S. Tsirkin, 2013/06/09
Re: [Qemu-devel] [PATCH RFT 0/5] QOM realize for virtio, Michael S. Tsirkin, 2013/06/09