[Qemu-devel] [PATCH v5 0/4] Refactor device_set_realized to avoid resource leak.

[Qemu-devel] [PATCH v5 0/4] 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.

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 (4):
  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
  pcie: don't assert when hotplug a PCIe device with 'function != 0'

 hw/core/qdev.c | 69 +++++++++++++++++++++++++++++++++++++++++++---------------
 hw/pci/pcie.c  |  6 ++++-
 2 files changed, 56 insertions(+), 19 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v5 1/4] 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 v5 2/4] 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.
Because we can not guarantee that a botched recursive has no
side-effects.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4a1ac5b..c869520 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -813,6 +813,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     BusState *bus;
     Error *local_err = NULL;
+    Error *child_unrealized_err = NULL;
 
     if (dev->hotplugged && !dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
@@ -875,13 +876,17 @@ 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;
+                if (child_unrealized_err == NULL) {
+                    child_unrealized_err = error_copy(local_err);
+                }
+                error_free(local_err);
+                local_err = NULL;
             }
         }
-        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;
@@ -889,6 +894,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
     if (local_err != NULL) {
         error_propagate(errp, local_err);
+        error_free(child_unrealized_err);
+        return;
+    } else if (child_unrealized_err != NULL) {
+        error_propagate(errp, child_unrealized_err);
         return;
     }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v5 3/4] 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>
---
 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 c869520..4a0f36a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -835,12 +835,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);
@@ -853,21 +855,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;
@@ -893,15 +898,34 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     if (local_err != NULL) {
-        error_propagate(errp, local_err);
         error_free(child_unrealized_err);
-        return;
+        goto fail;
     } else if (child_unrealized_err != NULL) {
         error_propagate(errp, child_unrealized_err);
         return;
     }
 
     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

[Qemu-devel] [PATCH v5 4/4] pcie: don't assert when hotplug a PCIe device with 'function != 0'

[arei.gonglei]

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

It's enough to report an error. Assert() is not acceptable
because the error is not a fatal error.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/pcie.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 1babddf..ab7f8a2 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
      * Right now, only a device of function = 0 is allowed to be
      * hot plugged/unplugged.
      */
-    assert(PCI_FUNC(pci_dev->devfn) == 0);
+    if (PCI_FUNC(pci_dev->devfn) != 0) {
+        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
+                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
+        return;
+    }
 
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                PCI_EXP_SLTSTA_PDS);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v5 0/4] Refactor device_set_realized to avoid resource leak.

[Michael S. Tsirkin]

On Tue, Sep 02, 2014 at 08:03:04PM +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


Patches  1-3 make sense for qemu-stable?

Andreas could you please comment and if yes pick them
up and add Cc qemu-stable?

2.1.1 freeze is tomorrow.

> If devcie hotplugging failed, will casuse resource leak.
> This patch series include address resouce leak and two other issuses.
> 
> 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 (4):
>   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
>   pcie: don't assert when hotplug a PCIe device with 'function != 0'
> 
>  hw/core/qdev.c | 69 +++++++++++++++++++++++++++++++++++++++++++---------------
>  hw/pci/pcie.c  |  6 ++++-
>  2 files changed, 56 insertions(+), 19 deletions(-)
> 
> -- 
> 1.7.12.4
> 

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

[Peter Crosthwaite]

On Tue, Sep 2, 2014 at 10:03 PM,  <arei.gonglei@huawei.com> wrote:
> 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 c869520..4a0f36a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -835,12 +835,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);
> @@ -853,21 +855,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;
> @@ -893,15 +898,34 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      }
>
>      if (local_err != NULL) {
> -        error_propagate(errp, local_err);
>          error_free(child_unrealized_err);
> -        return;
> +        goto fail;
>      } else if (child_unrealized_err != NULL) {
>          error_propagate(errp, child_unrealized_err);
>          return;
>      }
>
>      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 v5 2/4] qdev: using NULL instead of local_err for qbus_child unrealize

[Peter Crosthwaite]

On Tue, Sep 2, 2014 at 10:03 PM,  <arei.gonglei@huawei.com> wrote:
> 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.
> Because we can not guarantee that a botched recursive has no
> side-effects.
>

This last sentance doesn't make too much sense to me, but the rest of
the commit message covers it nicely so I think you can drop it.

> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4a1ac5b..c869520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -813,6 +813,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      BusState *bus;
>      Error *local_err = NULL;
> +    Error *child_unrealized_err = NULL;
>
>      if (dev->hotplugged && !dc->hotpluggable) {
>          error_set(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -875,13 +876,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              object_property_set_bool(OBJECT(bus), false, "realized",
>                                       &local_err);

I think you can make the concept of "pass in a errp only if I don't
have one already" simpler with this pattern:

Error **local_errp = local_err ? NULL : &local_err;
object_property_set_bool(... , local_errp)

Or if your into one-line killers you could do the ?: inline (although
some reviewers don't like that).

>              if (local_err != NULL) {
> -                break;
> +                if (child_unrealized_err == NULL) {
> +                    child_unrealized_err = error_copy(local_err);
> +                }
> +                error_free(local_err);
> +                local_err = NULL;
>              }

Then this whole if can go ...

>          }
> -        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) {

local_errp = local_err ? NULL : &local_err;

>              dc->unrealize(dev, &local_err);
>          }
>          dev->pending_deleted_event = true;
> @@ -889,6 +894,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> +        error_free(child_unrealized_err);
> +        return;
> +    } else if (child_unrealized_err != NULL) {
> +        error_propagate(errp, child_unrealized_err);

Then this if-else is not needed either.

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

>          return;
>      }
>
> --
> 1.7.12.4
>
>
>

Re: [Qemu-devel] [PATCH v5 4/4] pcie: don't assert when hotplug a PCIe device with 'function != 0'

[Michael S. Tsirkin]

On Tue, Sep 02, 2014 at 08:03:08PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> It's enough to report an error. Assert() is not acceptable
> because the error is not a fatal error.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>


I think it's an unrelated issue, don't send this
as part of a bugfix qdev patchset pls.

> ---
>  hw/pci/pcie.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 1babddf..ab7f8a2 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>       * Right now, only a device of function = 0 is allowed to be
>       * hot plugged/unplugged.
>       */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> +    if (PCI_FUNC(pci_dev->devfn) != 0) {
> +        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
> +                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
> +        return;
> +    }
>  
>      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                 PCI_EXP_SLTSTA_PDS);
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v5 1/4] qdev: using error_abort instead of using local_err

[Andreas Färber]

Am 02.09.2014 14:03, schrieb arei.gonglei@huawei.com:
> 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);
>          }
>  

One way to trigger this abort would be for the user to add a property of
that format via -object or similar mechanisms, when allowing a custom
path. Today, objects are always under /objects though.

In fact the only way to get an Error** back from
object_property_add[_child]() is if a duplicate property exists. Any
other error conditions would already abort in GLib code.

Anyway, this is not strictly an error fix and it's a question of how
much we want to protect the user from his own stupidity, so I'm leaning
towards applying this patch. I'd prefer to decouple it from the other
cleanup though.

Regards,
Andreas

-- 
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 v5 4/4] pcie: don't assert when hotplug a PCIe device with 'function != 0'

[Gonglei (Arei)]

> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Wednesday, September 03, 2014 9:48 PM
> Subject: Re: [PATCH v5 4/4] pcie: don't assert when hotplug a PCIe device with
> 'function != 0'
> 
> On Tue, Sep 02, 2014 at 08:03:08PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > It's enough to report an error. Assert() is not acceptable
> > because the error is not a fatal error.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> 
> 
> I think it's an unrelated issue, don't send this
> as part of a bugfix qdev patchset pls.
> 
Got it. Thanks

Best regards,
-Gonglei

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

[Gonglei (Arei)]

> From: peter.crosthwaite@petalogix.com
> [mailto:peter.crosthwaite@petalogix.com] On Behalf Of Peter Crosthwaite
> Sent: Wednesday, September 03, 2014 9:09 PM
g NULL instead of local_err
> for qbus_child unrealize
> 
> On Tue, Sep 2, 2014 at 10:03 PM,  <arei.gonglei@huawei.com> wrote:
> > 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.
> > Because we can not guarantee that a botched recursive has no
> > side-effects.
> >
> 
> This last sentance doesn't make too much sense to me, but the rest of
> the commit message covers it nicely so I think you can drop it.
> 
OK.

> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/core/qdev.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 4a1ac5b..c869520 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -813,6 +813,7 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >      BusState *bus;
> >      Error *local_err = NULL;
> > +    Error *child_unrealized_err = NULL;
> >
> >      if (dev->hotplugged && !dc->hotpluggable) {
> >          error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> object_get_typename(obj));
> > @@ -875,13 +876,17 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
> >              object_property_set_bool(OBJECT(bus), false, "realized",
> >                                       &local_err);
> 
> I think you can make the concept of "pass in a errp only if I don't
> have one already" simpler with this pattern:
> 
> Error **local_errp = local_err ? NULL : &local_err;
> object_property_set_bool(... , local_errp)
> 
OK, this is a good idea!

> Or if your into one-line killers you could do the ?: inline (although
> some reviewers don't like that).
> 
> >              if (local_err != NULL) {
> > -                break;
> > +                if (child_unrealized_err == NULL) {
> > +                    child_unrealized_err = error_copy(local_err);
> > +                }
> > +                error_free(local_err);
> > +                local_err = NULL;
> >              }
> 
> Then this whole if can go ...
> 
> >          }
> > -        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) {
> 
> local_errp = local_err ? NULL : &local_err;
> 
> >              dc->unrealize(dev, &local_err);
> >          }
> >          dev->pending_deleted_event = true;
> > @@ -889,6 +894,10 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
> >
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> > +        error_free(child_unrealized_err);
> > +        return;
> > +    } else if (child_unrealized_err != NULL) {
> > +        error_propagate(errp, child_unrealized_err);
> 
> Then this if-else is not needed either.
> 
Yes.

> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
Thanks. Will fix it!

Best regards,
-Gonglei

> Regards,
> Peter
> 
> >          return;
> >      }
> >
> > --
> > 1.7.12.4
> >
> >
> >

Re: [Qemu-devel] [PATCH v5 1/4] qdev: using error_abort instead of using local_err

[Gonglei (Arei)]

Hi,

> Subject: Re: [Qemu-devel] [PATCH v5 1/4] qdev: using error_abort instead of
> using local_err
> 
> Am 02.09.2014 14:03, schrieb arei.gonglei@huawei.com:
> > 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);
> >          }
> >
> 
> One way to trigger this abort would be for the user to add a property of
> that format via -object or similar mechanisms, when allowing a custom
> path. Today, objects are always under /objects though.
> 
> In fact the only way to get an Error** back from
> object_property_add[_child]() is if a duplicate property exists. Any
> other error conditions would already abort in GLib code.
> 
> Anyway, this is not strictly an error fix and it's a question of how
> much we want to protect the user from his own stupidity, so I'm leaning
> towards applying this patch. I'd prefer to decouple it from the other
> cleanup though.
> 
I'll post v6, rework patch 2/4 by Peter's suggestion, add 'Reviewed-by'
and remove patch 4/4 by Michael's idea. :)

Please consider to apply v6. Thanks!

Best regards,
-Gonglei

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