> From: peter.crosthwa...@petalogix.com > [mailto:peter.crosthwa...@petalogix.com] On Behalf Of Peter Crosthwaite > Sent: Tuesday, August 26, 2014 10:25 AM > 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? > > > } > > } > > - 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; > Hi, Peter.
I'm reworking this patch now. I think your above idea will lapse from the Original purpose of the device_set_realized(). Because: 1) the logic check two conditions, including value argument and dev->realized. If/else cannot descript the logic completely, but if/else if. 2) what we do when object_property_set_bool(OBJECT(bus), false, "realized", &local_err) failed, if we share this code? This is a trouble. So, I will send v3, please review it again. I add a set_realized/unrealized_fail Cleanup logic as your suggestion, but don't share code for unrealized. Thanks! Best regards, -Gonglei
