* [Qemu-devel] [PATCH v3 1/5] qom: split object_property_set_link()
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
@ 2014-03-05 20:51 ` Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 2/5] qom: don't make link NULL on object_property_set_link() failure Stefan Hajnoczi
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Faerber, Anthony Liguori, Peter Maydell
The path resolution logic in object_property_set_link() should be a
separate function. This makes the code easier to read and maintain.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qom/object.c | 59 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 20 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 660859c..6e2443c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1039,17 +1039,49 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
}
}
+/* 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)
+{
+ const char *type;
+ gchar *target_type;
+ bool ambiguous = false;
+ Object *target;
+
+ /* Go from link<FOO> to FOO. */
+ type = object_property_get_type(obj, name, NULL);
+ 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 {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+ }
+ target = NULL;
+ }
+ g_free(target_type);
+
+ return target;
+}
+
static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
Object **child = opaque;
Object *old_target;
- bool ambiguous = false;
- const char *type;
char *path;
- gchar *target_type;
-
- type = object_property_get_type(obj, name, NULL);
visit_type_str(v, &path, name, errp);
@@ -1059,24 +1091,11 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
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) {
+ target = object_resolve_link(obj, name, path, errp);
+ if (target) {
object_ref(target);
*child = target;
- } 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);
- }
}
- g_free(target_type);
}
g_free(path);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v3 2/5] qom: don't make link NULL on object_property_set_link() failure
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 1/5] qom: split object_property_set_link() Stefan Hajnoczi
@ 2014-03-05 20:51 ` Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 3/5] qom: make QOM link property unref optional Stefan Hajnoczi
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Faerber, Anthony Liguori, Peter Maydell
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.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qom/object.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 6e2443c..124d126 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1079,27 +1079,28 @@ static Object *object_resolve_link(Object *obj, const char *name,
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;
- char *path;
-
- visit_type_str(v, &path, name, errp);
+ Object *old_target = *child;
+ Object *new_target = NULL;
+ char *path = NULL;
- old_target = *child;
- *child = NULL;
+ visit_type_str(v, &path, name, &local_err);
- if (strcmp(path, "") != 0) {
- Object *target;
-
- target = object_resolve_link(obj, name, path, errp);
- if (target) {
- object_ref(target);
- *child = target;
- }
+ 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] 8+ messages in thread* [Qemu-devel] [PATCH v3 3/5] qom: make QOM link property unref optional
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 1/5] qom: split object_property_set_link() Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 2/5] qom: don't make link NULL on object_property_set_link() failure Stefan Hajnoczi
@ 2014-03-05 20:51 ` Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 4/5] qom: add check() argument to object_property_add_link() Stefan Hajnoczi
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Faerber, Anthony Liguori, Peter Maydell
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 a flags argument to object_property_add_link() so the
caller can specify which refcount behavior they require. The new
OBJ_PROP_LINK_UNREF_ON_RELEASE flag causes the link pointer to be
unreferenced when the property is deleted.
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 | 10 ++++++----
hw/dma/xilinx_axidma.c | 16 ++++++++++++----
hw/net/xilinx_axienet.c | 16 ++++++++++++----
hw/pcmcia/pxa2xx.c | 2 +-
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 | 11 ++++++++++-
qom/object.c | 36 +++++++++++++++++++++++++++++++++---
ui/console.c | 4 +++-
11 files changed, 85 insertions(+), 22 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c0b857f..1a90a67 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,8 +97,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
snprintf(name, sizeof(name), "child[%d]", kid->index);
object_property_add_link(OBJECT(bus), name,
object_get_typename(OBJECT(child)),
- (Object **)&kid->child,
- NULL);
+ (Object **)&kid->child, 0, NULL);
}
void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
@@ -761,7 +760,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, 0,
+ &error_abort);
}
static void device_post_init(Object *obj)
@@ -881,7 +881,9 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ 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..c8fda39 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,9 +537,13 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &local_errp);
object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
- (Object **)&cs->dma, &local_errp);
+ (Object **)&cs->dma,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &local_errp);
if (local_errp) {
goto xilinx_axidma_realize_fail;
}
@@ -571,10 +575,14 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &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..7ecf925 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -945,9 +945,13 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &local_errp);
object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
- (Object **) &cs->enet, &local_errp);
+ (Object **) &cs->enet,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &local_errp);
if (local_errp) {
goto xilinx_enet_realize_fail;
}
@@ -982,10 +986,14 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &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..6949214 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -198,7 +198,7 @@ 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, 0, 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..930b4f7 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE, 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..423ddb6 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
static Property virtio_ccw_rng_properties[] = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7b91841..eebb819 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index a16e3bc..2efda8b 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
static const TypeInfo virtio_rng_info = {
diff --git a/include/qom/object.h b/include/qom/object.h
index 9c7c361..52dc5cc 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1059,12 +1059,18 @@ Object *object_resolve_path_component(Object *parent, const gchar *part);
void object_property_add_child(Object *obj, const char *name,
Object *child, Error **errp);
+typedef enum {
+ /* Unref the link pointer when the property is deleted */
+ OBJ_PROP_LINK_UNREF_ON_RELEASE = 0x1,
+} ObjectPropertyLinkFlags;
+
/**
* 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
+ * @flags: additional options for the link
* @errp: if an error occurs, a pointer to an area to store the area
*
* Links establish relationships between objects. Links are unidirectional
@@ -1076,10 +1082,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 the
+ * <code>@flags</code> <code>OBJ_PROP_LINK_UNREF_ON_RELEASE</code> bit is set,
+ * 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,
+ ObjectPropertyLinkFlags flags,
Error **errp);
/**
diff --git a/qom/object.c b/qom/object.c
index 124d126..7f92cd2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,10 +1023,16 @@ out:
g_free(type);
}
+typedef struct {
+ Object **child;
+ ObjectPropertyLinkFlags flags;
+} 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) {
@@ -1080,7 +1086,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;
+ LinkProperty *prop = opaque;
+ Object **child = prop->child;
Object *old_target = *child;
Object *new_target = NULL;
char *path = NULL;
@@ -1106,18 +1113,41 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
}
}
+static void object_release_link_property(Object *obj, const char *name,
+ void *opaque)
+{
+ LinkProperty *prop = opaque;
+
+ if ((prop->flags & OBJ_PROP_LINK_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,
+ ObjectPropertyLinkFlags flags,
Error **errp)
{
+ Error *local_err = NULL;
+ LinkProperty *prop = g_malloc(sizeof(*prop));
gchar *full_type;
+ prop->child = child;
+ prop->flags = flags;
+
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..c6f460b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1178,7 +1178,9 @@ 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,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &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] 8+ messages in thread* [Qemu-devel] [PATCH v3 4/5] qom: add check() argument to object_property_add_link()
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
` (2 preceding siblings ...)
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 3/5] qom: make QOM link property unref optional Stefan Hajnoczi
@ 2014-03-05 20:51 ` Stefan Hajnoczi
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 5/5] virtio-rng: avoid default_backend refcount leak Stefan Hajnoczi
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Faerber, Anthony Liguori, Peter Maydell
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 check() argument to object_property_add_link().
Also provide default check() 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 | 8 ++++++--
hw/dma/xilinx_axidma.c | 4 ++++
hw/net/xilinx_axienet.c | 4 ++++
hw/pcmcia/pxa2xx.c | 4 +++-
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 | 18 ++++++++++++++++++
qom/object.c | 18 +++++++++++++++++-
ui/console.c | 1 +
13 files changed, 80 insertions(+), 4 deletions(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 77d0c66..c67acf5 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_allow_set_link_before_realize(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 1a90a67..cd34527 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,7 +97,10 @@ static void bus_add_child(BusState *bus, DeviceState *child)
snprintf(name, sizeof(name), "child[%d]", kid->index);
object_property_add_link(OBJECT(bus), name,
object_get_typename(OBJECT(child)),
- (Object **)&kid->child, 0, NULL);
+ (Object **)&kid->child,
+ NULL, /* read-only property */
+ 0, /* return ownership on prop deletion */
+ NULL);
}
void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
@@ -760,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, 0,
+ (Object **)&dev->parent_bus, NULL, 0,
&error_abort);
}
@@ -882,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_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
NULL);
}
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index c8fda39..14b887b 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -538,10 +538,12 @@ 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_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&local_errp);
object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
(Object **)&cs->dma,
+ object_property_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&local_errp);
if (local_errp) {
@@ -576,11 +578,13 @@ static void xilinx_axidma_init(Object *obj)
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
(Object **)&s->tx_data_dev,
+ qdev_prop_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
(Object **)&s->tx_control_dev,
+ qdev_prop_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&error_abort);
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 7ecf925..839d97c 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -946,10 +946,12 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
(Object **) &ds->enet,
+ object_property_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&local_errp);
object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
(Object **) &cs->enet,
+ object_property_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&local_errp);
if (local_errp) {
@@ -987,11 +989,13 @@ static void xilinx_enet_init(Object *obj)
object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
(Object **) &s->tx_data_dev,
+ qdev_prop_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&error_abort);
object_property_add_link(obj, "axistream-control-connected",
TYPE_STREAM_SLAVE,
(Object **) &s->tx_control_dev,
+ qdev_prop_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&error_abort);
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index 6949214..96f3774 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, 0, NULL);
+ (Object **)&s->card,
+ NULL, /* read-only property */
+ 0, 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 930b4f7..9c71afa 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_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 423ddb6..ad6c439 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_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index eebb819..ce97514 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_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 2efda8b..cbf0138 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_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
}
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0c0babf..68ddbce 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_allow_set_link_before_realize:
+ *
+ * Set the Error object if an attempt is made to set the link after realize.
+ * This function should be used as the check() argument to
+ * object_property_add_link().
+ */
+void qdev_prop_allow_set_link_before_realize(Object *obj, const char *name,
+ Object *val, Error **errp);
+
#endif
diff --git a/include/qom/object.h b/include/qom/object.h
index 52dc5cc..0363aeb 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1065,11 +1065,22 @@ typedef enum {
} ObjectPropertyLinkFlags;
/**
+ * object_property_allow_set_link:
+ *
+ * The default implementation of the object_property_add_link() check()
+ * callback function. It allows the link property to be set and never returns
+ * an error.
+ */
+void object_property_allow_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
+ * @check: callback to veto setting or NULL if the property is read-only
* @flags: additional options for the link
* @errp: if an error occurs, a pointer to an area to store the area
*
@@ -1079,6 +1090,11 @@ typedef enum {
*
* Links form the graph in the object model.
*
+ * The <code>@check()</code> callback is invoked when
+ * object_property_set_link() is called and can raise an error to prevent the
+ * link being set. If <code>@check</code> is NULL, the property is read-only
+ * and cannot be set.
+ *
* 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
@@ -1088,6 +1104,8 @@ typedef enum {
*/
void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
+ void (*check)(Object *obj, const char *name,
+ Object *val, Error **errp),
ObjectPropertyLinkFlags flags,
Error **errp);
diff --git a/qom/object.c b/qom/object.c
index 7f92cd2..a6db347 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,8 +1023,15 @@ out:
g_free(type);
}
+void object_property_allow_set_link(Object *obj, const char *name,
+ Object *val, Error **errp)
+{
+ /* Allow the link to be set, always */
+}
+
typedef struct {
Object **child;
+ void (*check)(Object *, const char *, Object *, Error **);
ObjectPropertyLinkFlags flags;
} LinkProperty;
@@ -1104,6 +1111,12 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
return;
}
+ prop->check(obj, name, new_target, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
if (new_target) {
object_ref(new_target);
}
@@ -1126,6 +1139,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 (*check)(Object *, const char *,
+ Object *, Error **),
ObjectPropertyLinkFlags flags,
Error **errp)
{
@@ -1134,13 +1149,14 @@ void object_property_add_link(Object *obj, const char *name,
gchar *full_type;
prop->child = child;
+ prop->check = check;
prop->flags = flags;
full_type = g_strdup_printf("link<%s>", type);
object_property_add(obj, name, full_type,
object_get_link_property,
- object_set_link_property,
+ check ? object_set_link_property : NULL,
object_release_link_property,
prop,
&local_err);
diff --git a/ui/console.c b/ui/console.c
index c6f460b..8b77ac5 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_allow_set_link,
OBJ_PROP_LINK_UNREF_ON_RELEASE,
&local_err);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v3 5/5] virtio-rng: avoid default_backend refcount leak
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
` (3 preceding siblings ...)
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 4/5] qom: add check() argument to object_property_add_link() Stefan Hajnoczi
@ 2014-03-05 20:51 ` Stefan Hajnoczi
2014-03-18 12:27 ` [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
2014-03-18 12:51 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-05 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Faerber, Anthony Liguori, Peter Maydell
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 cbf0138..b6ab361 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] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3 0/5] qom: link property fixes
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
` (4 preceding siblings ...)
2014-03-05 20:51 ` [Qemu-devel] [PATCH v3 5/5] virtio-rng: avoid default_backend refcount leak Stefan Hajnoczi
@ 2014-03-18 12:27 ` Stefan Hajnoczi
2014-03-18 12:51 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-18 12:27 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Peter Maydell, Paolo Bonzini, qemu-devel, Anthony Liguori,
Andreas Faerber
On Wed, Mar 05, 2014 at 09:51:19PM +0100, Stefan Hajnoczi wrote:
> v3:
> * Split first commit for easier reviewing [afaerber]
> * Turn bool argument into flags bitmask [pbonzini]
> * Rename set() callback to check() [pbonzini]
> * Rename object_property_default_set_link() to
> object_property_allow_set_link()
> * Rename qdev_prop_default_set_link() to
> qdev_prop_allow_set_link_before_realize()
>
> v2:
> * A NULL set() callback should make the property read-only
>
> There are two problems with QOM link properties:
>
> 1. There are refcount leaks in many object_property_add_link() callers.
> 2. There is no way to prevent link properties from being set after a device is
> realized.
>
> This patch series fixes these issues by extending the
> object_property_add_link() function.
>
> Stefan Hajnoczi (5):
> qom: split object_property_set_link()
> qom: don't make link NULL on object_property_set_link() failure
> qom: make QOM link property unref optional
> qom: add check() argument to object_property_add_link()
> virtio-rng: avoid default_backend refcount leak
>
> hw/core/qdev-properties.c | 12 ++++
> hw/core/qdev.c | 10 +++-
> hw/dma/xilinx_axidma.c | 20 +++++--
> hw/net/xilinx_axienet.c | 20 +++++--
> hw/pcmcia/pxa2xx.c | 4 +-
> hw/s390x/s390-virtio-bus.c | 4 +-
> hw/s390x/virtio-ccw.c | 4 +-
> hw/virtio/virtio-pci.c | 4 +-
> hw/virtio/virtio-rng.c | 7 ++-
> include/hw/qdev-properties.h | 11 ++++
> include/qom/object.h | 29 +++++++++-
> qom/object.c | 130 ++++++++++++++++++++++++++++++++-----------
> ui/console.c | 5 +-
> 13 files changed, 211 insertions(+), 49 deletions(-)
Ping?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3 0/5] qom: link property fixes
2014-03-05 20:51 [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
` (5 preceding siblings ...)
2014-03-18 12:27 ` [Qemu-devel] [PATCH v3 0/5] qom: link property fixes Stefan Hajnoczi
@ 2014-03-18 12:51 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-03-18 12:51 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Peter Maydell, Paolo Bonzini, qemu-devel, Anthony Liguori,
Andreas Faerber
On Wed, Mar 05, 2014 at 09:51:19PM +0100, Stefan Hajnoczi wrote:
> v3:
> * Split first commit for easier reviewing [afaerber]
> * Turn bool argument into flags bitmask [pbonzini]
> * Rename set() callback to check() [pbonzini]
> * Rename object_property_default_set_link() to
> object_property_allow_set_link()
> * Rename qdev_prop_default_set_link() to
> qdev_prop_allow_set_link_before_realize()
Since v3 no longer applies cleanly I have sent a rebased v4 series.
Stefan
^ permalink raw reply [flat|nested] 8+ messages in thread