[Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Andreas Färber]

Hello Peter,

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

Regards,
Andreas

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Pavel Fedin <p.fedin@samsung.com>

The following changes since commit 74fcbd22d20a2fbc1a47a7b00cce5bf98fd7be5f:

  hw/misc: Add support for ADC controller in Xilinx Zynq 7000 (2015-11-12 21:30:42 +0000)

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 c5760450796433621ccdc125866a84b892e2d5b6:

  MAINTAINERS: Add check-qom-{interface,proplist} to QOM (2015-11-18 21:13:50 +0100)

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

* Fix for properties on objects > 4 GiB
* Performance improvements for QOM property handling
* Assertion cleanups
* MAINTAINERS additions

----------------------------------------------------------------
Andreas Färber (2):
      qom: Clean up assertions to display values on failure
      MAINTAINERS: Add check-qom-{interface,proplist} to QOM

Daniel P. Berrange (6):
      qom: Introduce ObjectPropertyIterator struct for iteration
      qmp: Convert QMP code to use object property iterators
      vl: Convert machine help code to use object property iterators
      ppc: Convert spapr code to use object property iterators
      net: Convert net filter code to use object property iterators
      qom: Add a test case for complex property finalization

Ildar Isaev (1):
      qdev: Change Property::offset field to ptrdiff_t type

Pavel Fedin (1):
      qom: Replace object property list with GHashTable

 MAINTAINERS                |   2 +
 hw/ppc/spapr_drc.c         |   5 +-
 include/hw/qdev-core.h     |   2 +-
 include/qom/object.h       |  59 ++++++++++++-
 net/filter.c               |   5 +-
 qmp.c                      |  10 ++-
 qom/object.c               | 148 ++++++++++++++++++++++----------
 tests/check-qom-proplist.c | 205 +++++++++++++++++++++++++++++++++++++++++++++
 vl.c                       |   5 +-
 9 files changed, 388 insertions(+), 53 deletions(-)

[Qemu-devel] [PULL 01/10] qdev: Change Property::offset field to ptrdiff_t type

[Andreas Färber]

From: Ildar Isaev <ild@inbox.ru>

Property::offset field is calculated as a diff between two pointers:

  arrayprop->prop.offset = eltptr - (void *)dev;

If offset is declared as int, this subtraction can cause type overflow,
thus leading to failure of the subsequent assertion:

  assert(qdev_get_prop_ptr(dev, &arrayprop->prop) == eltptr);

So ptrdiff_t should be used instead.

Signed-off-by: Ildar Isaev <ild@inbox.ru>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/hw/qdev-core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e6dbde4..c537969 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -237,7 +237,7 @@ struct BusState {
 struct Property {
     const char   *name;
     PropertyInfo *info;
-    int          offset;
+    ptrdiff_t    offset;
     uint8_t      bitnr;
     qtype_code   qtype;
     int64_t      defval;
-- 
2.6.2

[Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Some users of QOM need to be able to iterate over properties
defined against an object instance. Currently they are just
directly using the QTAIL macros against the object properties
data structure.

This is bad because it exposes them to changes in the data
structure used to store properties, as well as changes in
functionality such as ability to register properties against
the class.

This provides an ObjectPropertyIterator struct which will
insulate the callers from the particular data structure
used to store properties. It can be used thus

  ObjectProperty *prop;
  ObjectPropertyIterator *iter;

  iter = object_property_iter_init(obj);
  while ((prop = object_property_iter_next(iter))) {
      ... do something with prop ...
  }
  object_property_iter_free(iter);

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
[AF: Fixed examples, style cleanups]
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/object.h       | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 28 ++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 0bb89d4..9f70314 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -960,6 +960,55 @@ void object_property_del(Object *obj, const char *name, Error **errp);
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp);
 
+typedef struct ObjectPropertyIterator ObjectPropertyIterator;
+
+/**
+ * object_property_iter_init:
+ * @obj: the object
+ *
+ * Initializes an iterator for traversing all properties
+ * registered against an object instance.
+ *
+ * It is forbidden to modify the property list while iterating,
+ * whether removing or adding properties.
+ *
+ * Typical usage pattern would be
+ *
+ * <example>
+ *   <title>Using object property iterators</title>
+ *   <programlisting>
+ *   ObjectProperty *prop;
+ *   ObjectPropertyIterator *iter;
+ *
+ *   iter = object_property_iter_init(obj);
+ *   while ((prop = object_property_iter_next(iter))) {
+ *     ... do something with prop ...
+ *   }
+ *   object_property_iter_free(iter);
+ *   </programlisting>
+ * </example>
+ *
+ * Returns: the new iterator
+ */
+ObjectPropertyIterator *object_property_iter_init(Object *obj);
+
+/**
+ * object_property_iter_free:
+ * @iter: the iterator instance
+ *
+ * Releases any resources associated with the iterator.
+ */
+void object_property_iter_free(ObjectPropertyIterator *iter);
+
+/**
+ * object_property_iter_next:
+ * @iter: the iterator instance
+ *
+ * Returns: the next property, or %NULL when all properties
+ * have been traversed.
+ */
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
+
 void object_unparent(Object *obj);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index c0decb6..1c926ce 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -67,6 +67,10 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
+struct ObjectPropertyIterator {
+    ObjectProperty *next;
+};
+
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -917,6 +921,30 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
     return NULL;
 }
 
+ObjectPropertyIterator *object_property_iter_init(Object *obj)
+{
+    ObjectPropertyIterator *ret = g_new0(ObjectPropertyIterator, 1);
+    ret->next = QTAILQ_FIRST(&obj->properties);
+    return ret;
+}
+
+void object_property_iter_free(ObjectPropertyIterator *iter)
+{
+    if (!iter) {
+        return;
+    }
+    g_free(iter);
+}
+
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
+{
+    ObjectProperty *ret = iter->next;
+    if (ret) {
+        iter->next = QTAILQ_NEXT(iter->next, node);
+    }
+    return ret;
+}
+
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 7400b1f..1be8b9e 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -283,6 +283,51 @@ static void test_dummy_getenum(void)
                                    &err);
     g_assert(err != NULL);
     error_free(err);
+
+    object_unparent(OBJECT(dobj));
+}
+
+
+static void test_dummy_iterator(void)
+{
+    Object *parent = object_get_objects_root();
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_with_props(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              &error_abort,
+                              "bv", "yes",
+                              "sv", "Hiss hiss hiss",
+                              "av", "platypus",
+                              NULL));
+
+    ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
+    bool seenbv = false, seensv = false, seenav = false, seentype;
+
+    iter = object_property_iter_init(OBJECT(dobj));
+    while ((prop = object_property_iter_next(iter))) {
+        if (g_str_equal(prop->name, "bv")) {
+            seenbv = true;
+        } else if (g_str_equal(prop->name, "sv")) {
+            seensv = true;
+        } else if (g_str_equal(prop->name, "av")) {
+            seenav = true;
+        } else if (g_str_equal(prop->name, "type")) {
+            /* This prop comes from the base Object class */
+            seentype = true;
+        } else {
+            g_printerr("Found prop '%s'\n", prop->name);
+            g_assert_not_reached();
+        }
+    }
+    object_property_iter_free(iter);
+    g_assert(seenbv);
+    g_assert(seenav);
+    g_assert(seensv);
+    g_assert(seentype);
+
+    object_unparent(OBJECT(dobj));
 }
 
 
@@ -297,6 +342,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qom/proplist/createv", test_dummy_createv);
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
+    g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
 
     return g_test_run();
 }
-- 
2.6.2

[Qemu-devel] [PULL 03/10] qmp: Convert QMP code to use object property iterators

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Stop directly accessing the Object::properties field data
structure and instead use the formal object property iterator
APIs. This insulates the code from future data structure
changes in the Object struct.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qmp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/qmp.c b/qmp.c
index ddc63ea..0a1fa19 100644
--- a/qmp.c
+++ b/qmp.c
@@ -210,6 +210,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
     bool ambiguous = false;
     ObjectPropertyInfoList *props = NULL;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
 
     obj = object_resolve_path(path, &ambiguous);
     if (obj == NULL) {
@@ -222,7 +223,8 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         return NULL;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    iter = object_property_iter_init(obj);
+    while ((prop = object_property_iter_next(iter))) {
         ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
 
         entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
@@ -232,6 +234,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         entry->value->name = g_strdup(prop->name);
         entry->value->type = g_strdup(prop->type);
     }
+    object_property_iter_free(iter);
 
     return props;
 }
@@ -503,6 +506,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
     ObjectClass *klass;
     Object *obj;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
     DevicePropertyInfoList *prop_list = NULL;
 
     klass = object_class_by_name(typename);
@@ -531,7 +535,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
 
     obj = object_new(typename);
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    iter = object_property_iter_init(obj);
+    while ((prop = object_property_iter_next(iter))) {
         DevicePropertyInfo *info;
         DevicePropertyInfoList *entry;
 
@@ -562,6 +567,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
         entry->next = prop_list;
         prop_list = entry;
     }
+    object_property_iter_free(iter);
 
     object_unref(obj);
 
-- 
2.6.2

[Qemu-devel] [PULL 04/10] vl: Convert machine help code to use object property iterators

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Stop directly accessing the Object::properties field data
structure and instead use the formal object property iterator
APIs. This insulates the code from future data structure
changes in the Object struct.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 7d993a5..4211ff1 100644
--- a/vl.c
+++ b/vl.c
@@ -1536,12 +1536,14 @@ MachineInfoList *qmp_query_machines(Error **errp)
 static int machine_help_func(QemuOpts *opts, MachineState *machine)
 {
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
 
     if (!qemu_opt_has_help_opt(opts)) {
         return 0;
     }
 
-    QTAILQ_FOREACH(prop, &OBJECT(machine)->properties, node) {
+    iter = object_property_iter_init(OBJECT(machine));
+    while ((prop = object_property_iter_next(iter))) {
         if (!prop->set) {
             continue;
         }
@@ -1554,6 +1556,7 @@ static int machine_help_func(QemuOpts *opts, MachineState *machine)
             error_printf("\n");
         }
     }
+    object_property_iter_free(iter);
 
     return 1;
 }
-- 
2.6.2

[Qemu-devel] [PULL 05/10] ppc: Convert spapr code to use object property iterators

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Stop directly accessing the Object::properties field data
structure and instead use the formal object property iterator
APIs. This insulates the code from future data structure
changes in the Object struct.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ppc/spapr_drc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 5d6ea7c..f34bc04 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -657,6 +657,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
 {
     Object *root_container;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
     uint32_t drc_count = 0;
     GArray *drc_indexes, *drc_power_domains;
     GString *drc_names, *drc_types;
@@ -680,7 +681,8 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
      */
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
 
-    QTAILQ_FOREACH(prop, &root_container->properties, node) {
+    iter = object_property_iter_init(root_container);
+    while ((prop = object_property_iter_next(iter))) {
         Object *obj;
         sPAPRDRConnector *drc;
         sPAPRDRConnectorClass *drck;
@@ -721,6 +723,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
                                     spapr_drc_get_type_str(drc->type));
         drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
     }
+    object_property_iter_free(iter);
 
     /* now write the drc count into the space we reserved at the
      * beginning of the arrays previously
-- 
2.6.2

[Qemu-devel] [PULL 06/10] net: Convert net filter code to use object property iterators

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Stop directly accessing the Object::properties field data
structure and instead use the formal object property iterator
APIs. This insulates the code from future data structure
changes in the Object struct.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 net/filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/filter.c b/net/filter.c
index 326f2b5..1365bad 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -137,6 +137,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     Error *local_err = NULL;
     char *str, *info;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
     StringOutputVisitor *ov;
 
     if (!nf->netdev_id) {
@@ -173,7 +174,8 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
 
     /* generate info str */
-    QTAILQ_FOREACH(prop, &OBJECT(nf)->properties, node) {
+    iter = object_property_iter_init(OBJECT(nf));
+    while ((prop = object_property_iter_next(iter))) {
         if (!strcmp(prop->name, "type")) {
             continue;
         }
@@ -187,6 +189,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
         g_free(str);
         g_free(info);
     }
+    object_property_iter_free(iter);
 }
 
 static void netfilter_finalize(Object *obj)
-- 
2.6.2

[Qemu-devel] [PULL 07/10] qom: Add a test case for complex property finalization

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Devices have some quite complex object child/link relationships
which place some requirements on the object_property_del_all()
function to consider that properties can be modified while
being iterated over.

This extends the QOM property test case to replicate the
device like structure and expose any potential bugs in the
object_property_del_all() function.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 tests/check-qom-proplist.c | 159 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1be8b9e..e674c0f 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -152,6 +152,148 @@ static const TypeInfo dummy_info = {
     .class_size = sizeof(DummyObjectClass),
 };
 
+
+/*
+ * The following 3 object classes are used to
+ * simulate the kind of relationships seen in
+ * qdev, which result in complex object
+ * property destruction ordering.
+ *
+ * DummyDev has a 'bus' child to a DummyBus
+ * DummyBus has a 'backend' child to a DummyBackend
+ * DummyDev has a 'backend' link to DummyBackend
+ *
+ * When DummyDev is finalized, it unparents the
+ * DummyBackend, which unparents the DummyDev
+ * which deletes the 'backend' link from DummyDev
+ * to DummyBackend. This illustrates that the
+ * object_property_del_all() method needs to
+ * cope with the list of properties being changed
+ * while it iterates over them.
+ */
+typedef struct DummyDev DummyDev;
+typedef struct DummyDevClass DummyDevClass;
+typedef struct DummyBus DummyBus;
+typedef struct DummyBusClass DummyBusClass;
+typedef struct DummyBackend DummyBackend;
+typedef struct DummyBackendClass DummyBackendClass;
+
+#define TYPE_DUMMY_DEV "qemu-dummy-dev"
+#define TYPE_DUMMY_BUS "qemu-dummy-bus"
+#define TYPE_DUMMY_BACKEND "qemu-dummy-backend"
+
+#define DUMMY_DEV(obj)                               \
+    OBJECT_CHECK(DummyDev, (obj), TYPE_DUMMY_DEV)
+#define DUMMY_BUS(obj)                               \
+    OBJECT_CHECK(DummyBus, (obj), TYPE_DUMMY_BUS)
+#define DUMMY_BACKEND(obj)                               \
+    OBJECT_CHECK(DummyBackend, (obj), TYPE_DUMMY_BACKEND)
+
+struct DummyDev {
+    Object parent_obj;
+
+    DummyBus *bus;
+};
+
+struct DummyDevClass {
+    ObjectClass parent_class;
+};
+
+struct DummyBus {
+    Object parent_obj;
+
+    DummyBackend *backend;
+};
+
+struct DummyBusClass {
+    ObjectClass parent_class;
+};
+
+struct DummyBackend {
+    Object parent_obj;
+};
+
+struct DummyBackendClass {
+    ObjectClass parent_class;
+};
+
+
+static void dummy_dev_init(Object *obj)
+{
+    DummyDev *dev = DUMMY_DEV(obj);
+    DummyBus *bus = DUMMY_BUS(object_new(TYPE_DUMMY_BUS));
+    DummyBackend *backend = DUMMY_BACKEND(object_new(TYPE_DUMMY_BACKEND));
+
+    object_property_add_child(obj, "bus", OBJECT(bus), NULL);
+    dev->bus = bus;
+    object_property_add_child(OBJECT(bus), "backend", OBJECT(backend), NULL);
+    bus->backend = backend;
+
+    object_property_add_link(obj, "backend", TYPE_DUMMY_BACKEND,
+                             (Object **)&bus->backend, NULL, 0, NULL);
+}
+
+static void dummy_dev_unparent(Object *obj)
+{
+    DummyDev *dev = DUMMY_DEV(obj);
+    object_unparent(OBJECT(dev->bus));
+}
+
+static void dummy_dev_class_init(ObjectClass *klass, void *opaque)
+{
+    klass->unparent = dummy_dev_unparent;
+}
+
+
+static void dummy_bus_init(Object *obj)
+{
+}
+
+static void dummy_bus_unparent(Object *obj)
+{
+    DummyBus *bus = DUMMY_BUS(obj);
+    object_property_del(obj->parent, "backend", NULL);
+    object_unparent(OBJECT(bus->backend));
+}
+
+static void dummy_bus_class_init(ObjectClass *klass, void *opaque)
+{
+    klass->unparent = dummy_bus_unparent;
+}
+
+static void dummy_backend_init(Object *obj)
+{
+}
+
+
+static const TypeInfo dummy_dev_info = {
+    .name          = TYPE_DUMMY_DEV,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyDev),
+    .instance_init = dummy_dev_init,
+    .class_size = sizeof(DummyDevClass),
+    .class_init = dummy_dev_class_init,
+};
+
+static const TypeInfo dummy_bus_info = {
+    .name          = TYPE_DUMMY_BUS,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyBus),
+    .instance_init = dummy_bus_init,
+    .class_size = sizeof(DummyBusClass),
+    .class_init = dummy_bus_class_init,
+};
+
+static const TypeInfo dummy_backend_info = {
+    .name          = TYPE_DUMMY_BACKEND,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyBackend),
+    .instance_init = dummy_backend_init,
+    .class_size = sizeof(DummyBackendClass),
+};
+
+
+
 static void test_dummy_createv(void)
 {
     Error *err = NULL;
@@ -331,18 +473,35 @@ static void test_dummy_iterator(void)
 }
 
 
+static void test_dummy_delchild(void)
+{
+    Object *parent = object_get_objects_root();
+    DummyDev *dev = DUMMY_DEV(
+        object_new_with_props(TYPE_DUMMY_DEV,
+                              parent,
+                              "dev0",
+                              &error_abort,
+                              NULL));
+
+    object_unparent(OBJECT(dev));
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
     module_call_init(MODULE_INIT_QOM);
     type_register_static(&dummy_info);
+    type_register_static(&dummy_dev_info);
+    type_register_static(&dummy_bus_info);
+    type_register_static(&dummy_backend_info);
 
     g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
     g_test_add_func("/qom/proplist/createv", test_dummy_createv);
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
     g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
+    g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
 
     return g_test_run();
 }
-- 
2.6.2

[Qemu-devel] [PULL 08/10] qom: Replace object property list with GHashTable

[Andreas Färber]

From: Pavel Fedin <p.fedin@samsung.com>

ARM GICv3 systems with large number of CPUs create lots of IRQ pins. Since
every pin is represented as a property, number of these properties becomes
very large. Every property add first makes sure there's no duplicates.
Traversing the list becomes very slow, therefore QEMU initialization takes
significant time (several seconds for e. g. 16 CPUs).

This patch replaces list with GHashTable, making lookup very fast. The only
drawback is that object_child_foreach() and object_child_foreach_recursive()
cannot add or remove properties during traversal, since GHashTableIter does
not have modify-safe version. However, the code seems not to modify objects
via these functions.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
[AF: Fixed object_property_del_{all,child}() issues]
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/object.h |  10 +++--
 qom/object.c         | 120 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 83 insertions(+), 47 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 9f70314..f172fea 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -344,8 +344,6 @@ typedef struct ObjectProperty
     ObjectPropertyResolve *resolve;
     ObjectPropertyRelease *release;
     void *opaque;
-
-    QTAILQ_ENTRY(ObjectProperty) node;
 } ObjectProperty;
 
 /**
@@ -405,7 +403,7 @@ struct Object
     /*< private >*/
     ObjectClass *class;
     ObjectFree *free;
-    QTAILQ_HEAD(, ObjectProperty) properties;
+    GHashTable *properties;
     uint32_t ref;
     Object *parent;
 };
@@ -1537,6 +1535,9 @@ void object_property_set_description(Object *obj, const char *name,
  * Call @fn passing each child of @obj and @opaque to it, until @fn returns
  * non-zero.
  *
+ * It is forbidden to add or remove children from @obj from the @fn
+ * callback.
+ *
  * Returns: The last value returned by @fn, or 0 if there is no child.
  */
 int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
@@ -1552,6 +1553,9 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
  * non-zero. Calls recursively, all child nodes of @obj will also be passed
  * all the way down to the leaf nodes of the tree. Depth first ordering.
  *
+ * It is forbidden to add or remove children from @obj (or its
+ * child nodes) from the @fn callback.
+ *
  * Returns: The last value returned by @fn, or 0 if there is no child.
  */
 int object_child_foreach_recursive(Object *obj,
diff --git a/qom/object.c b/qom/object.c
index 1c926ce..19e7561 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -68,7 +68,7 @@ struct TypeImpl
 };
 
 struct ObjectPropertyIterator {
-    ObjectProperty *next;
+    GHashTableIter iter;
 };
 
 static Type type_interface;
@@ -330,6 +330,16 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void object_property_free(gpointer data)
+{
+    ObjectProperty *prop = data;
+
+    g_free(prop->name);
+    g_free(prop->type);
+    g_free(prop->description);
+    g_free(prop);
+}
+
 void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
 {
     Object *obj = data;
@@ -344,7 +354,8 @@ void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
     object_ref(obj);
-    QTAILQ_INIT(&obj->properties);
+    obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
+                                            NULL, object_property_free);
     object_init_with_type(obj, type);
     object_post_init_with_type(obj, type);
 }
@@ -363,29 +374,51 @@ static inline bool object_property_is_child(ObjectProperty *prop)
 
 static void object_property_del_all(Object *obj)
 {
-    while (!QTAILQ_EMPTY(&obj->properties)) {
-        ObjectProperty *prop = QTAILQ_FIRST(&obj->properties);
-
-        QTAILQ_REMOVE(&obj->properties, prop, node);
-
-        if (prop->release) {
-            prop->release(obj, prop->name, prop->opaque);
+    ObjectProperty *prop;
+    GHashTableIter iter;
+    gpointer key, value;
+    bool released;
+
+    do {
+        released = false;
+        g_hash_table_iter_init(&iter, obj->properties);
+        while (g_hash_table_iter_next(&iter, &key, &value)) {
+            prop = value;
+            if (prop->release) {
+                prop->release(obj, prop->name, prop->opaque);
+                prop->release = NULL;
+                released = true;
+                break;
+            }
+            g_hash_table_iter_remove(&iter);
         }
+    } while (released);
 
-        g_free(prop->name);
-        g_free(prop->type);
-        g_free(prop->description);
-        g_free(prop);
-    }
+    g_hash_table_unref(obj->properties);
 }
 
 static void object_property_del_child(Object *obj, Object *child, Error **errp)
 {
     ObjectProperty *prop;
+    GHashTableIter iter;
+    gpointer key, value;
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, &key, &value)) {
+        prop = value;
+        if (object_property_is_child(prop) && prop->opaque == child) {
+            if (prop->release) {
+                prop->release(obj, prop->name, prop->opaque);
+                prop->release = NULL;
+            }
+            break;
+        }
+    }
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, &key, &value)) {
+        prop = value;
         if (object_property_is_child(prop) && prop->opaque == child) {
-            object_property_del(obj, prop->name, errp);
+            g_hash_table_iter_remove(&iter);
             break;
         }
     }
@@ -783,10 +816,12 @@ static int do_object_child_foreach(Object *obj,
                                    int (*fn)(Object *child, void *opaque),
                                    void *opaque, bool recurse)
 {
-    ObjectProperty *prop, *next;
+    GHashTableIter iter;
+    ObjectProperty *prop;
     int ret = 0;
 
-    QTAILQ_FOREACH_SAFE(prop, &obj->properties, node, next) {
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         if (object_property_is_child(prop)) {
             Object *child = prop->opaque;
 
@@ -883,13 +918,11 @@ object_property_add(Object *obj, const char *name, const char *type,
         return ret;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (strcmp(prop->name, name) == 0) {
-            error_setg(errp, "attempt to add duplicate property '%s'"
+    if (g_hash_table_contains(obj->properties, name)) {
+        error_setg(errp, "attempt to add duplicate property '%s'"
                        " to object (type '%s')", name,
                        object_get_typename(obj));
-            return NULL;
-        }
+        return NULL;
     }
 
     prop = g_malloc0(sizeof(*prop));
@@ -902,7 +935,7 @@ object_property_add(Object *obj, const char *name, const char *type,
     prop->release = release;
     prop->opaque = opaque;
 
-    QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
+    g_hash_table_insert(obj->properties, prop->name, prop);
     return prop;
 }
 
@@ -911,10 +944,9 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
 {
     ObjectProperty *prop;
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (strcmp(prop->name, name) == 0) {
-            return prop;
-        }
+    prop = g_hash_table_lookup(obj->properties, name);
+    if (prop) {
+        return prop;
     }
 
     error_setg(errp, "Property '.%s' not found", name);
@@ -924,7 +956,7 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
 ObjectPropertyIterator *object_property_iter_init(Object *obj)
 {
     ObjectPropertyIterator *ret = g_new0(ObjectPropertyIterator, 1);
-    ret->next = QTAILQ_FIRST(&obj->properties);
+    g_hash_table_iter_init(&ret->iter, obj->properties);
     return ret;
 }
 
@@ -938,30 +970,26 @@ void object_property_iter_free(ObjectPropertyIterator *iter)
 
 ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
 {
-    ObjectProperty *ret = iter->next;
-    if (ret) {
-        iter->next = QTAILQ_NEXT(iter->next, node);
+    gpointer key, val;
+    if (!g_hash_table_iter_next(&iter->iter, &key, &val)) {
+        return NULL;
     }
-    return ret;
+    return val;
 }
 
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
-    ObjectProperty *prop = object_property_find(obj, name, errp);
-    if (prop == NULL) {
+    ObjectProperty *prop = g_hash_table_lookup(obj->properties, name);
+
+    if (!prop) {
+        error_setg(errp, "Property '.%s' not found", name);
         return;
     }
 
     if (prop->release) {
         prop->release(obj, name, prop->opaque);
     }
-
-    QTAILQ_REMOVE(&obj->properties, prop, node);
-
-    g_free(prop->name);
-    g_free(prop->type);
-    g_free(prop->description);
-    g_free(prop);
+    g_hash_table_remove(obj->properties, name);
 }
 
 void object_property_get(Object *obj, Visitor *v, const char *name,
@@ -1481,11 +1509,13 @@ void object_property_add_const_link(Object *obj, const char *name,
 gchar *object_get_canonical_path_component(Object *obj)
 {
     ObjectProperty *prop = NULL;
+    GHashTableIter iter;
 
     g_assert(obj);
     g_assert(obj->parent != NULL);
 
-    QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
+    g_hash_table_iter_init(&iter, obj->parent->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         if (!object_property_is_child(prop)) {
             continue;
         }
@@ -1569,11 +1599,13 @@ static Object *object_resolve_partial_path(Object *parent,
                                               bool *ambiguous)
 {
     Object *obj;
+    GHashTableIter iter;
     ObjectProperty *prop;
 
     obj = object_resolve_abs_path(parent, parts, typename, 0);
 
-    QTAILQ_FOREACH(prop, &parent->properties, node) {
+    g_hash_table_iter_init(&iter, parent->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         Object *found;
 
         if (!object_property_is_child(prop)) {
-- 
2.6.2

[Qemu-devel] [PULL 09/10] qom: Clean up assertions to display values on failure

[Andreas Färber]

Instead of using g_assert() for integer comparisons, use
g_assert_cmpint() so that we can see the respective values.

While at it, fix one stray indentation.

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qom/object.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 19e7561..8c5c34e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -265,7 +265,7 @@ static void type_initialize(TypeImpl *ti)
         GSList *e;
         int i;
 
-        g_assert(parent->class_size <= ti->class_size);
+        g_assert_cmpint(parent->class_size, <=, ti->class_size);
         memcpy(ti->class, parent->class, parent->class_size);
         ti->class->interfaces = NULL;
 
@@ -347,9 +347,9 @@ void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
     g_assert(type != NULL);
     type_initialize(type);
 
-    g_assert(type->instance_size >= sizeof(Object));
+    g_assert_cmpint(type->instance_size, >=, sizeof(Object));
     g_assert(type->abstract == false);
-    g_assert(size >= type->instance_size);
+    g_assert_cmpint(size, >=, type->instance_size);
 
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
@@ -450,7 +450,7 @@ static void object_finalize(void *data)
     object_property_del_all(obj);
     object_deinit(obj, ti);
 
-    g_assert(obj->ref == 0);
+    g_assert_cmpint(obj->ref, ==, 0);
     if (obj->free) {
         obj->free(obj);
     }
@@ -872,7 +872,7 @@ void object_ref(Object *obj)
     if (!obj) {
         return;
     }
-     atomic_inc(&obj->ref);
+    atomic_inc(&obj->ref);
 }
 
 void object_unref(Object *obj)
@@ -880,7 +880,7 @@ void object_unref(Object *obj)
     if (!obj) {
         return;
     }
-    g_assert(obj->ref > 0);
+    g_assert_cmpint(obj->ref, >, 0);
 
     /* parent always holds a reference to its children */
     if (atomic_fetch_dec(&obj->ref) == 1) {
-- 
2.6.2

[Qemu-devel] [PULL 10/10] MAINTAINERS: Add check-qom-{interface, proplist} to QOM

[Andreas Färber]

Add the QOM unit tests to the QOM maintenance area so that maintainers
get CC'ed on changes and to document QOM test coverage.

Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e1fa72..28f0139 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1164,6 +1164,8 @@ F: include/qom/
 X: include/qom/cpu.h
 F: qom/
 X: qom/cpu.c
+F: tests/check-qom-interface.c
+F: tests/check-qom-proplist.c
 F: tests/qom-test.c
 
 QMP
-- 
2.6.2

Re: [Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> Some users of QOM need to be able to iterate over properties
> defined against an object instance. Currently they are just
> directly using the QTAIL macros against the object properties
> data structure.
>
> This is bad because it exposes them to changes in the data
> structure used to store properties, as well as changes in
> functionality such as ability to register properties against
> the class.
>
> This provides an ObjectPropertyIterator struct which will
> insulate the callers from the particular data structure
> used to store properties. It can be used thus
>
>   ObjectProperty *prop;
>   ObjectPropertyIterator *iter;
>
>   iter = object_property_iter_init(obj);
>   while ((prop = object_property_iter_next(iter))) {
>       ... do something with prop ...
>   }
>   object_property_iter_free(iter);

I see my review hasn't been addressed, probably because it came late.
Would you accept a follow-up patch to bring the iterator into line with
existing ones?

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Tested-by: Pavel Fedin <p.fedin@samsung.com>
> [AF: Fixed examples, style cleanups]
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Re: [Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Daniel P. Berrange]

On Thu, Nov 19, 2015 at 10:20:22AM +0100, Markus Armbruster wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> >
> > Some users of QOM need to be able to iterate over properties
> > defined against an object instance. Currently they are just
> > directly using the QTAIL macros against the object properties
> > data structure.
> >
> > This is bad because it exposes them to changes in the data
> > structure used to store properties, as well as changes in
> > functionality such as ability to register properties against
> > the class.
> >
> > This provides an ObjectPropertyIterator struct which will
> > insulate the callers from the particular data structure
> > used to store properties. It can be used thus
> >
> >   ObjectProperty *prop;
> >   ObjectPropertyIterator *iter;
> >
> >   iter = object_property_iter_init(obj);
> >   while ((prop = object_property_iter_next(iter))) {
> >       ... do something with prop ...
> >   }
> >   object_property_iter_free(iter);
> 
> I see my review hasn't been addressed, probably because it came late.
> Would you accept a follow-up patch to bring the iterator into line with
> existing ones?

I'll write such a patch if you like, but i guess waiting for it to merge
till 2.6 is no big deal ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Thu, Nov 19, 2015 at 10:20:22AM +0100, Markus Armbruster wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>> > From: "Daniel P. Berrange" <berrange@redhat.com>
>> >
>> > Some users of QOM need to be able to iterate over properties
>> > defined against an object instance. Currently they are just
>> > directly using the QTAIL macros against the object properties
>> > data structure.
>> >
>> > This is bad because it exposes them to changes in the data
>> > structure used to store properties, as well as changes in
>> > functionality such as ability to register properties against
>> > the class.
>> >
>> > This provides an ObjectPropertyIterator struct which will
>> > insulate the callers from the particular data structure
>> > used to store properties. It can be used thus
>> >
>> >   ObjectProperty *prop;
>> >   ObjectPropertyIterator *iter;
>> >
>> >   iter = object_property_iter_init(obj);
>> >   while ((prop = object_property_iter_next(iter))) {
>> >       ... do something with prop ...
>> >   }
>> >   object_property_iter_free(iter);
>> 
>> I see my review hasn't been addressed, probably because it came late.
>> Would you accept a follow-up patch to bring the iterator into line with
>> existing ones?
>
> I'll write such a patch if you like, but i guess waiting for it to merge
> till 2.6 is no big deal ?

Not even a little deal :)

Re: [Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Andreas Färber]

Am 19.11.2015 um 10:20 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> From: "Daniel P. Berrange" <berrange@redhat.com>
>>
>> Some users of QOM need to be able to iterate over properties
>> defined against an object instance. Currently they are just
>> directly using the QTAIL macros against the object properties
>> data structure.
>>
>> This is bad because it exposes them to changes in the data
>> structure used to store properties, as well as changes in
>> functionality such as ability to register properties against
>> the class.
>>
>> This provides an ObjectPropertyIterator struct which will
>> insulate the callers from the particular data structure
>> used to store properties. It can be used thus
>>
>>   ObjectProperty *prop;
>>   ObjectPropertyIterator *iter;
>>
>>   iter = object_property_iter_init(obj);
>>   while ((prop = object_property_iter_next(iter))) {
>>       ... do something with prop ...
>>   }
>>   object_property_iter_free(iter);
> 
> I see my review hasn't been addressed,

Well, it has, I double-checked that the missing "Iterator" above was
already on my branch, therefore my IRC comment pointing you to qom-next.

> probably because it came late.

Other than that you only seemed to discuss design alternatives, for
which neither you nor Daniel provided any actual patch I could've
applied. While I regularly do style fixups myself, and with the series
missing -rc0 also functional fixes, posting a diff for review/record, I
do not see redesigning a 6-patch series as something I can silently do
last-minute without full respin, for which -rc1 did not leave time.

There was a v3 with iterators, and Pavel pinged v4 twice, I did once
too, and the last delay after getting the series to work was only due to
me inserting Daniel's test case (legit hardfreeze material), so ...

> Would you accept a follow-up patch to bring the iterator into line with
> existing ones?

... yes, from my perspective any such cleanups can be done post-2.5.

Please note that both patch 6/7 (included) and 7/7 (not in this pull)
enhance the iterator, so follow-up patches should be based on qom-next
please.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Peter Maydell]

On 18 November 2015 at 20:39, Andreas Färber <afaerber@suse.de> wrote:
> Hello Peter,
>
> This is my late QOM (devices) patch queue. Please pull.
>
> Regards,
> Andreas
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
>
> The following changes since commit 74fcbd22d20a2fbc1a47a7b00cce5bf98fd7be5f:
>
>   hw/misc: Add support for ADC controller in Xilinx Zynq 7000 (2015-11-12 21:30:42 +0000)
>
> 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 c5760450796433621ccdc125866a84b892e2d5b6:
>
>   MAINTAINERS: Add check-qom-{interface,proplist} to QOM (2015-11-18 21:13:50 +0100)
>
> ----------------------------------------------------------------
> QOM infrastructure fixes and device conversions
>
> * Fix for properties on objects > 4 GiB
> * Performance improvements for QOM property handling
> * Assertion cleanups
> * MAINTAINERS additions

Hi. Unfortunately this doesn't build with our minimum supported
glib version:

/Users/pm215/src/qemu-for-merges/qom/object.c:921:9: warning: implicit
declaration of function 'g_hash_table_contains' is invalid in C99
[-Wimplicit-function-declaration]
    if (g_hash_table_contains(obj->properties, name)) {
        ^

g_hash_table_contains was only introduced in glib 2.32.

thanks
-- PMM

Re: [Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Daniel P. Berrange]

On Thu, Nov 19, 2015 at 10:54:15AM +0000, Peter Maydell wrote:
> On 18 November 2015 at 20:39, Andreas Färber <afaerber@suse.de> wrote:
> > Hello Peter,
> >
> > This is my late QOM (devices) patch queue. Please pull.
> >
> > Regards,
> > Andreas
> >
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Daniel P. Berrange <berrange@redhat.com>
> > Cc: Pavel Fedin <p.fedin@samsung.com>
> >
> > The following changes since commit 74fcbd22d20a2fbc1a47a7b00cce5bf98fd7be5f:
> >
> >   hw/misc: Add support for ADC controller in Xilinx Zynq 7000 (2015-11-12 21:30:42 +0000)
> >
> > 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 c5760450796433621ccdc125866a84b892e2d5b6:
> >
> >   MAINTAINERS: Add check-qom-{interface,proplist} to QOM (2015-11-18 21:13:50 +0100)
> >
> > ----------------------------------------------------------------
> > QOM infrastructure fixes and device conversions
> >
> > * Fix for properties on objects > 4 GiB
> > * Performance improvements for QOM property handling
> > * Assertion cleanups
> > * MAINTAINERS additions
> 
> Hi. Unfortunately this doesn't build with our minimum supported
> glib version:
> 
> /Users/pm215/src/qemu-for-merges/qom/object.c:921:9: warning: implicit
> declaration of function 'g_hash_table_contains' is invalid in C99
> [-Wimplicit-function-declaration]
>     if (g_hash_table_contains(obj->properties, name)) {
>         ^
> 
> g_hash_table_contains was only introduced in glib 2.32.

Since we never store NULL as a valid value in the hash tble, we
can simply replace this with g_hash_table_lookup instead

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.11.2015 um 10:20 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> From: "Daniel P. Berrange" <berrange@redhat.com>
>>>
>>> Some users of QOM need to be able to iterate over properties
>>> defined against an object instance. Currently they are just
>>> directly using the QTAIL macros against the object properties
>>> data structure.
>>>
>>> This is bad because it exposes them to changes in the data
>>> structure used to store properties, as well as changes in
>>> functionality such as ability to register properties against
>>> the class.
>>>
>>> This provides an ObjectPropertyIterator struct which will
>>> insulate the callers from the particular data structure
>>> used to store properties. It can be used thus
>>>
>>>   ObjectProperty *prop;
>>>   ObjectPropertyIterator *iter;
>>>
>>>   iter = object_property_iter_init(obj);
>>>   while ((prop = object_property_iter_next(iter))) {
>>>       ... do something with prop ...
>>>   }
>>>   object_property_iter_free(iter);
>> 
>> I see my review hasn't been addressed,
>
> Well, it has, I double-checked that the missing "Iterator" above was
> already on my branch, therefore my IRC comment pointing you to qom-next.
>
>> probably because it came late.
>
> Other than that you only seemed to discuss design alternatives, for
> which neither you nor Daniel provided any actual patch I could've
> applied. While I regularly do style fixups myself, and with the series
> missing -rc0 also functional fixes, posting a diff for review/record, I
> do not see redesigning a 6-patch series as something I can silently do
> last-minute without full respin, for which -rc1 did not leave time.

I certainly didn't expect you to address my review yourself.  You
could've replied with a short note asking Dan to address the remainder
of my review in a follow-up patch.  But no harm done, because I'm not
shy following up about remainders of my reviews myself.

> There was a v3 with iterators, and Pavel pinged v4 twice, I did once
> too, and the last delay after getting the series to work was only due to
> me inserting Daniel's test case (legit hardfreeze material), so ...
>
>> Would you accept a follow-up patch to bring the iterator into line with
>> existing ones?
>
> ... yes, from my perspective any such cleanups can be done post-2.5.

By now should be done, even.

> Please note that both patch 6/7 (included) and 7/7 (not in this pull)
> enhance the iterator, so follow-up patches should be based on qom-next
> please.
>
> Thanks,
> Andreas

Re: [Qemu-devel] [PULL 10/10] MAINTAINERS: Add check-qom-{interface, proplist} to QOM

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 09:39:38PM +0100, Andreas Färber wrote:
> Add the QOM unit tests to the QOM maintenance area so that maintainers
> get CC'ed on changes and to document QOM test coverage.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 10/10] MAINTAINERS: Add check-qom-{interface, proplist} to QOM

[Igor Mammedov]

On Wed, 18 Nov 2015 21:39:38 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Add the QOM unit tests to the QOM maintenance area so that maintainers
> get CC'ed on changes and to document QOM test coverage.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9e1fa72..28f0139 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1164,6 +1164,8 @@ F: include/qom/
>  X: include/qom/cpu.h
>  F: qom/
>  X: qom/cpu.c
> +F: tests/check-qom-interface.c
> +F: tests/check-qom-proplist.c
>  F: tests/qom-test.c
>  
>  QMP

Re: [Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Andreas Färber]

Am 19.11.2015 um 12:53 schrieb Daniel P. Berrange:
> On Thu, Nov 19, 2015 at 10:54:15AM +0000, Peter Maydell wrote:
>> On 18 November 2015 at 20:39, Andreas Färber <afaerber@suse.de> wrote:
>>> Hello Peter,
>>>
>>> This is my late QOM (devices) patch queue. Please pull.
>>>
>>> Regards,
>>> Andreas
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Daniel P. Berrange <berrange@redhat.com>
>>> Cc: Pavel Fedin <p.fedin@samsung.com>
>>>
>>> The following changes since commit 74fcbd22d20a2fbc1a47a7b00cce5bf98fd7be5f:
>>>
>>>   hw/misc: Add support for ADC controller in Xilinx Zynq 7000 (2015-11-12 21:30:42 +0000)
>>>
>>> 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 c5760450796433621ccdc125866a84b892e2d5b6:
>>>
>>>   MAINTAINERS: Add check-qom-{interface,proplist} to QOM (2015-11-18 21:13:50 +0100)
>>>
>>> ----------------------------------------------------------------
>>> QOM infrastructure fixes and device conversions
>>>
>>> * Fix for properties on objects > 4 GiB
>>> * Performance improvements for QOM property handling
>>> * Assertion cleanups
>>> * MAINTAINERS additions
>>
>> Hi. Unfortunately this doesn't build with our minimum supported
>> glib version:
>>
>> /Users/pm215/src/qemu-for-merges/qom/object.c:921:9: warning: implicit
>> declaration of function 'g_hash_table_contains' is invalid in C99
>> [-Wimplicit-function-declaration]
>>     if (g_hash_table_contains(obj->properties, name)) {
>>         ^
>>
>> g_hash_table_contains was only introduced in glib 2.32.
> 
> Since we never store NULL as a valid value in the hash tble, we
> can simply replace this with g_hash_table_lookup instead

Seems to work fine, too:

diff --git a/qom/object.c b/qom/object.c
index 19e7561..ff34c86 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -918,7 +918,7 @@ object_property_add(Object *obj, const char *name,
const char *type,
         return ret;
     }

-    if (g_hash_table_contains(obj->properties, name)) {
+    if (g_hash_table_lookup(obj->properties, name) != NULL) {
         error_setg(errp, "attempt to add duplicate property '%s'"
                        " to object (type '%s')", name,
                        object_get_typename(obj));

Will respin.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL for-2.5 00/10] QOM devices patch queue 2015-11-18

[Daniel P. Berrange]

On Thu, Nov 19, 2015 at 03:09:47PM +0100, Andreas Färber wrote:
> Am 19.11.2015 um 12:53 schrieb Daniel P. Berrange:
> > On Thu, Nov 19, 2015 at 10:54:15AM +0000, Peter Maydell wrote:
> >> On 18 November 2015 at 20:39, Andreas Färber <afaerber@suse.de> wrote:
> >>> Hello Peter,
> >>>
> >>> This is my late QOM (devices) patch queue. Please pull.
> >>>
> >>> Regards,
> >>> Andreas
> >>>
> >>> Cc: Peter Maydell <peter.maydell@linaro.org>
> >>> Cc: Daniel P. Berrange <berrange@redhat.com>
> >>> Cc: Pavel Fedin <p.fedin@samsung.com>
> >>>
> >>> The following changes since commit 74fcbd22d20a2fbc1a47a7b00cce5bf98fd7be5f:
> >>>
> >>>   hw/misc: Add support for ADC controller in Xilinx Zynq 7000 (2015-11-12 21:30:42 +0000)
> >>>
> >>> 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 c5760450796433621ccdc125866a84b892e2d5b6:
> >>>
> >>>   MAINTAINERS: Add check-qom-{interface,proplist} to QOM (2015-11-18 21:13:50 +0100)
> >>>
> >>> ----------------------------------------------------------------
> >>> QOM infrastructure fixes and device conversions
> >>>
> >>> * Fix for properties on objects > 4 GiB
> >>> * Performance improvements for QOM property handling
> >>> * Assertion cleanups
> >>> * MAINTAINERS additions
> >>
> >> Hi. Unfortunately this doesn't build with our minimum supported
> >> glib version:
> >>
> >> /Users/pm215/src/qemu-for-merges/qom/object.c:921:9: warning: implicit
> >> declaration of function 'g_hash_table_contains' is invalid in C99
> >> [-Wimplicit-function-declaration]
> >>     if (g_hash_table_contains(obj->properties, name)) {
> >>         ^
> >>
> >> g_hash_table_contains was only introduced in glib 2.32.
> > 
> > Since we never store NULL as a valid value in the hash tble, we
> > can simply replace this with g_hash_table_lookup instead
> 
> Seems to work fine, too:
> 
> diff --git a/qom/object.c b/qom/object.c
> index 19e7561..ff34c86 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -918,7 +918,7 @@ object_property_add(Object *obj, const char *name,
> const char *type,
>          return ret;
>      }
> 
> -    if (g_hash_table_contains(obj->properties, name)) {
> +    if (g_hash_table_lookup(obj->properties, name) != NULL) {
>          error_setg(errp, "attempt to add duplicate property '%s'"
>                         " to object (type '%s')", name,
>                         object_get_typename(obj));
> 
> Will respin.

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PULL 02/10] qom: Introduce ObjectPropertyIterator struct for iteration

[Andreas Färber]

From: "Daniel P. Berrange" <berrange@redhat.com>

Some users of QOM need to be able to iterate over properties
defined against an object instance. Currently they are just
directly using the QTAIL macros against the object properties
data structure.

This is bad because it exposes them to changes in the data
structure used to store properties, as well as changes in
functionality such as ability to register properties against
the class.

This provides an ObjectPropertyIterator struct which will
insulate the callers from the particular data structure
used to store properties. It can be used thus

  ObjectProperty *prop;
  ObjectPropertyIterator *iter;

  iter = object_property_iter_init(obj);
  while ((prop = object_property_iter_next(iter))) {
      ... do something with prop ...
  }
  object_property_iter_free(iter);

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Pavel Fedin <p.fedin@samsung.com>
[AF: Fixed examples, style cleanups]
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/object.h       | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 28 ++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 0bb89d4..9f70314 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -960,6 +960,55 @@ void object_property_del(Object *obj, const char *name, Error **errp);
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp);
 
+typedef struct ObjectPropertyIterator ObjectPropertyIterator;
+
+/**
+ * object_property_iter_init:
+ * @obj: the object
+ *
+ * Initializes an iterator for traversing all properties
+ * registered against an object instance.
+ *
+ * It is forbidden to modify the property list while iterating,
+ * whether removing or adding properties.
+ *
+ * Typical usage pattern would be
+ *
+ * <example>
+ *   <title>Using object property iterators</title>
+ *   <programlisting>
+ *   ObjectProperty *prop;
+ *   ObjectPropertyIterator *iter;
+ *
+ *   iter = object_property_iter_init(obj);
+ *   while ((prop = object_property_iter_next(iter))) {
+ *     ... do something with prop ...
+ *   }
+ *   object_property_iter_free(iter);
+ *   </programlisting>
+ * </example>
+ *
+ * Returns: the new iterator
+ */
+ObjectPropertyIterator *object_property_iter_init(Object *obj);
+
+/**
+ * object_property_iter_free:
+ * @iter: the iterator instance
+ *
+ * Releases any resources associated with the iterator.
+ */
+void object_property_iter_free(ObjectPropertyIterator *iter);
+
+/**
+ * object_property_iter_next:
+ * @iter: the iterator instance
+ *
+ * Returns: the next property, or %NULL when all properties
+ * have been traversed.
+ */
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
+
 void object_unparent(Object *obj);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index c0decb6..1c926ce 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -67,6 +67,10 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
+struct ObjectPropertyIterator {
+    ObjectProperty *next;
+};
+
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -917,6 +921,30 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
     return NULL;
 }
 
+ObjectPropertyIterator *object_property_iter_init(Object *obj)
+{
+    ObjectPropertyIterator *ret = g_new0(ObjectPropertyIterator, 1);
+    ret->next = QTAILQ_FIRST(&obj->properties);
+    return ret;
+}
+
+void object_property_iter_free(ObjectPropertyIterator *iter)
+{
+    if (!iter) {
+        return;
+    }
+    g_free(iter);
+}
+
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
+{
+    ObjectProperty *ret = iter->next;
+    if (ret) {
+        iter->next = QTAILQ_NEXT(iter->next, node);
+    }
+    return ret;
+}
+
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 7400b1f..1be8b9e 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -283,6 +283,51 @@ static void test_dummy_getenum(void)
                                    &err);
     g_assert(err != NULL);
     error_free(err);
+
+    object_unparent(OBJECT(dobj));
+}
+
+
+static void test_dummy_iterator(void)
+{
+    Object *parent = object_get_objects_root();
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_with_props(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              &error_abort,
+                              "bv", "yes",
+                              "sv", "Hiss hiss hiss",
+                              "av", "platypus",
+                              NULL));
+
+    ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
+    bool seenbv = false, seensv = false, seenav = false, seentype;
+
+    iter = object_property_iter_init(OBJECT(dobj));
+    while ((prop = object_property_iter_next(iter))) {
+        if (g_str_equal(prop->name, "bv")) {
+            seenbv = true;
+        } else if (g_str_equal(prop->name, "sv")) {
+            seensv = true;
+        } else if (g_str_equal(prop->name, "av")) {
+            seenav = true;
+        } else if (g_str_equal(prop->name, "type")) {
+            /* This prop comes from the base Object class */
+            seentype = true;
+        } else {
+            g_printerr("Found prop '%s'\n", prop->name);
+            g_assert_not_reached();
+        }
+    }
+    object_property_iter_free(iter);
+    g_assert(seenbv);
+    g_assert(seenav);
+    g_assert(seensv);
+    g_assert(seentype);
+
+    object_unparent(OBJECT(dobj));
 }
 
 
@@ -297,6 +342,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qom/proplist/createv", test_dummy_createv);
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
+    g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
 
     return g_test_run();
 }
-- 
2.6.2