On Tue, Aug 19, 2014 at 7:41 PM, <arei.gong...@huawei.com> wrote:
> From: Gonglei <arei.gong...@huawei.com>
>
> At present, the local variable local_err is reused at multi-places,
> Which will cause resource leak in some scenarios.
>
The problem isn't really the local_err reusage. It's the fact that
this function doesn't have partial cleanup implemented (the
dc->unrealize call you add here is needed but not in original code at
all). Doing a fuller audit of the function, it seems to have outgrown
the simplistic if (!local_err) approach to error handling. I think the
goto-fallthrough system might be a cleaner alternative. Perhaps finish
the fn with:
dev->realized = value;
return;
post_realize_fail:
if (dc->unrealize) {
dc->unrealize(dev, NULL);
}
fail:
if (local_err != NULL) {
error_propagate(errp, local_err);
return;
}
}
Then goto the appropriate error label as local_err population is
detected as each relevant point.
Regards,
Peter
> Example:
>
> 1. Assuming that "dc->realize(dev, &local_err)" execute successful
> and local_err == NULL;
> 2. Executing device hotplug in hotplug_handler_plug(), but failed
> (It is prone to occur). Then local_err != NULL;
> 3. error_propagate(errp, local_err) and return. But the resources
> which been allocated in dc->realize() will be leaked.
> Simple backtrace:
> dc->realize()
> |->device_realize
> |->pci_qdev_init()
> |->do_pci_register_device()
> |->etc.
>
> To avoid the resource leak, using some different local error variables.
> For different error conditions, release the corresponding resources.
>
> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> ---
> hw/core/qdev.c | 75
> +++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3e7085e..b3a463b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -839,41 +839,58 @@ 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) {
> - hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> - dev, &local_err);
> - } else if (local_err == NULL &&
> - object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> - HotplugHandler *hotplug_ctrl;
> - MachineState *machine = MACHINE(qdev_get_machine());
> - MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> - if (mc->get_hotplug_handler) {
> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> - if (hotplug_ctrl) {
> - hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> + if (local_err == NULL) {
> + Error *hotplug_err = NULL;
> +
> + if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> + hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> + dev, &hotplug_err);
> + } 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);
> +
> + if (mc->get_hotplug_handler) {
> + hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> + if (hotplug_ctrl) {
> + hotplug_handler_plug(hotplug_ctrl, dev,
> &hotplug_err);
> + }
> }
> }
> - }
>
> - if (qdev_get_vmsd(dev) && local_err == NULL) {
> - 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",
> - &local_err);
> - if (local_err != NULL) {
> - break;
> + if (hotplug_err == NULL) {
> + Error *err = NULL;
> + 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);
> }
> +
> + QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> + object_property_set_bool(OBJECT(bus), true, "realized",
> + &err);
> + if (err != NULL) {
> + if (qdev_get_vmsd(dev)) {
> + vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
> + }
> +
> + break;
> + }
> + }
> +
> + if (dev->hotplugged && err == NULL) {
> + device_reset(dev);
> + }
> + error_free(err);
> + } else {
> + if (dc->unrealize) {
> + dc->unrealize(dev, NULL);
> + }
> +
> + error_propagate(errp, hotplug_err);
> + return;
> }
> }
> - if (dev->hotplugged && local_err == NULL) {
> - device_reset(dev);
> - }
> dev->pending_deleted_event = false;
> } else if (!value && dev->realized) {
> QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> --
> 1.7.12.4
>
>
>
