[2/3] qdev: Refactor device_set_realized to avoid resource leak

From: Gonglei <arei.gonglei@huawei.com>

At present, the local variable local_err is reused at multi-places,
Which will cause resource leak in some scenarios.

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.gonglei@huawei.com>
---
 hw/core/qdev.c | 75 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 29 deletions(-)

On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@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.gonglei@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
>
>
>

> -----Original Message-----

> From: peter.crosthwaite@petalogix.com

> On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:

> > From: Gonglei <arei.gonglei@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.

> 

Hi, Peter. I know your mean. vmstate_unregister() also should be called,
beside dc->unrealized(). 

I will re-realize as your suggestion. Thanks!

Best regards,
-Gonglei