Re: [Qemu-devel] [PATCH v2 4/7] virtio-blk: use aliases instead of duplicate qdev properties
On Thu, May 29, 2014 at 07:03:42PM +1000, Peter Crosthwaite wrote:
On Fri, May 23, 2014 at 1:40 AM, Stefan Hajnoczi stefa...@redhat.com wrote:
virtio-blk-pci, virtio-blk-s390, and virtio-blk-ccw all duplicate the
qdev properties of their VirtIOBlock child. This approach does not work
well with string or pointer properties since we must be careful about
leaking or double-freeing them.
Use the QOM alias property to forward property accesses to the
VirtIOBlock child. This way no duplication is necessary.
Remember to stop calling virtio_blk_set_conf() so that we don't clobber
the values already set on the VirtIOBlock instance.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
v2:
* Add qdev_alias_all_properties() instead of virtio-blk-specific
function [Paolo]
---
hw/core/qdev.c | 19 +++
The commit message proper makes no reference to the new qedev core
feature for alias properties. That said, I think the qdev change would
work nicely as a separate patch and this ones commit message would
stand nicely as-is without the qdev patch content.
Good point, I'll split qdev_alias_all_properties() into a separate
patch.
hw/s390x/s390-virtio-bus.c | 9 +
hw/s390x/s390-virtio-bus.h | 1 -
hw/s390x/virtio-ccw.c| 3 +--
hw/s390x/virtio-ccw.h| 1 -
hw/virtio/virtio-pci.c | 3 +--
hw/virtio/virtio-pci.h | 1 -
include/hw/qdev-properties.h | 2 ++
8 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 936eae6..e41f5b8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -724,6 +724,25 @@ void qdev_property_add_static(DeviceState *dev,
Property *prop,
}
}
+/* @qdev_alias_all_properties - Add alias properties to the source object
for
+ * all qdev properties on the target DeviceState.
+ */
+void qdev_alias_all_properties(DeviceState *target, Object *source)
+{
+ObjectClass *class;
+Property *prop;
+
+class = object_get_class(OBJECT(target));
+do {
+for (prop = DEVICE_CLASS(class)-props; prop prop-name;
prop++) {
Don't de-reference an inline QOM cast macro. DEVICE_CLASS(class) will
need its own local variable. Andreas, can we update QOM conventions
with this finer point? I think we already have Avoid using cast
macros other than OBJECT() inline but I remember a thread a while
back along the lines of no de-referencing being the main intention of
this rule.
Sure, this is easy to change. Could you explain the reasoning behind it
though?
By the way, this is copied from hw/core/qdev.c:device_initfn() where we
also dereference inline.
Stefan
Re: [Qemu-devel] [PATCH v2 4/7] virtio-blk: use aliases instead of duplicate qdev properties
On Fri, May 23, 2014 at 1:40 AM, Stefan Hajnoczi stefa...@redhat.com wrote:
virtio-blk-pci, virtio-blk-s390, and virtio-blk-ccw all duplicate the
qdev properties of their VirtIOBlock child. This approach does not work
well with string or pointer properties since we must be careful about
leaking or double-freeing them.
Use the QOM alias property to forward property accesses to the
VirtIOBlock child. This way no duplication is necessary.
Remember to stop calling virtio_blk_set_conf() so that we don't clobber
the values already set on the VirtIOBlock instance.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
v2:
* Add qdev_alias_all_properties() instead of virtio-blk-specific
function [Paolo]
---
hw/core/qdev.c | 19 +++
The commit message proper makes no reference to the new qedev core
feature for alias properties. That said, I think the qdev change would
work nicely as a separate patch and this ones commit message would
stand nicely as-is without the qdev patch content.
hw/s390x/s390-virtio-bus.c | 9 +
hw/s390x/s390-virtio-bus.h | 1 -
hw/s390x/virtio-ccw.c| 3 +--
hw/s390x/virtio-ccw.h| 1 -
hw/virtio/virtio-pci.c | 3 +--
hw/virtio/virtio-pci.h | 1 -
include/hw/qdev-properties.h | 2 ++
8 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 936eae6..e41f5b8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -724,6 +724,25 @@ void qdev_property_add_static(DeviceState *dev, Property
*prop,
}
}
+/* @qdev_alias_all_properties - Add alias properties to the source object for
+ * all qdev properties on the target DeviceState.
+ */
+void qdev_alias_all_properties(DeviceState *target, Object *source)
+{
+ObjectClass *class;
+Property *prop;
+
+class = object_get_class(OBJECT(target));
+do {
+for (prop = DEVICE_CLASS(class)-props; prop prop-name; prop++) {
Don't de-reference an inline QOM cast macro. DEVICE_CLASS(class) will
need its own local variable. Andreas, can we update QOM conventions
with this finer point? I think we already have Avoid using cast
macros other than OBJECT() inline but I remember a thread a while
back along the lines of no de-referencing being the main intention of
this rule.
Regards,
Peter
[Qemu-devel] [PATCH v2 4/7] virtio-blk: use aliases instead of duplicate qdev properties
virtio-blk-pci, virtio-blk-s390, and virtio-blk-ccw all duplicate the
qdev properties of their VirtIOBlock child. This approach does not work
well with string or pointer properties since we must be careful about
leaking or double-freeing them.
Use the QOM alias property to forward property accesses to the
VirtIOBlock child. This way no duplication is necessary.
Remember to stop calling virtio_blk_set_conf() so that we don't clobber
the values already set on the VirtIOBlock instance.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
v2:
* Add qdev_alias_all_properties() instead of virtio-blk-specific
function [Paolo]
---
hw/core/qdev.c | 19 +++
hw/s390x/s390-virtio-bus.c | 9 +
hw/s390x/s390-virtio-bus.h | 1 -
hw/s390x/virtio-ccw.c| 3 +--
hw/s390x/virtio-ccw.h| 1 -
hw/virtio/virtio-pci.c | 3 +--
hw/virtio/virtio-pci.h | 1 -
include/hw/qdev-properties.h | 2 ++
8 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 936eae6..e41f5b8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -724,6 +724,25 @@ void qdev_property_add_static(DeviceState *dev, Property
*prop,
}
}
+/* @qdev_alias_all_properties - Add alias properties to the source object for
+ * all qdev properties on the target DeviceState.
+ */
+void qdev_alias_all_properties(DeviceState *target, Object *source)
+{
+ObjectClass *class;
+Property *prop;
+
+class = object_get_class(OBJECT(target));
+do {
+for (prop = DEVICE_CLASS(class)-props; prop prop-name; prop++) {
+object_property_add_alias(source, prop-name,
+ OBJECT(target), prop-name,
+ error_abort);
+}
+class = object_class_get_parent(class);
+} while (class != object_class_by_name(TYPE_DEVICE));
+}
+
static bool device_get_realized(Object *obj, Error **errp)
{
DeviceState *dev = DEVICE(obj);
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 9c71afa..041d4e2 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -179,7 +179,6 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev)
{
VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev);
DeviceState *vdev = DEVICE(dev-vdev);
-virtio_blk_set_conf(vdev, (dev-blk));
qdev_set_parent_bus(vdev, BUS(s390_dev-bus));
if (qdev_init(vdev) 0) {
return -1;
@@ -192,6 +191,7 @@ static void s390_virtio_blk_instance_init(Object *obj)
VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_BLK);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev), NULL);
+qdev_alias_all_properties(DEVICE(dev-vdev), obj);
}
static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
@@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
.class_init= s390_virtio_net_class_init,
};
-static Property s390_virtio_blk_properties[] = {
-DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
-DEFINE_PROP_END_OF_LIST(),
-};
-
static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
{
-DeviceClass *dc = DEVICE_CLASS(klass);
VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
k-init = s390_virtio_blk_init;
-dc-props = s390_virtio_blk_properties;
}
static const TypeInfo s390_virtio_blk = {
diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h
index ac81bd8..ffd0df7 100644
--- a/hw/s390x/s390-virtio-bus.h
+++ b/hw/s390x/s390-virtio-bus.h
@@ -124,7 +124,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev);
typedef struct VirtIOBlkS390 {
VirtIOS390Device parent_obj;
VirtIOBlock vdev;
-VirtIOBlkConf blk;
} VirtIOBlkS390;
/* virtio-scsi-s390 */
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 082bb42..f33293d 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -725,7 +725,6 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
{
VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
DeviceState *vdev = DEVICE(dev-vdev);
-virtio_blk_set_conf(vdev, (dev-blk));
qdev_set_parent_bus(vdev, BUS(ccw_dev-bus));
if (qdev_init(vdev) 0) {
return -1;
@@ -739,6 +738,7 @@ static void virtio_ccw_blk_instance_init(Object *obj)
VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_BLK);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev), NULL);
+qdev_alias_all_properties(DEVICE(dev-vdev), obj);
}
static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
@@ -1126,7 +1126,6 @@ static const TypeInfo virtio_ccw_net = {
static Property virtio_ccw_blk_properties[] = {
DEFINE_PROP_STRING(devno, VirtioCcwDevice, bus_id),
DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
-
