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

[Gonglei (Arei)]

> -----Original Message-----
> From: address@hidden
> [mailto:address@hidden On Behalf Of Peter Crosthwaite
> Sent: Monday, September 01, 2014 2:08 PM
> 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 v3 2/3] qdev: add cleanup logic in
> device_set_realized() to avoid resource leak
> 
> On Mon, Sep 1, 2014 at 3:50 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 | 73
> ++++++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 53 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 4a1ac5b..2a82cc1 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_realized_fail;
> 
> should this label be child_realize_fail for clarity? The object is
> already realized by now so this name unqualified is a little
> misleading.
> 
> >              }
> >          }
> > -        if (dev->hotplugged && local_err == NULL) {
> > +
> > +        if (dev->hotplugged) {
> >              device_reset(dev);
> >          }
> >          dev->pending_deleted_event = false;
> > @@ -875,24 +881,51 @@ 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 set_unrealized_fail;
> >              }
> >          }
> > -        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) {
> > +        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_unrealized_fail:
> 
> child_unrealize_fail
> 
> > +    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> > +        object_property_set_bool(OBJECT(bus), true, "realized",
> 
> "true" doesn't look right to me. It means if the recursive unrealize
> botches it will re-realize all children? The idea behind my comment
> last spin was to forcefully unrealize all children regardless of
> early-iteration errors. I think set_realized_fail and set
> unrealized_fail should be the same logic. You should keep going with
> your cleanup on error rather than undo the cleanup already done.
> 
I knew your mean, but forgot to explain in pervious email, sorry for this.
When one child unrealized failed, the interface will return error for user,
so user think the device existed yet in QEMU, but not hotplugged.
If we keep going with cleanup, the state saved by QEMU is not same with user's 
thought. 

Best regards,
-Gonglei