* [Qemu-devel] [PATCH v2 1/4] qom: split object_property_set_link()
2014-03-05 9:40 [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
@ 2014-03-05 9:40 ` Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 2/4] qom: make QOM link property unref optional Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 9:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Hajnoczi, Andreas Faerber, Anthony Liguori,
Paolo Bonzini
The path resolution logic in object_property_set_link() should be a
separate function. This makes the code easier to read and maintain.
More importantly, the error behavior of object_property_set_link() is
dangerous. It sets the link property object to NULL if an error occurs.
A setter function should either succeed or fail, it shouldn't leave the
value NULL on failure.
This patch splits the code and fixes the error case so the old link
property object is left in place on failure.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qom/object.c | 79 ++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 660859c..c11ea1e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1039,48 +1039,65 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
}
}
-static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
+/* object_resolve_link:
+ *
+ * Lookup an object and ensure its type matches the link property type. This
+ * is similar to object_resolve_path() except type verification against the
+ * link property is performed.
+ *
+ * Returns: The matched object or NULL on path lookup failures.
+ */
+static Object *object_resolve_link(Object *obj, const char *name,
+ const char *path, Error **errp)
{
- Object **child = opaque;
- Object *old_target;
- bool ambiguous = false;
const char *type;
- char *path;
gchar *target_type;
+ bool ambiguous = false;
+ Object *target;
+ /* Go from link<FOO> to FOO. */
type = object_property_get_type(obj, name, NULL);
-
- visit_type_str(v, &path, name, errp);
-
- old_target = *child;
- *child = NULL;
-
- if (strcmp(path, "") != 0) {
- Object *target;
-
- /* Go from link<FOO> to FOO. */
- target_type = g_strndup(&type[5], strlen(type) - 6);
- target = object_resolve_path_type(path, target_type, &ambiguous);
-
- if (ambiguous) {
- error_set(errp, QERR_AMBIGUOUS_PATH, path);
- } else if (target) {
- object_ref(target);
- *child = target;
+ target_type = g_strndup(&type[5], strlen(type) - 6);
+ target = object_resolve_path_type(path, target_type, &ambiguous);
+
+ if (ambiguous) {
+ error_set(errp, QERR_AMBIGUOUS_PATH, path);
+ } else if (!target) {
+ target = object_resolve_path(path, &ambiguous);
+ if (target || ambiguous) {
+ error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
} else {
- target = object_resolve_path(path, &ambiguous);
- if (target || ambiguous) {
- error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
- } else {
- error_set(errp, QERR_DEVICE_NOT_FOUND, path);
- }
+ error_set(errp, QERR_DEVICE_NOT_FOUND, path);
}
- g_free(target_type);
}
+ g_free(target_type);
+ return target;
+}
+
+static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ Error *local_err = NULL;
+ Object **child = opaque;
+ Object *old_target = *child;
+ Object *new_target = NULL;
+ char *path = NULL;
+
+ visit_type_str(v, &path, name, &local_err);
+ if (!local_err && strcmp(path, "") != 0) {
+ new_target = object_resolve_link(obj, name, path, &local_err);
+ }
g_free(path);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ if (new_target) {
+ object_ref(new_target);
+ }
+ *child = new_target;
if (old_target != NULL) {
object_unref(old_target);
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v2 2/4] qom: make QOM link property unref optional
2014-03-05 9:40 [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 1/4] qom: split object_property_set_link() Stefan Hajnoczi
@ 2014-03-05 9:40 ` Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 3/4] qom: add set() argument to object_property_add_link() Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 9:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Michael S. Tsirkin,
Alexander Graf, Peter Crosthwaite, Stefan Hajnoczi, Paolo Bonzini,
Andreas Faerber
Some object_property_add_link() callers expect property deletion to
unref the link property object. Other callers expect to manage the
refcount themselves. The former are currently broken and therefore leak
the link property object.
This patch adds the bool unref_on_release argument to
object_property_add_link() so the caller can specify which refcount
behavior they require.
This fixes refcount leaks in qdev.c, xilinx_axidma.c, xilinx_axienet.c,
s390-virtio-bus.c, virtio-pci.c, virtio-rng.c, and ui/console.c.
Rationale for refcount behavior:
* hw/core/qdev.c
- bus children are explicitly unreferenced, don't interfere
- parent_bus is essentially a read-only property that doesn't hold a
refcount, don't unref
- hotplug_handler is leaked, do unref
* hw/dma/xilinx_axidma.c
- rx stream "dma" links are set using set_link, therefore they
need unref
- tx streams are set using set_link, therefore they need unref
* hw/net/xilinx_axienet.c
- same reasoning as hw/dma/xilinx_axidma.c
* hw/pcmcia/pxa2xx.c
- pxa2xx bypasses set_link and therefore does not use refcounts
* hw/s390x/s390-virtio-bus.c
* hw/virtio/virtio-pci.c
* hw/virtio/virtio-rng.c
* ui/console.c
- set_link is used and there is no explicit unref, do unref
Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/core/qdev.c | 7 +++++--
hw/dma/xilinx_axidma.c | 12 ++++++++----
hw/net/xilinx_axienet.c | 12 ++++++++----
hw/pcmcia/pxa2xx.c | 4 +++-
hw/s390x/s390-virtio-bus.c | 3 ++-
hw/s390x/virtio-ccw.c | 3 ++-
hw/virtio/virtio-pci.c | 3 ++-
hw/virtio/virtio-rng.c | 3 ++-
include/qom/object.h | 7 ++++++-
qom/object.c | 39 ++++++++++++++++++++++++++++++++++-----
ui/console.c | 3 ++-
11 files changed, 74 insertions(+), 22 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c0b857f..273c737 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -98,6 +98,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
object_property_add_link(OBJECT(bus), name,
object_get_typename(OBJECT(child)),
(Object **)&kid->child,
+ false, /* return ownership on prop deletion */
NULL);
}
@@ -761,7 +762,8 @@ static void device_initfn(Object *obj)
} while (class != object_class_by_name(TYPE_DEVICE));
object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
- (Object **)&dev->parent_bus, &error_abort);
+ (Object **)&dev->parent_bus, false,
+ &error_abort);
}
static void device_post_init(Object *obj)
@@ -881,7 +883,8 @@ static void qbus_initfn(Object *obj)
QTAILQ_INIT(&bus->children);
object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
TYPE_HOTPLUG_HANDLER,
- (Object **)&bus->hotplug_handler, NULL);
+ (Object **)&bus->hotplug_handler,
+ true, NULL);
}
static char *default_bus_get_fw_dev_path(DeviceState *dev)
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 19f07b3..cc5826d 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,9 +537,11 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
Error *local_errp = NULL;
object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
- (Object **)&ds->dma, &local_errp);
+ (Object **)&ds->dma,
+ true, &local_errp);
object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
- (Object **)&cs->dma, &local_errp);
+ (Object **)&cs->dma,
+ true, &local_errp);
if (local_errp) {
goto xilinx_axidma_realize_fail;
}
@@ -571,10 +573,12 @@ static void xilinx_axidma_init(Object *obj)
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
- (Object **)&s->tx_data_dev, &error_abort);
+ (Object **)&s->tx_data_dev,
+ true, &error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
- (Object **)&s->tx_control_dev, &error_abort);
+ (Object **)&s->tx_control_dev,
+ true, &error_abort);
object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
TYPE_XILINX_AXI_DMA_DATA_STREAM);
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 0bd5eda..385e059 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -945,9 +945,11 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
Error *local_errp = NULL;
object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
- (Object **) &ds->enet, &local_errp);
+ (Object **) &ds->enet,
+ true, &local_errp);
object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
- (Object **) &cs->enet, &local_errp);
+ (Object **) &cs->enet,
+ true, &local_errp);
if (local_errp) {
goto xilinx_enet_realize_fail;
}
@@ -982,10 +984,12 @@ static void xilinx_enet_init(Object *obj)
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
- (Object **) &s->tx_data_dev, &error_abort);
+ (Object **) &s->tx_data_dev,
+ true, &error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
- (Object **) &s->tx_control_dev, &error_abort);
+ (Object **) &s->tx_control_dev,
+ true, &error_abort);
object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
TYPE_XILINX_AXI_ENET_DATA_STREAM);
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index 8f17596..f55a806 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -198,7 +198,9 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
s->slot.irq = qemu_allocate_irqs(pxa2xx_pcmcia_set_irq, s, 1)[0];
object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
- (Object **)&s->card, NULL);
+ (Object **)&s->card,
+ false, /* don't unref on prop deletion */
+ NULL);
}
/* Insert a new card into a slot */
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index e4fc353..0c9ff92 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -313,7 +313,8 @@ static void s390_virtio_rng_instance_init(Object *obj)
object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
- (Object **)&dev->vdev.conf.rng, NULL);
+ (Object **)&dev->vdev.conf.rng,
+ true, NULL);
}
static uint64_t s390_virtio_device_vq_token(VirtIOS390Device *dev, int vq)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index f6e0e3e..2fe782b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1184,7 +1184,8 @@ static void virtio_ccw_rng_instance_init(Object *obj)
object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
- (Object **)&dev->vdev.conf.rng, NULL);
+ (Object **)&dev->vdev.conf.rng,
+ true, NULL);
}
static Property virtio_ccw_rng_properties[] = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7b91841..e91de26 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1517,7 +1517,8 @@ static void virtio_rng_initfn(Object *obj)
object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
- (Object **)&dev->vdev.conf.rng, NULL);
+ (Object **)&dev->vdev.conf.rng,
+ true, NULL);
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index a16e3bc..54ae848 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -223,7 +223,8 @@ static void virtio_rng_initfn(Object *obj)
VirtIORNG *vrng = VIRTIO_RNG(obj);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
- (Object **)&vrng->conf.rng, NULL);
+ (Object **)&vrng->conf.rng,
+ true, NULL);
}
static const TypeInfo virtio_rng_info = {
diff --git a/include/qom/object.h b/include/qom/object.h
index 9c7c361..276d02e 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1065,6 +1065,8 @@ void object_property_add_child(Object *obj, const char *name,
* @name: the name of the property
* @type: the qobj type of the link
* @child: a pointer to where the link object reference is stored
+ * @unref_on_release: whether to release the link object reference when
+ * the property is deleted
* @errp: if an error occurs, a pointer to an area to store the area
*
* Links establish relationships between objects. Links are unidirectional
@@ -1076,10 +1078,13 @@ void object_property_add_child(Object *obj, const char *name,
* Ownership of the pointer that @child points to is transferred to the
* link property. The reference count for <code>*@child</code> is
* managed by the property from after the function returns till the
- * property is deleted with object_property_del().
+ * property is deleted with object_property_del(). If
+ * <code>@unref_on_release</code> is true, the reference count is
+ * decremented when the property is deleted.
*/
void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
+ bool unref_on_release,
Error **errp);
/**
diff --git a/qom/object.c b/qom/object.c
index c11ea1e..89c5358 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,10 +1023,16 @@ out:
g_free(type);
}
+typedef struct {
+ Object **child;
+ bool unref_on_release;
+} LinkProperty;
+
static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
- Object **child = opaque;
+ LinkProperty *lprop = opaque;
+ Object **child = lprop->child;
gchar *path;
if (*child) {
@@ -1079,8 +1085,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
Error *local_err = NULL;
- Object **child = opaque;
- Object *old_target = *child;
+ LinkProperty *prop = opaque;
+ Object *old_target = *prop->child;
Object *new_target = NULL;
char *path = NULL;
@@ -1097,24 +1103,47 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
if (new_target) {
object_ref(new_target);
}
- *child = new_target;
+ *prop->child = new_target;
if (old_target != NULL) {
object_unref(old_target);
}
}
+static void object_release_link_property(Object *obj, const char *name,
+ void *opaque)
+{
+ LinkProperty *prop = opaque;
+
+ if (prop->unref_on_release && *prop->child) {
+ object_unref(*prop->child);
+ }
+ g_free(prop);
+}
+
void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
+ bool unref_on_release,
Error **errp)
{
+ Error *local_err = NULL;
+ LinkProperty *prop = g_malloc(sizeof(*prop));
gchar *full_type;
+ prop->child = child;
+ prop->unref_on_release = unref_on_release;
+
full_type = g_strdup_printf("link<%s>", type);
object_property_add(obj, name, full_type,
object_get_link_property,
object_set_link_property,
- NULL, child, errp);
+ object_release_link_property,
+ prop,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ g_free(prop);
+ }
g_free(full_type);
}
diff --git a/ui/console.c b/ui/console.c
index 502e160..af2d617 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1178,7 +1178,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
obj = object_new(TYPE_QEMU_CONSOLE);
s = QEMU_CONSOLE(obj);
object_property_add_link(obj, "device", TYPE_DEVICE,
- (Object **)&s->device, &local_err);
+ (Object **)&s->device,
+ true, &local_err);
if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
(console_type == GRAPHIC_CONSOLE))) {
--
1.8.5.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v2 3/4] qom: add set() argument to object_property_add_link()
2014-03-05 9:40 [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 1/4] qom: split object_property_set_link() Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 2/4] qom: make QOM link property unref optional Stefan Hajnoczi
@ 2014-03-05 9:40 ` Stefan Hajnoczi
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 4/4] virtio-rng: avoid default_backend refcount leak Stefan Hajnoczi
2014-03-05 13:29 ` [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 9:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Michael S. Tsirkin,
Alexander Graf, Peter Crosthwaite, Stefan Hajnoczi, Paolo Bonzini,
Andreas Faerber
There are currently three types of object_property_add_link() callers:
1. The link property may be set at any time.
2. The link property of a DeviceState instance may only be set before
realize.
3. The link property may never be set, it is read-only.
Something similar can already be achieved with
object_property_add_str()'s set() argument. Follow its example and add
a set() argument to object_property_add_link().
Also provide default set() functions for case #1 and #2. Case #3 is
covered by passing a NULL function pointer.
Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/core/qdev-properties.c | 12 ++++++++++++
hw/core/qdev.c | 4 +++-
hw/dma/xilinx_axidma.c | 4 ++++
hw/net/xilinx_axienet.c | 4 ++++
hw/pcmcia/pxa2xx.c | 1 +
hw/s390x/s390-virtio-bus.c | 1 +
hw/s390x/virtio-ccw.c | 1 +
hw/virtio/virtio-pci.c | 1 +
hw/virtio/virtio-rng.c | 1 +
include/hw/qdev-properties.h | 11 +++++++++++
include/qom/object.h | 12 ++++++++++++
qom/object.c | 20 +++++++++++++++++++-
ui/console.c | 1 +
13 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 77d0c66..2858f11 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -21,6 +21,18 @@ void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
}
}
+void qdev_prop_default_set_link(Object *obj, const char *name,
+ Object *val, Error **errp)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ if (dev->realized) {
+ error_setg(errp, "Attempt to set link property '%s' on device '%s' "
+ "(type '%s') after it was realized",
+ name, dev->id, object_get_typename(obj));
+ }
+}
+
void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
{
void *ptr = dev;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 273c737..c40fd8b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -98,6 +98,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
object_property_add_link(OBJECT(bus), name,
object_get_typename(OBJECT(child)),
(Object **)&kid->child,
+ NULL, /* read-only property */
false, /* return ownership on prop deletion */
NULL);
}
@@ -762,7 +763,7 @@ static void device_initfn(Object *obj)
} while (class != object_class_by_name(TYPE_DEVICE));
object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
- (Object **)&dev->parent_bus, false,
+ (Object **)&dev->parent_bus, NULL, false,
&error_abort);
}
@@ -884,6 +885,7 @@ static void qbus_initfn(Object *obj)
object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
TYPE_HOTPLUG_HANDLER,
(Object **)&bus->hotplug_handler,
+ object_property_default_set_link,
true, NULL);
}
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index cc5826d..a0da529 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -538,9 +538,11 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
(Object **)&ds->dma,
+ object_property_default_set_link,
true, &local_errp);
object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
(Object **)&cs->dma,
+ object_property_default_set_link,
true, &local_errp);
if (local_errp) {
goto xilinx_axidma_realize_fail;
@@ -574,10 +576,12 @@ static void xilinx_axidma_init(Object *obj)
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
(Object **)&s->tx_data_dev,
+ qdev_prop_default_set_link,
true, &error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
(Object **)&s->tx_control_dev,
+ qdev_prop_default_set_link,
true, &error_abort);
object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 385e059..4e139b7 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -946,9 +946,11 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
(Object **) &ds->enet,
+ object_property_default_set_link,
true, &local_errp);
object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
(Object **) &cs->enet,
+ object_property_default_set_link,
true, &local_errp);
if (local_errp) {
goto xilinx_enet_realize_fail;
@@ -985,10 +987,12 @@ static void xilinx_enet_init(Object *obj)
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
(Object **) &s->tx_data_dev,
+ qdev_prop_default_set_link,
true, &error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
(Object **) &s->tx_control_dev,
+ qdev_prop_default_set_link,
true, &error_abort);
object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index f55a806..ee113e1 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -199,6 +199,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
(Object **)&s->card,
+ NULL, /* read-only property */
false, /* don't unref on prop deletion */
NULL);
}
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 0c9ff92..027558b 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -314,6 +314,7 @@ static void s390_virtio_rng_instance_init(Object *obj)
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
(Object **)&dev->vdev.conf.rng,
+ qdev_prop_default_set_link,
true, NULL);
}
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 2fe782b..53fa86e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1185,6 +1185,7 @@ static void virtio_ccw_rng_instance_init(Object *obj)
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
(Object **)&dev->vdev.conf.rng,
+ qdev_prop_default_set_link,
true, NULL);
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e91de26..2b3c93f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1518,6 +1518,7 @@ static void virtio_rng_initfn(Object *obj)
object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
(Object **)&dev->vdev.conf.rng,
+ qdev_prop_default_set_link,
true, NULL);
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 54ae848..8efe0aa 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -224,6 +224,7 @@ static void virtio_rng_initfn(Object *obj)
object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
(Object **)&vrng->conf.rng,
+ qdev_prop_default_set_link,
true, NULL);
}
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0c0babf..da35cb3 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,4 +201,15 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
*/
void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
Error **errp);
+
+/**
+ * @qdev_prop_default_set_link:
+ *
+ * Set the Error object if an attempt is made to set the link after realize.
+ * This function should be used as the set() argument to
+ * object_property_add_link().
+ */
+void qdev_prop_default_set_link(Object *obj, const char *name,
+ Object *val, Error **errp);
+
#endif
diff --git a/include/qom/object.h b/include/qom/object.h
index 276d02e..fc25888 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1060,11 +1060,21 @@ void object_property_add_child(Object *obj, const char *name,
Object *child, Error **errp);
/**
+ * object_property_default_set_link:
+ *
+ * The default implementation of the object_property_add_link() set callback
+ * function. It allows the link property to be set and never returns an error.
+ */
+void object_property_default_set_link(Object *, const char *,
+ Object *, Error **);
+
+/**
* object_property_add_link:
* @obj: the object to add a property to
* @name: the name of the property
* @type: the qobj type of the link
* @child: a pointer to where the link object reference is stored
+ * @set: the setter or NULL if the property is read-only
* @unref_on_release: whether to release the link object reference when
* the property is deleted
* @errp: if an error occurs, a pointer to an area to store the area
@@ -1084,6 +1094,8 @@ void object_property_add_child(Object *obj, const char *name,
*/
void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
+ void (*set)(Object *obj, const char *name,
+ Object *val, Error **errp),
bool unref_on_release,
Error **errp);
diff --git a/qom/object.c b/qom/object.c
index 89c5358..70ba858 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,9 +1023,16 @@ out:
g_free(type);
}
+void object_property_default_set_link(Object *obj, const char *name,
+ Object *val, Error **errp)
+{
+ /* Allow the link to be set, always */
+}
+
typedef struct {
Object **child;
bool unref_on_release;
+ void (*set)(Object *, const char *, Object *, Error **);
} LinkProperty;
static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
@@ -1100,6 +1107,14 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
return;
}
+ if (prop->set) {
+ prop->set(obj, name, new_target, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ }
+
if (new_target) {
object_ref(new_target);
}
@@ -1122,6 +1137,8 @@ static void object_release_link_property(Object *obj, const char *name,
void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
+ void (*set)(Object *, const char *,
+ Object *, Error **),
bool unref_on_release,
Error **errp)
{
@@ -1130,13 +1147,14 @@ void object_property_add_link(Object *obj, const char *name,
gchar *full_type;
prop->child = child;
+ prop->set = set;
prop->unref_on_release = unref_on_release;
full_type = g_strdup_printf("link<%s>", type);
object_property_add(obj, name, full_type,
object_get_link_property,
- object_set_link_property,
+ set ? object_set_link_property : NULL,
object_release_link_property,
prop,
&local_err);
diff --git a/ui/console.c b/ui/console.c
index af2d617..6220654 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1179,6 +1179,7 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
s = QEMU_CONSOLE(obj);
object_property_add_link(obj, "device", TYPE_DEVICE,
(Object **)&s->device,
+ object_property_default_set_link,
true, &local_err);
if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
--
1.8.5.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v2 4/4] virtio-rng: avoid default_backend refcount leak
2014-03-05 9:40 [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
` (2 preceding siblings ...)
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 3/4] qom: add set() argument to object_property_add_link() Stefan Hajnoczi
@ 2014-03-05 9:40 ` Stefan Hajnoczi
2014-03-05 13:29 ` [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 9:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Stefan Hajnoczi, Paolo Bonzini,
Andreas Faerber, KONRAD Frederic
QOM child properties take a reference to the object and release it when
the property is deleted. Therefore we should unref the default_backend
after we have added it as a child property.
Cc: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/virtio/virtio-rng.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 8efe0aa..ddba756 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -162,6 +162,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
OBJECT(vrng->conf.default_backend),
NULL);
+ /* The child property took a reference, we can safely drop ours now */
+ object_unref(OBJECT(vrng->conf.default_backend));
+
object_property_set_link(OBJECT(dev),
OBJECT(vrng->conf.default_backend),
"rng", NULL);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/4] qom: link property fixes
2014-03-05 9:40 [Qemu-devel] [PATCH v2 0/4] qom: link property fixes Stefan Hajnoczi
` (3 preceding siblings ...)
2014-03-05 9:40 ` [Qemu-devel] [PATCH v2 4/4] virtio-rng: avoid default_backend refcount leak Stefan Hajnoczi
@ 2014-03-05 13:29 ` Stefan Hajnoczi
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 13:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Andreas Faerber, Anthony Liguori, Paolo Bonzini
On Wed, Mar 05, 2014 at 10:40:24AM +0100, Stefan Hajnoczi wrote:
> v2:
> * A NULL set() callback should make the property read-only
This version does not address Paolo and Andreas' review comments.
I will send v3.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread