From f5946dbab388050da6d9343978a38c81cce0508d Mon Sep 17 00:00:00 2001 From: Christian Borntraeger Date: Wed, 19 Mar 2014 12:24:27 +0100 Subject: [PATCH 1/7] vl.c: Fix memory leak in qemu_register_machine() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 261747f176f6 (vl: Use MachineClass instead of global QEMUMachine list) valgrind complains about the following: ==54082== 57 bytes in 3 blocks are definitely lost in loss record 365 of 729 ==54082== at 0x4031AFE: malloc (vg_replace_malloc.c:292) ==54082== by 0x4145569: g_malloc (in /usr/lib64/libglib-2.0.so.0.3400.2) ==54082== by 0x415F9E9: g_strconcat (in /usr/lib64/libglib-2.0.so.0.3400.2) ==54082== by 0x80157FE7: qemu_register_machine (vl.c:1597) ==54082== by 0x80208E6B: module_call_init (module.c:105) ==54082== by 0x80013B91: main (vl.c:3000) Turns out that valgrind is right. We simply forget the memory that g_strconcat() has allocated. Lets free it after the type_register(). We need a 2nd variable due to constness of the name part of the type structure. Signed-off-by: Christian Borntraeger Reviewed-by: Michael S. Tsirkin Signed-off-by: Andreas Färber --- vl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/vl.c b/vl.c index f0fe48b106..0e82f06989 100644 --- a/vl.c +++ b/vl.c @@ -1587,14 +1587,16 @@ static void machine_class_init(ObjectClass *oc, void *data) int qemu_register_machine(QEMUMachine *m) { + char *name = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL); TypeInfo ti = { - .name = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL), + .name = name, .parent = TYPE_MACHINE, .class_init = machine_class_init, .class_data = (void *)m, }; type_register(&ti); + g_free(name); return 0; } From c8897e8eb965c0d091683ffaf127c50f8231d048 Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Tue, 18 Mar 2014 17:26:35 +0200 Subject: [PATCH 2/7] vl.c: Fix OpenBSD compilation issue due to namespace collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Machine rewriting added MACHINE() macro which is already in use by other OpenBSD library. Since qemu/sockets.h exposes the OpenBSD namespace, the minimalistic approach is to add it as the first QEMU include. Reported-by: Brad Smith Signed-off-by: Marcel Apfelbaum Signed-off-by: Andreas Färber --- include/hw/boards.h | 1 + vl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index 7bd2ea7736..dd2c70da36 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -54,6 +54,7 @@ struct QEMUMachine { int qemu_register_machine(QEMUMachine *m); #define TYPE_MACHINE "machine" +#undef MACHINE /* BSD defines it and QEMU does not use it */ #define MACHINE(obj) \ OBJECT_CHECK(MachineState, (obj), TYPE_MACHINE) #define MACHINE_GET_CLASS(obj) \ diff --git a/vl.c b/vl.c index 0e82f06989..e2e2ac799e 100644 --- a/vl.c +++ b/vl.c @@ -58,6 +58,7 @@ int main(int argc, char **argv) #include +#include "qemu/sockets.h" #include "hw/hw.h" #include "hw/boards.h" #include "hw/usb.h" @@ -103,7 +104,6 @@ int main(int argc, char **argv) #include "disas/disas.h" -#include "qemu/sockets.h" #include "slirp/libslirp.h" From f5ec6704c73932291c303d0cf72352ac26411d0d Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:53 +0100 Subject: [PATCH 3/7] qom: Split object_property_set_link() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Paolo Bonzini Signed-off-by: Andreas Färber --- qom/object.c | 60 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/qom/object.c b/qom/object.c index a2a1ffa1b3..2877a00b81 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1039,17 +1039,50 @@ 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 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 +1092,11 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque, if (strcmp(path, "") != 0) { Object *target; - /* Go from link 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); From c6aed9833419eed9de19919ff31aa021a6171521 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:54 +0100 Subject: [PATCH 4/7] qom: Don't make link NULL on object_property_set_link() failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Paolo Bonzini Signed-off-by: Andreas Färber --- qom/object.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/qom/object.c b/qom/object.c index 2877a00b81..cc946d9dac 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1080,27 +1080,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; + Object *old_target = *child; + Object *new_target = NULL; + char *path = NULL; - visit_type_str(v, &path, name, errp); + visit_type_str(v, &path, name, &local_err); - old_target = *child; - *child = NULL; - - 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); } From 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:55 +0100 Subject: [PATCH 5/7] 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 ++++++---- 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 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); } diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c index 19f07b3b25..c8fda39122 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 0bd5eda199..7ecf9251a7 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 8f17596cc3..6949214df3 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 e4fc35366b..930b4f7cd0 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 a01801e342..aebb2dec6d 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1272,7 +1272,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 7b91841a1d..eebb819d98 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 a16e3bc52e..2efda8b443 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 4cd77049e4..9feb441986 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -1067,12 +1067,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 @@ -1084,10 +1090,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 *@child 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 + * @flags OBJ_PROP_LINK_UNREF_ON_RELEASE 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 cc946d9dac..9e22f11e21 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) { @@ -1081,7 +1087,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; @@ -1107,18 +1114,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 4df251d579..9974212409 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1180,7 +1180,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); object_property_add_uint32_ptr(obj, "head", &s->head, &local_err); From 39f72ef94ba74701d18daf82b44c18a60f94eb60 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:56 +0100 Subject: [PATCH 6/7] 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(-) 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", From abdffd1fb78c1b98bda925d3d59123beca6761a3 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 19 Mar 2014 08:58:57 +0100 Subject: [PATCH 7/7] virtio-rng: Avoid default_backend refcount leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Signed-off-by: Andreas Färber --- 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 cbf01389a2..b6ab3610cb 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);