[Qemu-devel] [PATCH v6 0/3] Refactor device_set_realized to avoid resource leak

[Qemu-devel] [PATCH v6 0/3] Refactor device_set_realized to avoid resource leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

after committing
 [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

If devcie hotplugging failed, will casuse resource leak.
This patch series include address resouce leak and two other issuses.

v6 -> v5:
 - rework patch 2/3 by Peter's suggestion.
 - add 'Reviewed-by' tag.
 - remove a patch about PCIe hotplugging by Michael's idea.

v5 -> v4:
 - add patch 1/4 'Reviewed-by' tag.
 - change patch 2/4, propagate firsh child unrealizing failure, and
   change this patch's commit message.(Peter)

v4 -> v3:
 - add patch 2/4.(Thanks for Peter's suggestion)
 - rework patch 3/4 based on patch 2/4.

v3 -> v2:
 - add cleanup logic for set bus/child_bus realized/unrealized failed.
 - change patch 1/3 commit message, add 'Reviewed-by' tag.

v2 -> v1:
 - rewrite patch 1/3, using error_abort instead of local_err.
 - rewrite patch 2/3, add cleanup logic for different error embranchment.
 - rewrite title of patch 3/3, and a syntax fix.

Gonglei (3):
  qdev: using error_abort instead of using local_err
  qdev: using NULL instead of local_err for qbus_child unrealize
  qdev: add cleanup logic in device_set_realized() to avoid resource
    leak

 hw/core/qdev.c | 70 +++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 23 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v6 1/3] qdev: using error_abort instead of using local_err

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

This error can not happen normally. If it happens indicates
something very wrong, we should abort QEMU. moreover, The
user can only refer to /machine/peripheral, not
/machine/unattached.

Meanwhile remove superfluous check about local_err.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/core/qdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..4a1ac5b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     if (value && !dev->realized) {
-        if (!obj->parent && local_err == NULL) {
+        if (!obj->parent) {
             static int unattached_count;
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
             object_property_add_child(container_get(qdev_get_machine(),
                                                     "/unattached"),
-                                      name, obj, &local_err);
+                                      name, obj, &error_abort);
             g_free(name);
         }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v6 2/3] qdev: using NULL instead of local_err for qbus_child unrealize

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Forcefully unrealize all children regardless of early-iteration
errors(if happened). We should keep going with cleanup operation
rather than report a error immediately, meanwhile store the first
child unrealize failure and propagate it at the end. We also
forcefully un-vmsding and unrealizing actual object too.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/core/qdev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4a1ac5b..6f37cd3 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -871,18 +871,18 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         }
         dev->pending_deleted_event = false;
     } else if (!value && dev->realized) {
+        Error **local_errp = NULL;
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+            local_errp = local_err ? NULL : &local_err;
             object_property_set_bool(OBJECT(bus), false, "realized",
-                                     &local_err);
-            if (local_err != NULL) {
-                break;
-            }
+                                     local_errp);
         }
-        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) {
-            dc->unrealize(dev, &local_err);
+        if (dc->unrealize) {
+            local_errp = local_err ? NULL : &local_err;
+            dc->unrealize(dev, local_errp);
         }
         dev->pending_deleted_event = true;
     }
-- 
1.7.12.4

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

[arei.gonglei]

From: Gonglei <arei.gonglei@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.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/core/qdev.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6f37cd3..fcb1638 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,24 @@ 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",
+
+        QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+            object_property_set_bool(OBJECT(bus), true, "realized",
                                          &local_err);
-                if (local_err != NULL) {
-                    break;
-                }
+            if (local_err != NULL) {
+                goto child_realize_fail;
             }
         }
-        if (dev->hotplugged && local_err == NULL) {
+        if (dev->hotplugged) {
             device_reset(dev);
         }
         dev->pending_deleted_event = false;
@@ -888,11 +893,30 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
+        goto fail;
     }
 
     dev->realized = value;
+    return;
+
+child_realize_fail:
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        object_property_set_bool(OBJECT(bus), false, "realized",
+                                 NULL);
+    }
+
+    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);
+    return;
 }
 
 static bool device_get_hotpluggable(Object *obj, Error **errp)
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v6 0/3] Refactor device_set_realized to avoid resource leak

[Michael S. Tsirkin]

On Thu, Sep 04, 2014 at 10:18:23AM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> after committing
>  [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
> 
> If devcie hotplugging failed, will casuse resource leak.
> This patch series include address resouce leak and two other issuses.
> 
> v6 -> v5:
>  - rework patch 2/3 by Peter's suggestion.
>  - add 'Reviewed-by' tag.
>  - remove a patch about PCIe hotplugging by Michael's idea.
> 
> v5 -> v4:
>  - add patch 1/4 'Reviewed-by' tag.
>  - change patch 2/4, propagate firsh child unrealizing failure, and
>    change this patch's commit message.(Peter)
> 
> v4 -> v3:
>  - add patch 2/4.(Thanks for Peter's suggestion)
>  - rework patch 3/4 based on patch 2/4.
> 
> v3 -> v2:
>  - add cleanup logic for set bus/child_bus realized/unrealized failed.
>  - change patch 1/3 commit message, add 'Reviewed-by' tag.
> 
> v2 -> v1:
>  - rewrite patch 1/3, using error_abort instead of local_err.
>  - rewrite patch 2/3, add cleanup logic for different error embranchment.
>  - rewrite title of patch 3/3, and a syntax fix.
> 
> Gonglei (3):
>   qdev: using error_abort instead of using local_err
>   qdev: using NULL instead of local_err for qbus_child unrealize
>   qdev: add cleanup logic in device_set_realized() to avoid resource
>     leak

Applied and Cc qemu-stable, thanks!

>  hw/core/qdev.c | 70 +++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 47 insertions(+), 23 deletions(-)
> 
> -- 
> 1.7.12.4
> 
> 

Re: [Qemu-devel] [PATCH v6 0/3] Refactor device_set_realized to avoid resource leak

[Andreas Färber]

Am 04.09.2014 17:30, schrieb Michael S. Tsirkin:
> On Thu, Sep 04, 2014 at 10:18:23AM +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> after committing
>>  [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
>>
>> If devcie hotplugging failed, will casuse resource leak.
>> This patch series include address resouce leak and two other issuses.
>>
>> v6 -> v5:
>>  - rework patch 2/3 by Peter's suggestion.
>>  - add 'Reviewed-by' tag.
>>  - remove a patch about PCIe hotplugging by Michael's idea.
>>
>> v5 -> v4:
>>  - add patch 1/4 'Reviewed-by' tag.
>>  - change patch 2/4, propagate firsh child unrealizing failure, and
>>    change this patch's commit message.(Peter)
>>
>> v4 -> v3:
>>  - add patch 2/4.(Thanks for Peter's suggestion)
>>  - rework patch 3/4 based on patch 2/4.
>>
>> v3 -> v2:
>>  - add cleanup logic for set bus/child_bus realized/unrealized failed.
>>  - change patch 1/3 commit message, add 'Reviewed-by' tag.
>>
>> v2 -> v1:
>>  - rewrite patch 1/3, using error_abort instead of local_err.
>>  - rewrite patch 2/3, add cleanup logic for different error embranchment.
>>  - rewrite title of patch 3/3, and a syntax fix.
>>
>> Gonglei (3):
>>   qdev: using error_abort instead of using local_err
>>   qdev: using NULL instead of local_err for qbus_child unrealize
>>   qdev: add cleanup logic in device_set_realized() to avoid resource
>>     leak
> 
> Applied and Cc qemu-stable, thanks!

Objection, patch 1/3 is already on qom-next. Please make sure to sync
the commit message if you want to take it through your tree.

Thanks,
Andreas

> 
>>  hw/core/qdev.c | 70 +++++++++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 47 insertions(+), 23 deletions(-)
>>
>> -- 
>> 1.7.12.4
>>
>>


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v6 0/3] Refactor device_set_realized to avoid resource leak

[Michael S. Tsirkin]

On Thu, Sep 04, 2014 at 05:48:25PM +0200, Andreas Färber wrote:
> Am 04.09.2014 17:30, schrieb Michael S. Tsirkin:
> > On Thu, Sep 04, 2014 at 10:18:23AM +0800, arei.gonglei@huawei.com wrote:
> >> From: Gonglei <arei.gonglei@huawei.com>
> >>
> >> after committing
> >>  [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
> >>
> >> If devcie hotplugging failed, will casuse resource leak.
> >> This patch series include address resouce leak and two other issuses.
> >>
> >> v6 -> v5:
> >>  - rework patch 2/3 by Peter's suggestion.
> >>  - add 'Reviewed-by' tag.
> >>  - remove a patch about PCIe hotplugging by Michael's idea.
> >>
> >> v5 -> v4:
> >>  - add patch 1/4 'Reviewed-by' tag.
> >>  - change patch 2/4, propagate firsh child unrealizing failure, and
> >>    change this patch's commit message.(Peter)
> >>
> >> v4 -> v3:
> >>  - add patch 2/4.(Thanks for Peter's suggestion)
> >>  - rework patch 3/4 based on patch 2/4.
> >>
> >> v3 -> v2:
> >>  - add cleanup logic for set bus/child_bus realized/unrealized failed.
> >>  - change patch 1/3 commit message, add 'Reviewed-by' tag.
> >>
> >> v2 -> v1:
> >>  - rewrite patch 1/3, using error_abort instead of local_err.
> >>  - rewrite patch 2/3, add cleanup logic for different error embranchment.
> >>  - rewrite title of patch 3/3, and a syntax fix.
> >>
> >> Gonglei (3):
> >>   qdev: using error_abort instead of using local_err
> >>   qdev: using NULL instead of local_err for qbus_child unrealize
> >>   qdev: add cleanup logic in device_set_realized() to avoid resource
> >>     leak
> > 
> > Applied and Cc qemu-stable, thanks!
> 
> Objection, patch 1/3 is already on qom-next. Please make sure to sync
> the commit message if you want to take it through your tree.
> 
> Thanks,
> Andreas

No, it's ok, I'll drop it - but please remember to notify list
when you pick up patches (if you did and I missed this somehow,
my apologies). Also pls remember to add  Cc qemu-stable in your tree.


Other QOM related patches:
hw/machine: Free old values of string properties
and
virtio-pci: fix virtio-net child refcount in transports

would you like to pick them up as well?


> > 
> >>  hw/core/qdev.c | 70 +++++++++++++++++++++++++++++++++++++++-------------------
> >>  1 file changed, 47 insertions(+), 23 deletions(-)
> >>
> >> -- 
> >> 1.7.12.4
> >>
> >>
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v6 2/3] qdev: using NULL instead of local_err for qbus_child unrealize

[Andreas Färber]

Am 04.09.2014 04:18, schrieb arei.gonglei@huawei.com:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Forcefully unrealize all children regardless of early-iteration
> errors(if happened). We should keep going with cleanup operation
> rather than report a error immediately, meanwhile store the first
> child unrealize failure and propagate it at the end. We also
> forcefully un-vmsding and unrealizing actual object too.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/core/qdev.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4a1ac5b..6f37cd3 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -871,18 +871,18 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          }
>          dev->pending_deleted_event = false;
>      } else if (!value && dev->realized) {
> +        Error **local_errp = NULL;
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +            local_errp = local_err ? NULL : &local_err;
>              object_property_set_bool(OBJECT(bus), false, "realized",
> -                                     &local_err);
> -            if (local_err != NULL) {
> -                break;
> -            }
> +                                     local_errp);
>          }
> -        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) {
> -            dc->unrealize(dev, &local_err);
> +        if (dc->unrealize) {
> +            local_errp = local_err ? NULL : &local_err;
> +            dc->unrealize(dev, local_errp);
>          }
>          dev->pending_deleted_event = true;
>      }

This looks much cleaner now, thanks, applied.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg