Re: [Qemu-devel] [PATCH v2 2/3] qdev: add cleanup logic in device_set_realized() to avoid resource leak

[Peter Crosthwaite]

On Thu, Aug 21, 2014 at 12:11 PM,  <address@hidden> wrote:
> From: Gonglei <address@hidden>
>
> At present, this function doesn't have partial cleanup implemented,
> 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.
>
> Adding fuller cleanup logic which assure that function can
> goto appropriate error label as local_err population is
> detected as each relevant point.
>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  hw/core/qdev.c | 58 
> ++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4a1ac5b..c1a36f0 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,25 @@ 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",
> -                                         &local_err);
> -                if (local_err != NULL) {
> -                    break;
> -                }
> +
> +        QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +            object_property_set_bool(OBJECT(bus), true, "realized",
> +                                     &local_err);
> +            if (local_err != NULL) {
> +                goto set_bool_fail;

This is actually quite tricky to cleanup right, as if this borks part
way through the iterations wont you need to recursive unrealize any
children that have already been successfully realized?

>              }
>          }
> -        if (dev->hotplugged && local_err == NULL) {
> +
> +        if (dev->hotplugged) {
>              device_reset(dev);
>          }
>          dev->pending_deleted_event = false;
> @@ -875,24 +881,36 @@ static void device_set_realized(Object *obj, bool 
> value, Error **errp)
>              object_property_set_bool(OBJECT(bus), false, "realized",
>                                       &local_err);
>              if (local_err != NULL) {
> -                break;
> +                goto fail;
>              }

I'm starting the question the need for this break (or goto). If any
one of the child unrealizations fails, then the iteration stops,
inhibiting garbage collection (i..e one bad child, stops the cleanup
for whole set). Should subsequent iterations of the loop continue (but
you will need to set errp = NULL)?

About my comment above, you may be able to inspect the child realized
property, to recycle this loop as the cleanup needed for partial child
realization fail. That is, always loop through the whole list and if
the child is is realized, unrealize it.

>          }
> -        if (qdev_get_vmsd(dev) && local_err == NULL) {
> +        if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }

>From here on seems very similar to the fail conditions now. Is there
code share opportunity? - to just jump to what you have labelled as
set_bool_fail and let the error handler implement the unrealization
process? If the realize success path returns from inside the if, then
you don't need a success path goto in unrealize path either. Here's
the basic idea:

if (doing_realize) {
   foo(&err);
   if (err) goto foo_fail;
   bar(&err);
   if (err) goto bar_fail;
   baz(&err);
   if (err) goto baz_fail;
   dc->realized = true;
   return;
} /* else - doing an unrealize */
   un_baz();
baz_fail:
   un_bar();
bar_fail:
   un_foo();
foo_fail:
   dc->realized = false;
return;

> -        if (dc->unrealize && local_err == NULL) {
> +        if (dc->unrealize) {
>              dc->unrealize(dev, &local_err);
>          }
>          dev->pending_deleted_event = true;
> -    }
>
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return;
> +        if (local_err != NULL) {
> +            goto fail;
> +        }
>      }
>
>      dev->realized = value;
> +    return;
> +
> +set_bool_fail:
> +    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);

You would then conditionalise error propagation like in original code.

Regards,
Peter

> +    return;
>  }
>
>  static bool device_get_hotpluggable(Object *obj, Error **errp)
> --
> 1.7.12.4
>
>
>