Hi, Peter. Firstly, I appreciate your so careful and useful comments very much.
> -----Original Message----- > From: peter.crosthwa...@petalogix.com > [mailto:peter.crosthwa...@petalogix.com] On Behalf Of Peter Crosthwaite > Sent: Tuesday, August 26, 2014 10:25 AM > To: Gonglei (Arei) > Cc: qemu-devel@nongnu.org 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, <arei.gong...@huawei.com> wrote: > > From: Gonglei <arei.gong...@huawei.com> > > > > 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 <arei.gong...@huawei.com> > > --- > > 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 > > > > > >