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

[Gonglei (Arei)]

Hi, Peter.

Firstly, I appreciate your so careful and useful comments very much.

> -----Original Message-----
> From: address@hidden
> [mailto:address@hidden On Behalf Of Peter Crosthwaite
> Sent: Tuesday, August 26, 2014 10:25 AM
> To: Gonglei (Arei)
> Cc: address@hidden Developers; Huangweidong (C); Michael S. Tsirkin;
> Luonengjun; Huangpeng (Peter); Igor Mammedov; Paolo Bonzini; Andreas
> Färber
> Subject: Re: [Qemu-devel] [PATCH v2 2/3] qdev: add cleanup logic in
> device_set_realized() to avoid resource leak
> 
> 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?
> 
Yes. I think we should unrealize those children.

> >              }
> >          }
> > -        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)?
> 
IMHO, continue to cleanup for other children maybe a good idea.

> 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.
> 
Yes.

> >          }
> > -        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 we share those code, whether it will people feel puzzled? 
When the normal unrealized operation changed into error handler
implementation.

Best regards,
-Gonglei

> 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
> >
> >
> >