[RFT,2/5] virtio: Convert VirtioDevice to QOM realize/unrealize

Hi Andreas,

On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaerber@suse.de> wrote:
> VirtioDevice's DeviceClass::exit code cleaning up bus_name is no longer
> overwritten by virtio-{blk,serial,net,scsi,balloon,rng} and vhost-scsi.
>
> Note: VirtIOSCSI and VHostSCSI now perform some initializations after
> VirtIOSCSICommon's realize calls virtio_bus_plug_device(), namely
> creating the SCSIBus and initializing /dev/vhost-scsi respectively.
>
> While at it, drop duplicate VIRTIO_DEVICE() casts and avoid qdev.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/9pfs/virtio-9p-device.c         | 67 ++++++++++++++++--------------
>  hw/9pfs/virtio-9p.h                | 13 ++++++
>  hw/block/virtio-blk.c              | 52 ++++++++++++++---------
>  hw/char/virtio-serial-bus.c        | 49 ++++++++++++++--------
>  hw/net/virtio-net.c                | 48 ++++++++++++---------
>  hw/scsi/vhost-scsi.c               | 59 +++++++++++++++-----------
>  hw/scsi/virtio-scsi.c              | 85 ++++++++++++++++++++++++--------------
>  hw/virtio/virtio-balloon.c         | 50 +++++++++++++---------
>  hw/virtio/virtio-rng.c             | 53 ++++++++++++++----------
>  hw/virtio/virtio.c                 | 20 ++++-----
>  include/hw/virtio/vhost-scsi.h     | 13 ++++++
>  include/hw/virtio/virtio-balloon.h | 13 ++++++
>  include/hw/virtio/virtio-blk.h     | 13 ++++++
>  include/hw/virtio/virtio-net.h     | 13 ++++++
>  include/hw/virtio/virtio-rng.h     | 13 ++++++
>  include/hw/virtio/virtio-scsi.h    | 29 +++++++++++--
>  include/hw/virtio/virtio-serial.h  | 13 ++++++
>  include/hw/virtio/virtio.h         |  6 ++-
>  18 files changed, 406 insertions(+), 203 deletions(-)
>
> 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
> @@ -41,15 +41,17 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
>      g_free(cfg);
>  }
>
> -static int virtio_9p_device_init(VirtIODevice *vdev)
> +static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>  {
> -    V9fsState *s = VIRTIO_9P(vdev);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    V9fsState *s = VIRTIO_9P(dev);
> +    V9fsClass *v9c = VIRTIO_9P_GET_CLASS(dev);
>      int i, len;
>      struct stat stat;
>      FsDriverEntry *fse;
>      V9fsPath path;
>
> -    virtio_init(VIRTIO_DEVICE(s), "virtio-9p", VIRTIO_ID_9P,
> +    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
>                  sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
>
>      /* initialize pdu allocator */
> @@ -65,17 +67,17 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>
>      if (!fse) {
>          /* We don't have a fsdev identified by fsdev_id */
> -        fprintf(stderr, "Virtio-9p device couldn't find fsdev with the "
> -                "id = %s\n",
> -                s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
> -        return -1;
> +        error_setg(errp, "Virtio-9p device couldn't find fsdev with the "
> +                   "id = %s",
> +                   s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
> +        return;
>      }
>
>      if (!s->fsconf.tag) {
>          /* we haven't specified a mount_tag */
> -        fprintf(stderr, "fsdev with id %s needs mount_tag arguments\n",
> -                s->fsconf.fsdev_id);
> -        return -1;
> +        error_setg(errp, "fsdev with id %s needs mount_tag arguments",
> +                   s->fsconf.fsdev_id);
> +        return;
>      }
>
>      s->ctx.export_flags = fse->export_flags;
> @@ -83,9 +85,9 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>      s->ctx.exops.get_st_gen = NULL;
>      len = strlen(s->fsconf.tag);
>      if (len > MAX_TAG_LEN - 1) {
> -        fprintf(stderr, "mount tag '%s' (%d bytes) is longer than "
> -                "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
> -        return -1;
> +        error_setg(errp, "mount tag '%s' (%d bytes) is longer than "
> +                   "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
> +        return;
>      }
>
>      s->tag = strdup(s->fsconf.tag);
> @@ -97,13 +99,13 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>      qemu_co_rwlock_init(&s->rename_lock);
>
>      if (s->ops->init(&s->ctx) < 0) {
> -        fprintf(stderr, "Virtio-9p Failed to initialize fs-driver with id:%s"
> -                " and export path:%s\n", s->fsconf.fsdev_id, s->ctx.fs_root);
> -        return -1;
> +        error_setg(errp, "Virtio-9p Failed to initialize fs-driver with id:%s"
> +                   " and export path:%s", s->fsconf.fsdev_id, s->ctx.fs_root);
> +        return;
>      }
>      if (v9fs_init_worker_threads() < 0) {
> -        fprintf(stderr, "worker thread initialization failed\n");
> -        return -1;
> +        error_setg(errp, "worker thread initialization failed");
> +        return;
>      }
>
>      /*
> @@ -113,20 +115,20 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>       */
>      v9fs_path_init(&path);
>      if (s->ops->name_to_path(&s->ctx, NULL, "/", &path) < 0) {
> -        fprintf(stderr,
> -                "error in converting name to path %s", strerror(errno));
> -        return -1;
> +        error_setg(errp,
> +                   "error in converting name to path %s", strerror(errno));
> +        return;
>      }
>      if (s->ops->lstat(&s->ctx, &path, &stat)) {
> -        fprintf(stderr, "share path %s does not exist\n", fse->path);
> -        return -1;
> +        error_setg(errp, "share path %s does not exist", fse->path);
> +        return;
>      } else if (!S_ISDIR(stat.st_mode)) {
> -        fprintf(stderr, "share path %s is not a directory\n", fse->path);
> -        return -1;
> +        error_setg(errp, "share path %s is not a directory", fse->path);
> +        return;
>      }
>      v9fs_path_free(&path);
>
> -    return 0;
> +    v9c->parent_realize(dev, errp);
>  }
>
>  /* virtio-9p device */
> @@ -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:

to define struct FooClass when creating new abstractions (or virtual
functions if your C++).

Regards,
Peter

Hi,

Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaerber@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);
> +}
> +
> 
> 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.

Regards,
Andreas

Hi Andreas,

On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@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 <afaerber@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);
>> +}
>> +
>>
>> 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
>

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Hi Andreas,
>
> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@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 <afaerber@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

>>> +
>>>
>>> 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
>>

On Sun, Jun 09, 2013 at 09:08:15PM -0500, Anthony Liguori wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> 
> > Hi Andreas,
> >
> > On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@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 <afaerber@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
> >>

Am 10.06.2013 04:08, schrieb Anthony Liguori:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
>>>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaerber@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().

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

Hi Andreas,

On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.06.2013 04:08, schrieb Anthony Liguori:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>>> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
>>>>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaerber@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().
>
> We can certainly fix that by passing ObjectClass *oc argument instead of
> DeviceState *dev and using object_class_get_parent(oc)

That is slightly more defensive but does cost you a line of code on
every usage (to do the DEVICE_GET_CLASS(dev)). Im happy either way as
its the need for the FooClass definition that really bloats the code.
Under this modified proposal its a two line change rather than the one
which is not so bad.

> - dc is used
> uninitialized above.
>

Typo on my part. should be "class"

>> 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.
>

+1. I thought we were trying to get away from abstractions
(SYS_BUS_DEVICE, I2C_DEVICE etc, and friends) defining their own
abstract inititialization fns (init or realize) so this would be a
step backwards. It also makes the code base very inconsistent with
itself. Devices which inherit from an  abstraction that already have a
realize need to use the specific realize while those that dont would
use the generic one (DeviceClass::realize). Also makes life very hard
when someone wants to add common realize functionality to an
abstraction (e.g. implemenent a non-trivial SysBusDevice::Realize) as
all the concrete classes would then need conversion.

> 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?

Sounds good to me, and makes it easier to disentangle this particular
issue with the rest of your changes.

Regards,
Peter

> Long-term a VirtioDevice::realize() would be blurring semantics though.
>
> Partially affects pending ISA series as well (PIT/PIC).
>
> 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
>

Hi All,

On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.06.2013 04:08, schrieb Anthony Liguori:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>>
>> 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).
>

This got merged, so I took the opportunity to workshop changing one of
them to use this super idea for the sake of discussion. Patches on
list. Please review in the context of this discussion.

Regards,
Peter