Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Hi All, On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber afaer...@suse.de wrote: Am 10.06.2013 04:08, schrieb Anthony Liguori: Peter Crosthwaite peter.crosthwa...@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
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Am 10.06.2013 04:08, schrieb Anthony Liguori:
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber afaer...@suse.de wrote:
Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber afaer...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Hi Andreas,
On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber afaer...@suse.de wrote:
Am 10.06.2013 04:08, schrieb Anthony Liguori:
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber afaer...@suse.de wrote:
Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber afaer...@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
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
On Sun, Jun 09, 2013 at 09:08:15PM -0500, Anthony Liguori wrote:
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
Hi Andreas,
On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber afaer...@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 afaer...@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
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
On Fri, Jun 07, 2013 at 08:18:57PM +0200, Andreas Färber 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.
There are also improvements to error handling here, they can
be done by a separate patch.
Please don't mix refactoring and functional changes -
this patch is huge as it is.
Also, could changes to each device be done in a separate patch?
This last might be hard, not a must IMO.
Signed-off-by: Andreas Färber afaer...@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
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
Hi Andreas,
On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber afaer...@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 afaer...@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
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Hi,
Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber afaer...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Hi Andreas,
On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber afaer...@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 afaer...@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
[Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
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 afaer...@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));
+
Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize
Hi Andreas,
On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber afaer...@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 afaer...@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,
-
