[Qemu-devel] [PULL for-2.1 0/7] QOM devices patch queue 2014-09-04

[Qemu-devel] [PULL for-2.1 0/7] QOM devices patch queue 2014-09-04

[Andreas Färber]

Hello Peter,

This is my QOM (devices) patch queue. Please pull.

Regards,
Andreas

Cc: Peter Maydell <peter.maydell@linaro.org>

Cc: Michael S. Tsirkin <mst@redhat.com>

The following changes since commit 01eb313907dda97313b8fea62e5632fca64f069c:

  Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-2014-09-03' into staging (2014-09-04 13:33:53 +0100)

are available in the git repository at:


  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter

for you to fetch changes up to 1d45a705fc007a13f20d18473290082eae6d1725:

  qdev: Add cleanup logic in device_set_realized() to avoid resource leak (2014-09-04 19:15:54 +0200)

----------------------------------------------------------------
QOM infrastructure fixes and device conversions

* Cleanups for recursive device unrealization

----------------------------------------------------------------
Alexey Kardashevskiy (1):
      qom: Make object_child_foreach() safe for objects removal

Andreas Färber (1):
      machine: Clean up -machine handling

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

Peter Crosthwaite (2):
      qom: Add automatic arrayification to object_property_add()
      memory: Remove object_property_add_child_array()

 hw/core/qdev.c | 70 +++++++++++++++++++++++++++++++++++++++-------------------
 memory.c       | 30 +++++--------------------
 qom/object.c   | 25 +++++++++++++++++++--
 vl.c           |  8 +++----
 4 files changed, 79 insertions(+), 54 deletions(-)

[Qemu-devel] [PULL 1/7] qom: Make object_child_foreach() safe for objects removal

[Andreas Färber]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

Current object_child_foreach() uses QTAILQ_FOREACH() to walk
through children and that makes children removal from the callback
impossible.

This makes object_child_foreach() use QTAILQ_FOREACH_SAFE().

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qom/object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 79bd0cc..a298b32 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -668,10 +668,10 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
 int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
                          void *opaque)
 {
-    ObjectProperty *prop;
+    ObjectProperty *prop, *next;
     int ret = 0;
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    QTAILQ_FOREACH_SAFE(prop, &obj->properties, node, next) {
         if (object_property_is_child(prop)) {
             ret = fn(prop->opaque, opaque);
             if (ret != 0) {
-- 
1.8.4.5

[Qemu-devel] [PULL 2/7] machine: Clean up -machine handling

[Andreas Färber]

Since commit c4090f8, -object options are no longer handled through
object_set_property(), so clean up -object leftovers by renaming the
function and dropping special-casing of qom-type and id properties.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 vl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index a9fd207..9c9acf5 100644
--- a/vl.c
+++ b/vl.c
@@ -2841,15 +2841,15 @@ static void free_and_trace(gpointer mem)
     free(mem);
 }
 
-static int object_set_property(const char *name, const char *value, void *opaque)
+static int machine_set_property(const char *name, const char *value,
+                                void *opaque)
 {
     Object *obj = OBJECT(opaque);
     StringInputVisitor *siv;
     Error *local_err = NULL;
     char *c, *qom_name;
 
-    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 ||
-        strcmp(name, "type") == 0) {
+    if (strcmp(name, "type") == 0) {
         return 0;
     }
 
@@ -4254,7 +4254,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     machine_opts = qemu_get_machine_opts();
-    if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
+    if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
                          1) < 0) {
         object_unref(OBJECT(current_machine));
         exit(1);
-- 
1.8.4.5

[Qemu-devel] [PULL 3/7] qom: Add automatic arrayification to object_property_add()

[Andreas Färber]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

If "[*]" is given as the last part of a QOM property name, treat that
as an array property. The added property is given the first available
name, replacing the * with a decimal number counting from 0.

First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
on.

Callers may inspect the ObjectProperty * return value to see what
number the added property was given.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qom/object.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index a298b32..da0919a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -728,6 +728,27 @@ object_property_add(Object *obj, const char *name, const char *type,
                     void *opaque, Error **errp)
 {
     ObjectProperty *prop;
+    size_t name_len = strlen(name);
+
+    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
+        int i;
+        ObjectProperty *ret;
+        char *name_no_array = g_strdup(name);
+
+        name_no_array[name_len - 3] = '\0';
+        for (i = 0; ; ++i) {
+            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
+
+            ret = object_property_add(obj, full_name, type, get, set,
+                                      release, opaque, NULL);
+            g_free(full_name);
+            if (ret) {
+                break;
+            }
+        }
+        g_free(name_no_array);
+        return ret;
+    }
 
     QTAILQ_FOREACH(prop, &obj->properties, node) {
         if (strcmp(prop->name, name) == 0) {
-- 
1.8.4.5

[Qemu-devel] [PULL 4/7] memory: Remove object_property_add_child_array()

[Andreas Färber]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Obsoleted by automatic object_property_add() arrayification.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 memory.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/memory.c b/memory.c
index ef0be1c..1bae951 100644
--- a/memory.c
+++ b/memory.c
@@ -876,30 +876,6 @@ static char *memory_region_escape_name(const char *name)
     return escaped;
 }
 
-static void object_property_add_child_array(Object *owner,
-                                            const char *name,
-                                            Object *child)
-{
-    int i;
-    char *base_name = memory_region_escape_name(name);
-
-    for (i = 0; ; i++) {
-        char *full_name = g_strdup_printf("%s[%d]", base_name, i);
-        Error *local_err = NULL;
-
-        object_property_add_child(owner, full_name, child, &local_err);
-        g_free(full_name);
-        if (!local_err) {
-            break;
-        }
-
-        error_free(local_err);
-    }
-
-    g_free(base_name);
-}
-        
-
 void memory_region_init(MemoryRegion *mr,
                         Object *owner,
                         const char *name,
@@ -917,8 +893,12 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 
     if (name) {
-        object_property_add_child_array(owner, name, OBJECT(mr));
+        char *escaped_name = memory_region_escape_name(name);
+        char *name_array = g_strdup_printf("%s[*]", escaped_name);
+        object_property_add_child(owner, name_array, OBJECT(mr), &error_abort);
         object_unref(OBJECT(mr));
+        g_free(name_array);
+        g_free(escaped_name);
     }
 }
 
-- 
1.8.4.5

[Qemu-devel] [PULL 5/7] qdev: Use error_abort instead of using local_err

[Andreas Färber]

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

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

While at it, remove superfluous check about local_err.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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.8.4.5

[Qemu-devel] [PULL 6/7] qdev: Use NULL instead of local_err for qbus_child unrealize

[Andreas Färber]

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

Forcefully unrealize all children regardless of errors in earlier
iterations (if any). We should keep going with cleanup operation
rather than report an error immediately. Therefore store the first
child unrealization failure and propagate it at the end. We also
forcefully unregister vmsd and unrealize actual object, too.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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.8.4.5

[Qemu-devel] [PULL 7/7] qdev: Add cleanup logic in device_set_realized() to avoid resource leak

[Andreas Färber]

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

At present, this function doesn't have partial cleanup implemented,
which will cause resource leaks in some scenarios.

Example:

1. Assume that "dc->realize(dev, &local_err)" executes successful
   and local_err == NULL;
2. device hotplug in hotplug_handler_plug() executes but fails
   (it is prone to occur). Then local_err != NULL;
3. error_propagate(errp, local_err) and return. But the resources
   which have been allocated in dc->realize() will be leaked.
Simple backtrace:
  dc->realize()
   |->device_realize
            |->pci_qdev_init()
                |->do_pci_register_device()
                |->etc.

Add fuller cleanup logic which assures that function can
goto appropriate error label as local_err population is
detected at each relevant point.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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.8.4.5

Re: [Qemu-devel] [PULL for-2.1 0/7] QOM devices patch queue 2014-09-04

[Peter Maydell]

On 4 September 2014 18:21, Andreas Färber <afaerber@suse.de> wrote:
> Hello Peter,
>
> This is my QOM (devices) patch queue. Please pull.
>
> Regards,
> Andreas
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
>
> The following changes since commit 01eb313907dda97313b8fea62e5632fca64f069c:
>
>   Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-2014-09-03' into staging (2014-09-04 13:33:53 +0100)
>
> are available in the git repository at:
>
>
>   git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter
>
> for you to fetch changes up to 1d45a705fc007a13f20d18473290082eae6d1725:
>
>   qdev: Add cleanup logic in device_set_realized() to avoid resource leak (2014-09-04 19:15:54 +0200)
>
> ----------------------------------------------------------------
> QOM infrastructure fixes and device conversions
>
> * Cleanups for recursive device unrealization
>
> ----------------------------------------------------------------
> Alexey Kardashevskiy (1):
>       qom: Make object_child_foreach() safe for objects removal
>
> Andreas Färber (1):
>       machine: Clean up -machine handling
>
> Gonglei (3):
>       qdev: Use error_abort instead of using local_err
>       qdev: Use NULL instead of local_err for qbus_child unrealize
>       qdev: Add cleanup logic in device_set_realized() to avoid resource leak
>
> Peter Crosthwaite (2):
>       qom: Add automatic arrayification to object_property_add()
>       memory: Remove object_property_add_child_array()

Applied, thanks.

-- PMM