From 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:55 +0100 Subject: qom: Make QOM link property unref optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Peter Crosthwaite Cc: Alexander Graf Cc: Anthony Liguori Cc: "Michael S. Tsirkin" Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Signed-off-by: Andreas Färber --- hw/core/qdev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'hw/core/qdev.c') diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 9f0a522ee8..a182917222 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) @@ -824,7 +823,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) @@ -944,7 +944,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); object_property_add_bool(obj, "realized", bus_get_realized, bus_set_realized, NULL); } -- cgit 1.4.1 From 39f72ef94ba74701d18daf82b44c18a60f94eb60 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:56 +0100 Subject: qom: Add check() argument to object_property_add_link() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Peter Crosthwaite Cc: Alexander Graf Cc: Anthony Liguori Cc: "Michael S. Tsirkin" Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini [AF: Tweaked documentation comment] Signed-off-by: Andreas Färber --- 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(-) (limited to 'hw/core/qdev.c') diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 77d0c66635..c67acf58b5 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 a182917222..97acf62906 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) @@ -823,7 +826,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); } @@ -945,6 +948,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); object_property_add_bool(obj, "realized", diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c index c8fda39122..14b887bfa8 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 7ecf9251a7..839d97ca86 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 6949214df3..96f377453d 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 930b4f7cd0..9c71afa031 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 aebb2dec6d..2bf0af8f0a 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1273,6 +1273,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 eebb819d98..ce97514b69 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 2efda8b443..cbf01389a2 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 3c000eea75..c46e908d71 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -204,4 +204,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 9feb441986..a641dcde10 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -1072,12 +1072,23 @@ typedef enum { OBJ_PROP_LINK_UNREF_ON_RELEASE = 0x1, } 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 * @@ -1087,6 +1098,11 @@ typedef enum { * * Links form the graph in the object model. * + * The @check() callback is invoked when + * object_property_set_link() is called and can raise an error to prevent the + * link being set. If @check 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 *@child is * managed by the property from after the function returns till the @@ -1096,6 +1112,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 9e22f11e21..f4de619b7b 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; @@ -1105,6 +1112,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); } @@ -1127,6 +1140,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) { @@ -1135,13 +1150,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 9974212409..e057755c04 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1181,6 +1181,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); object_property_add_uint32_ptr(obj, "head", -- cgit 1.4.1