From d578029e71311de1b1476229d88d4aca02b783a3 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Tue, 2 Sep 2014 20:03:05 +0800 Subject: qdev: Use error_abort instead of using local_err MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This error can not happen normally. If it happens, it indicates something very wrong, we should abort QEMU. Moreover, the user can only refer to /machine/peripheral or /objects, not /machine/unattached. While at it, remove superfluous check about local_err. Signed-off-by: Gonglei Reviewed-by: Peter Crosthwaite Signed-off-by: Andreas Färber --- hw/core/qdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'hw/core/qdev.c') diff --git a/hw/core/qdev.c b/hw/core/qdev.c index da1ba48..4a1ac5b 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (value && !dev->realized) { - if (!obj->parent && local_err == NULL) { + if (!obj->parent) { static int unattached_count; gchar *name = g_strdup_printf("device[%d]", unattached_count++); object_property_add_child(container_get(qdev_get_machine(), "/unattached"), - name, obj, &local_err); + name, obj, &error_abort); g_free(name); } -- cgit v1.1 From cd4520adcab70dbac8db3fe4d41836dca63715a4 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Thu, 4 Sep 2014 10:18:25 +0800 Subject: qdev: Use NULL instead of local_err for qbus_child unrealize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forcefully unrealize all children regardless of errors in earlier iterations (if any). We should keep going with cleanup operation rather than report an error immediately. Therefore store the first child unrealization failure and propagate it at the end. We also forcefully unregister vmsd and unrealize actual object, too. Signed-off-by: Gonglei Reviewed-by: Peter Crosthwaite Cc: qemu-stable@nongnu.org Signed-off-by: Andreas Färber --- hw/core/qdev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'hw/core/qdev.c') diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 4a1ac5b..6f37cd3 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -871,18 +871,18 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } dev->pending_deleted_event = false; } else if (!value && dev->realized) { + Error **local_errp = NULL; QLIST_FOREACH(bus, &dev->child_bus, sibling) { + local_errp = local_err ? NULL : &local_err; object_property_set_bool(OBJECT(bus), false, "realized", - &local_err); - if (local_err != NULL) { - break; - } + local_errp); } - if (qdev_get_vmsd(dev) && local_err == NULL) { + if (qdev_get_vmsd(dev)) { vmstate_unregister(dev, qdev_get_vmsd(dev), dev); } - if (dc->unrealize && local_err == NULL) { - dc->unrealize(dev, &local_err); + if (dc->unrealize) { + local_errp = local_err ? NULL : &local_err; + dc->unrealize(dev, local_errp); } dev->pending_deleted_event = true; } -- cgit v1.1 From 1d45a705fc007a13f20d18473290082eae6d1725 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Thu, 4 Sep 2014 10:18:26 +0800 Subject: qdev: Add cleanup logic in device_set_realized() to avoid resource leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At present, this function doesn't have partial cleanup implemented, which will cause resource leaks in some scenarios. Example: 1. Assume that "dc->realize(dev, &local_err)" executes successful and local_err == NULL; 2. device hotplug in hotplug_handler_plug() executes but fails (it is prone to occur). Then local_err != NULL; 3. error_propagate(errp, local_err) and return. But the resources which have been allocated in dc->realize() will be leaked. Simple backtrace: dc->realize() |->device_realize |->pci_qdev_init() |->do_pci_register_device() |->etc. Add fuller cleanup logic which assures that function can goto appropriate error label as local_err population is detected at each relevant point. Signed-off-by: Gonglei Reviewed-by: Peter Crosthwaite Cc: qemu-stable@nongnu.org Signed-off-by: Andreas Färber --- hw/core/qdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 14 deletions(-) (limited to 'hw/core/qdev.c') diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 6f37cd3..fcb1638 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -834,12 +834,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp) dc->realize(dev, &local_err); } - if (dev->parent_bus && dev->parent_bus->hotplug_handler && - local_err == NULL) { + if (local_err != NULL) { + goto fail; + } + + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); - } else if (local_err == NULL && - object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { + } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { HotplugHandler *hotplug_ctrl; MachineState *machine = MACHINE(qdev_get_machine()); MachineClass *mc = MACHINE_GET_CLASS(machine); @@ -852,21 +854,24 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } } - if (qdev_get_vmsd(dev) && local_err == NULL) { + if (local_err != NULL) { + goto post_realize_fail; + } + + if (qdev_get_vmsd(dev)) { vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, dev->instance_id_alias, dev->alias_required_for_version); } - if (local_err == NULL) { - QLIST_FOREACH(bus, &dev->child_bus, sibling) { - object_property_set_bool(OBJECT(bus), true, "realized", + + QLIST_FOREACH(bus, &dev->child_bus, sibling) { + object_property_set_bool(OBJECT(bus), true, "realized", &local_err); - if (local_err != NULL) { - break; - } + if (local_err != NULL) { + goto child_realize_fail; } } - if (dev->hotplugged && local_err == NULL) { + if (dev->hotplugged) { device_reset(dev); } dev->pending_deleted_event = false; @@ -888,11 +893,30 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (local_err != NULL) { - error_propagate(errp, local_err); - return; + goto fail; } dev->realized = value; + return; + +child_realize_fail: + QLIST_FOREACH(bus, &dev->child_bus, sibling) { + object_property_set_bool(OBJECT(bus), false, "realized", + NULL); + } + + if (qdev_get_vmsd(dev)) { + vmstate_unregister(dev, qdev_get_vmsd(dev), dev); + } + +post_realize_fail: + if (dc->unrealize) { + dc->unrealize(dev, NULL); + } + +fail: + error_propagate(errp, local_err); + return; } static bool device_get_hotpluggable(Object *obj, Error **errp) -- cgit v1.1