qom: more efficient object property handling

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;
ObjectProperty *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 <[email protected]>

---
 include/qom/object.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 31 ++++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)

include/qom/object.h +9 -0

index be7280c..761ffec 100644
@@ -960,6 +960,56 @@ 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.
+ *

include/qom/object.h +4 -0

+ * have been traversed.
+ */
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
+
 void object_unparent(Object *obj);
 
 /**

qom/object.c +21 -0

index 4805328..7dace59 100644
@@ -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,33 @@ 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);
+}
+
+

qom/object.c +0 -0

 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);

tests/check-qom-proplist.c +45 -0

index 7400b1f..1be8b9e 100644
@@ -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));
 }
 
 

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;
ObjectProperty *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 <[email protected]>

---
 include/qom/object.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 31 ++++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)

include/qom/object.h +50 -0

index be7280c..761ffec 100644
@@ -960,6 +960,56 @@ 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;
+ *   ObjectProperty *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
+ *
+ * Release 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);
 
 /**

qom/object.c +8 -0

index 4805328..7dace59 100644
@@ -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,33 @@ 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);

qom/object.c +13 -0

+}
+
+
+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);

I apologize for the lateness of my review.

qom/object.c ObjectProperty *object_property_find(Object *obj, const char *name, · line 937

Quoted 6 lines

qom/object.c +6 -0

+void object_property_iter_free(ObjectPropertyIterator *iter)
+{
+    if (!iter) {
+        return;
+    }
+    g_free(iter);

g_free(NULL) is perfectly safe; please drop the conditional.

Quoted 10 lines · Markus Armbruster

I apologize for the lateness of my review.

qom/object.c ObjectProperty *object_property_find(Object *obj, const char *name, · line 937

Quoted 6 lines

qom/object.c +6 -0

+void object_property_iter_free(ObjectPropertyIterator *iter)
+{
+    if (!iter) {
+        return;
+    }
+    g_free(iter);

g_free(NULL) is perfectly safe; please drop the conditional.

The next patch in the series has to free some fields in 'iter' so the check for NULL upfront is not redundant.

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 <[email protected]>

---
 qmp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 9 lines

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 <[email protected]>

---
 qmp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by Andreas Färber <[email protected]>

Markus/Luiz, can you ack this change? Markus requested these iterators, I believe.

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 <[email protected]>

---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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 <[email protected]>

---
 hw/ppc/spapr_drc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

hw/ppc/spapr_drc.c +3 -1

index 5d6ea7c..f34bc04 100644
@@ -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;

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 <[email protected]>

---
 net/filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/filter.c +3 -1

index 326f2b5..1365bad 100644
@@ -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;
         }

From: Pavel Fedin <[email protected]>

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 modify their objects during traversal, since GHashTableIter does not have modify-safe version. However, the code seems not to modify objects via these functions.

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

---
 include/qom/object.h | 10 ++++--
 qom/object.c         | 98 ++++++++++++++++++++++++++++++----------------------
 2 files changed, 64 insertions(+), 44 deletions(-)

qom/object.c +1 -1

index 7dace59..dd01652 100644
@@ -68,7 +68,7 @@ struct TypeImpl
 };
 
 struct ObjectPropertyIterator {
-    ObjectProperty *next;
+    GHashTableIter iter;
 };
 
 static Type type_interface;

qom/object.c +9 -0

+{
+    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;

qom/object.c +0 -4

@@ -363,29 +374,35 @@ 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);

qom/object.c +4 -5

-
-        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;

qom/object.c +1 -0

+            g_hash_table_iter_remove(&iter);
             break;
         }
     }

qom/object.c +4 -4

@@ -940,31 +956,27 @@ 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;
 }

qom/object.c +1 -7

-
-    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,

@@ -1484,11 +1496,13 @@ void object_property_add_const_link(Object *obj, const char *name,
 gchar *object_get_canonical_path_component(Object *obj)

qom/object.c +0 -0

         if (!object_property_is_child(prop)) {
             continue;
         }

@@ -1572,11 +1586,13 @@ static Object *object_resolve_partial_path(Object *parent,
                                               bool *ambiguous)

Quoted 10 lines

static void object_property_del_all(Object *obj)
{

qom/object.c +3 -4

-    while (!QTAILQ_EMPTY(&obj->properties)) {
-        ObjectProperty *prop = QTAILQ_FIRST(&obj->properties);
-
-        QTAILQ_REMOVE(&obj->properties, prop, node);
+    ObjectProperty *prop;
+    GHashTableIter iter;
+    gpointer key, value;

qom/object.c static inline bool object_property_is_child(ObjectProperty *prop) · line 386

Quoted 6 lines

qom/object.c +3 -0

+    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);
         }

Why is this not in property_free(), too? Is there a timing difference?

qom/object.c static inline bool object_property_is_child(ObjectProperty *prop) · line 390

Quoted 10 lines

qom/object.c +2 -5

-
-        g_free(prop->name);
-        g_free(prop->type);
-        g_free(prop->description);
-        g_free(prop);
     }
+
+    g_hash_table_unref(obj->properties);
 }

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 5 lines

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

"modify objects" seems a little misleading here; from what I see only adding or removing properties (including child<>s) is forbidden, right? Modifying one ObjectProperty or its value should still be okay.

Quoted 1 line

 2 files changed, 64 insertions(+), 44 deletions(-)

[...]

Quoted 17 lines

qom/object.c +1 -1

index 7dace59..dd01652 100644
@@ -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)
     }
 }
 

qom/object.c static void object_post_init_with_type(Object *obj, TypeImpl *ti) · line 333

Quoted 1 line

qom/object.c +1 -0

+static void property_free(gpointer data)

Bikeshed: We might call this object_property_free() unless there's a precedence for property_...?

Quoted 13 lines

qom/object.c +9 -0

+{
+    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;

[...]

Quoted 12 lines

qom/object.c +3 -4

@@ -363,29 +374,35 @@ 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);
+    ObjectProperty *prop;
+    GHashTableIter iter;
+    gpointer key, value;
 

qom/object.c static inline bool object_property_is_child(ObjectProperty *prop) · line 386

Quoted 6 lines

qom/object.c +3 -0

+    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);
         }

Why is this not in property_free(), too? Is there a timing difference?

qom/object.c ObjectProperty *object_property_find(Object *obj, const char *name, · line 946

Quoted 9 lines

qom/object.c +1 -1

@@ -924,7 +940,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;
 }
 

Is it intentional that our iterator pattern differs?

Quoted 17 lines

qom/object.c +4 -4

@@ -940,31 +956,27 @@ 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;
 }
 
 

qom/object.c void object_property_iter_free(ObjectPropertyIterator *iter) · line 972

Quoted 8 lines

qom/object.c +4 -2

 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);

Is this a behavioral change?

Quoted 6 lines

qom/object.c +1 -0

@@ -1484,11 +1496,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;
 

qom/object.c void object_property_add_const_link(Object *obj, const char *name, · line 1505

Quoted 6 lines

qom/object.c +2 -1

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

Is this cast needed?

Quoted 5 lines

qom/object.c void object_property_add_const_link(Object *obj, const char *name, · line 1505

Quoted 3 lines

qom/object.c +2 -1

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

Is this cast needed?

Probably not, as any pointer should coerce to void * without an explicit cast, unless it had a 'const' involved.

From: Pavel Fedin <[email protected]>

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 modify their objects during traversal, since GHashTableIter does not have modify-safe version. However, the code seems not to modify objects via these functions.

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

(note these seemed misordered)

I have queued things up to 6/7 on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

This patch didn't apply and I had to hand-apply one hunk (which I double-checked, but you never know).

Unfortunately I run into this test failure:

TEST: tests/device-introspect-test... (pid=4094)

  /s390x/device/introspect/list:                                       OK
  /s390x/device/introspect/none:                                       OK
  /s390x/device/introspect/abstract:                                   OK

/s390x/device/introspect/concrete:
(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed
** ERROR:/home/andreas/QEMU/qemu/qom/object.c:867:object_unref: assertion
failed: (obj->ref > 0)
Broken pipe
FAIL
GTester: last random seed: R02S4fa2068506971129a7ebe2323dbe03b7 (pid=4104)
FAIL: tests/device-introspect-test
TEST: tests/qom-test... (pid=4105)

  /s390x/qom/s390-ccw-virtio-2.5:                                      OK
  /s390x/qom/s390-ccw-virtio-2.4:                                      OK
  /s390x/qom/none:                                                     OK

/s390x/qom/s390-virtio:
WARNING
The s390-virtio machine (non-ccw) is deprecated. It will be removed in 2.6. Please use s390-ccw-virtio OK
PASS: tests/qom-test

Are you sure you tested all targets?
Any hunch where this might stem from?

The below patch reveals that the ref count is 0. Might be just a symptom of the actual problem though.

Quoted 63 lines

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

(note these seemed misordered)

I have queued things up to 6/7 on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

This patch didn't apply and I had to hand-apply one hunk (which I double-checked, but you never know).

Unfortunately I run into this test failure:

TEST: tests/device-introspect-test... (pid=4094)

  /s390x/device/introspect/list:                                       OK
  /s390x/device/introspect/none:                                       OK
  /s390x/device/introspect/abstract:                                   OK

/s390x/device/introspect/concrete:
(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed
** ERROR:/home/andreas/QEMU/qemu/qom/object.c:867:object_unref: assertion
failed: (obj->ref > 0)
Broken pipe
FAIL
GTester: last random seed: R02S4fa2068506971129a7ebe2323dbe03b7 (pid=4104)
FAIL: tests/device-introspect-test
TEST: tests/qom-test... (pid=4105)

  /s390x/qom/s390-ccw-virtio-2.5:                                      OK
  /s390x/qom/s390-ccw-virtio-2.4:                                      OK
  /s390x/qom/none:                                                     OK

/s390x/qom/s390-virtio:
WARNING
The s390-virtio machine (non-ccw) is deprecated. It will be removed in 2.6. Please use s390-ccw-virtio OK
PASS: tests/qom-test

Are you sure you tested all targets?
Any hunch where this might stem from?

Quoted 2 lines

The below patch reveals that the ref count is 0. Might be just a symptom of the actual problem though.

A simpler reproducer is s390x-softmmu/qemu-system-s390x -device sclp,help which fails with this patch and succeeds without.

Quoted 8 lines

Quoted 8 lines

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init() and uses a TYPE_... constant as property name. But nothing else stands out immediately.

Hello!

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

Wow... Actually this may come from attempts to modify the tree inside iteration.

Quoted 3 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init() and uses a TYPE_... constant as property name. But nothing else stands out immediately.

I think we should refactor this and retry. If not all problems go away, then we are indeed modifying the tree during iteration, and
we have to find some solution.

From: Pavel Fedin <[email protected]>

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 modify their objects during traversal, since GHashTableIter does not have modify-safe version. However, the code seems not to modify objects via these functions.

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

I wonder... Could we have both list and hashtable? hashtable for searching by name and list for iteration. In this case we would
not have to use glib's iterators, and would be free of problems with them. Just keep the list and hashtable in sync.
Or, is there any hashtable implementation out there which would keep iterators valid during modification? OTOH, glib has a function "remove the element at iterator's position", and we could postpone addition. So, perhaps, using both
containers would be an overkill, just refactor the code to adapt to the new behavior.

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

Quoted 8 lines

Wow... Actually this may come from attempts to modify the tree inside iteration.

Quoted 3 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init() and uses a TYPE_... constant as property name. But nothing else stands out immediately.

I think we should refactor this and retry. If not all problems go away, then we are indeed modifying the tree during iteration, and
we have to find some solution.

David, Conny,

the current tree of afaerber

https://github.com/afaerber/qemu-cpu/commits/qom-next

has this patch:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

which causes failures in make check. A simple reproducer is

qemu-system-s390x -device sclp,help

any idea what would be the most simple fix?
Can we refactor this to create the event facility and the bus in the machine or whatever?

Quoted 5 lines

I wonder... Could we have both list and hashtable? hashtable for searching by name and list for iteration. In this case we would
not have to use glib's iterators, and would be free of problems with them. Just keep the list and hashtable in sync.
Or, is there any hashtable implementation out there which would keep iterators valid during modification? OTOH, glib has a function "remove the element at iterator's position", and we could postpone addition. So, perhaps, using both
containers would be an overkill, just refactor the code to adapt to the new behavior.

My idea, which I wanted to investigate after the weekend, is iterating through the hashtable to create a list of prop->release functions and call them only after finishing the iteration. That might not work either, so we may need to loop over the releasing to allow for released properties to disappear after prop->release().

Am 16.11.2015 um 09:16 schrieb Christian Borntraeger:

Quoted 44 lines · Pavel Fedin

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

Quoted 8 lines

Wow... Actually this may come from attempts to modify the tree inside iteration.

Quoted 3 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init() and uses a TYPE_... constant as property name. But nothing else stands out immediately.

I think we should refactor this and retry. If not all problems go away, then we are indeed modifying the tree during iteration, and
we have to find some solution.

David, Conny,

the current tree of afaerber

https://github.com/afaerber/qemu-cpu/commits/qom-next

has this patch:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

Quoted 8 lines

which causes failures in make check. A simple reproducer is

qemu-system-s390x -device sclp,help

any idea what would be the most simple fix?
Can we refactor this to create the event facility and the bus in the machine or whatever?

I believe it is rather a very general problem with the new object_property_del_all() implementation. It iterates through properties, releasing child<> and link<> properties, which results in an unref, which at some point unparents that device, removing it in the parent's properties hashtable while the parent is iterating through it.

In this case it seems to be about the bus child<> on the event facility.

Quoted 5 lines

Quoted 5 lines

I wonder... Could we have both list and hashtable? hashtable for searching by name and list for iteration. In this case we would
not have to use glib's iterators, and would be free of problems with them. Just keep the list and hashtable in sync.
Or, is there any hashtable implementation out there which would keep iterators valid during modification? OTOH, glib has a function "remove the element at iterator's position", and we could postpone addition. So, perhaps, using both
containers would be an overkill, just refactor the code to adapt to the new behavior.

My idea, which I wanted to investigate after the weekend, is iterating through the hashtable to create a list of prop->release functions and call them only after finishing the iteration. That might not work either, so we may need to loop over the releasing to allow for released properties to disappear after prop->release().

Am 16.11.2015 um 10:38 schrieb Andreas Färber:

Quoted 68 lines

Am 16.11.2015 um 09:16 schrieb Christian Borntraeger:

Quoted 44 lines · Pavel Fedin

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

Quoted 8 lines

Wow... Actually this may come from attempts to modify the tree inside iteration.

Quoted 3 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init() and uses a TYPE_... constant as property name. But nothing else stands out immediately.

I think we should refactor this and retry. If not all problems go away, then we are indeed modifying the tree during iteration, and
we have to find some solution.

David, Conny,

the current tree of afaerber

https://github.com/afaerber/qemu-cpu/commits/qom-next

has this patch:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

Quoted 8 lines

which causes failures in make check. A simple reproducer is

qemu-system-s390x -device sclp,help

any idea what would be the most simple fix?
Can we refactor this to create the event facility and the bus in the machine or whatever?

I believe it is rather a very general problem with the new object_property_del_all() implementation. It iterates through properties, releasing child<> and link<> properties, which results in an unref, which at some point unparents that device, removing it in the parent's properties hashtable while the parent is iterating through it.

In this case it seems to be about the bus child<> on the event facility.

Quoted 5 lines

Quoted 5 lines

I wonder... Could we have both list and hashtable? hashtable for searching by name and list for iteration. In this case we would
not have to use glib's iterators, and would be free of problems with them. Just keep the list and hashtable in sync.
Or, is there any hashtable implementation out there which would keep iterators valid during modification? OTOH, glib has a function "remove the element at iterator's position", and we could postpone addition. So, perhaps, using both
containers would be an overkill, just refactor the code to adapt to the new behavior.

Quoted 5 lines

My idea, which I wanted to investigate after the weekend, is iterating through the hashtable to create a list of prop->release functions and call them only after finishing the iteration. That might not work either, so we may need to loop over the releasing to allow for released properties to disappear after prop->release().

I went with the latter and squashed the attached fixup (without last two hunks, preparing a separate patch for that), interrupting each iteration after prop->release() to be safe. That seems to fix it.

Will prepend and test Dan's unit test next.

qom/object.c +23 -7

index 0ac3bc1..284fa38 100644
@@ -377,14 +377,22 @@ static void object_property_del_all(Object *obj)
     ObjectProperty *prop;
     GHashTableIter iter;
     gpointer key, value;
+    bool released;
 
-    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);
+    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_hash_table_unref(obj->properties);
 }

@@ -401,7 +409,15 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
         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) {
             g_hash_table_iter_remove(&iter);
             break;
         }

@@ -856,7 +872,7 @@ void object_ref(Object *obj)
     if (!obj) {
         return;
     }
-     atomic_inc(&obj->ref);
+    atomic_inc(&obj->ref);
 }
 
 void object_unref(Object *obj)

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init()

and uses a TYPE_... constant as property name.

Quoted 2 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init()

There's nothing wrong with that. It's wrong however to use qdev_set_parent_bus in instance_init. That should be moved to sclp_realize, before the realized property is set to true.

Otherwise, sclp->event_facility outlives its parent. I'm not sure why that works only with the list and not with the hash table though. It may well be a bug in this patch.

Quoted 1 line

and uses a TYPE_... constant as property name.

That's just a little weird, it doesn't break anything per se.

Quoted 6 lines

Quoted 2 lines

Thanks! sclp_init() seems to violate several QOM design principles in that it uses object_new() during TypeInfo::instance_init()

There's nothing wrong with that. It's wrong however to use qdev_set_parent_bus in instance_init. That should be moved to sclp_realize, before the realized property is set to true.

Negative, realize is too late. Since there are two call sites for initialization, I don't see a better place for it.

Quoted 63 lines · Andreas Färber

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 8 lines

From: Pavel Fedin <[email protected]>

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).

Quoted 8 lines

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

Signed-off-by Daniel P. Berrange <[email protected]>

Signed-off-by Pavel Fedin <[email protected]>

(note these seemed misordered)

I have queued things up to 6/7 on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

This patch didn't apply and I had to hand-apply one hunk (which I double-checked, but you never know).

Unfortunately I run into this test failure:

TEST: tests/device-introspect-test... (pid=4094)

  /s390x/device/introspect/list:                                       OK
  /s390x/device/introspect/none:                                       OK
  /s390x/device/introspect/abstract:                                   OK

/s390x/device/introspect/concrete:
(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed

(process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed
** ERROR:/home/andreas/QEMU/qemu/qom/object.c:867:object_unref: assertion
failed: (obj->ref > 0)
Broken pipe
FAIL
GTester: last random seed: R02S4fa2068506971129a7ebe2323dbe03b7 (pid=4104)
FAIL: tests/device-introspect-test
TEST: tests/qom-test... (pid=4105)

  /s390x/qom/s390-ccw-virtio-2.5:                                      OK
  /s390x/qom/s390-ccw-virtio-2.4:                                      OK
  /s390x/qom/none:                                                     OK

/s390x/qom/s390-virtio:
WARNING
The s390-virtio machine (non-ccw) is deprecated. It will be removed in 2.6. Please use s390-ccw-virtio OK
PASS: tests/qom-test

Are you sure you tested all targets?
Any hunch where this might stem from?

Quoted 2 lines

The below patch reveals that the ref count is 0. Might be just a symptom of the actual problem though.

A simpler reproducer is s390x-softmmu/qemu-system-s390x -device sclp,help which fails with this patch and succeeds without.

-----Original Message-----
From: [email protected] [mailto:qemu-devel- [email protected]] On Behalf Of Daniel P. Berrange
Sent: Tuesday, October 13, 2015 3:38 PM
To: [email protected]
Cc: Pavel Fedin; Markus Armbruster; Paolo Bonzini; Andreas Färber
Subject: [Qemu-devel] [PATCH v4 7/7] qom: allow properties to be registered against classes

When there are many instances of a given class, registering properties against the instance is wasteful of resources. The majority of objects have a statically defined list of possible properties, so most of the properties are easily registerable against the class. Only those properties which are conditionally registered at runtime need be recorded against the klass.

Registering properties against classes also makes it possible to provide static introspection of QOM - currently introspection is only possible after creating an instance of a class, which severely limits its usefulness.

This impl only supports simple scalar properties. It does not attempt to allow child object / link object properties against the class. There are ways to support those too, but it would make this patch more complicated, so it is left as an exercise for the future.

There is no equivalent to object_property_del provided, since classes must be immutable once they are defined.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 include/qom/object.h       |  50 ++++++++++
 qom/object.c               | 237 ++++++++++++++++++++++++++++++++++++++++++---
 tests/check-qom-proplist.c |  31 ++++--
 3 files changed, 293 insertions(+), 25 deletions(-)

include/qom/object.h +11 -0

index 2a54515..38f41d3 100644
@@ -381,6 +381,8 @@ struct ObjectClass
     const char *class_cast_cache[OBJECT_CLASS_CAST_CACHE];
 
     ObjectUnparent *unparent;
+
+    GHashTable *properties;
 };
 
 /**

@@ -947,6 +949,13 @@ ObjectProperty *object_property_add(Object *obj, const char *name,
 
 void object_property_del(Object *obj, const char *name, Error **errp);
 
+ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
+                                          const char *type,
+                                          ObjectPropertyAccessor *get,
+                                          ObjectPropertyAccessor *set,
+                                          ObjectPropertyRelease *release,
+                                          void *opaque, Error **errp);
+
 /**
  * object_property_find:
  * @obj: the object

@@ -957,6 +966,8 @@ void object_property_del(Object *obj, const char *name, Error **errp);
  */
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp);
+ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
+                                           Error **errp);
 
 typedef struct ObjectPropertyIterator ObjectPropertyIterator;
 

@@ -964,8 +975,14 @@ typedef struct ObjectPropertyIterator ObjectPropertyIterator;
  * object_property_iter_init:

include/qom/object.h +33 -0

  *
  * It is forbidden to modify the property list while iterating,
  * whether removing or adding properties.

@@ -1375,6 +1392,12 @@ void object_property_add_str(Object *obj, const char *name,
                              void (*set)(Object *, const char *, Error **),
                              Error **errp);
 
+void object_class_property_add_str(ObjectClass *klass, const char *name,
+                                   char *(*get)(Object *, Error **),
+                                   void (*set)(Object *, const char *,
+                                               Error **),
+                                   Error **errp);
+
 /**
  * object_property_add_bool:
  * @obj: the object to add a property to

@@ -1391,6 +1414,11 @@ void object_property_add_bool(Object *obj, const char *name,
                               void (*set)(Object *, bool, Error **),
                               Error **errp);
 
+void object_class_property_add_bool(ObjectClass *klass, const char *name,
+                                    bool (*get)(Object *, Error **),
+                                    void (*set)(Object *, bool, Error **),
+                                    Error **errp);
+
 /**
  * object_property_add_enum:
  * @obj: the object to add a property to

@@ -1410,6 +1438,13 @@ void object_property_add_enum(Object *obj, const char *name,
                               void (*set)(Object *, int, Error **),
                               Error **errp);
 
+void object_class_property_add_enum(ObjectClass *klass, const char *name,
+                                    const char *typename,
+                                    const char * const *strings,
+                                    int (*get)(Object *, Error **),
+                                    void (*set)(Object *, int, Error **),
+                                    Error **errp);
+
 /**
  * object_property_add_tm:
  * @obj: the object to add a property to

@@ -1424,6 +1459,10 @@ void object_property_add_tm(Object *obj, const char *name,
                             void (*get)(Object *, struct tm *, Error **),
                             Error **errp);
 
+void object_class_property_add_tm(ObjectClass *klass, const char *name,
+                                  void (*get)(Object *, struct tm *, Error **),
+                                  Error **errp);
+
 /**
  * object_property_add_uint8_ptr:
  * @obj: the object to add a property to

@@ -1436,6 +1475,8 @@ void object_property_add_tm(Object *obj, const char *name,
  */
 void object_property_add_uint8_ptr(Object *obj, const char *name,
                                    const uint8_t *v, Error **errp);
+void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
+                                         const uint8_t *v, Error **errp);
 
 /**
  * object_property_add_uint16_ptr:

@@ -1449,6 +1490,8 @@ void object_property_add_uint8_ptr(Object *obj, const char *name,
  */
 void object_property_add_uint16_ptr(Object *obj, const char *name,
                                     const uint16_t *v, Error **errp);
+void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
+                                          const uint16_t *v, Error **errp);
 
 /**
  * object_property_add_uint32_ptr:

@@ -1462,6 +1505,8 @@ void object_property_add_uint16_ptr(Object *obj, const char *name,
  */
 void object_property_add_uint32_ptr(Object *obj, const char *name,
                                     const uint32_t *v, Error **errp);
+void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
+                                          const uint32_t *v, Error **errp);
 
 /**
  * object_property_add_uint64_ptr:

@@ -1475,6 +1520,8 @@ void object_property_add_uint32_ptr(Object *obj, const char *name,
  */
 void object_property_add_uint64_ptr(Object *obj, const char *name,
                                     const uint64_t *v, Error **Errp);
+void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
+                                          const uint64_t *v, Error **Errp);
 
 /**
  * object_property_add_alias:

@@ -1526,6 +1573,9 @@ void object_property_add_const_link(Object *obj, const char *name,
  */
 void object_property_set_description(Object *obj, const char *name,
                                      const char *description, Error **errp);
+void object_class_property_set_description(ObjectClass *klass, const char *name,
+                                           const char *description,
+                                           Error **errp);
 
 /**
  * object_child_foreach:

qom/object.c +11 -0

index dd01652..f41edf4 100644
@@ -68,6 +68,7 @@ struct TypeImpl
 };
 
 struct ObjectPropertyIterator {
+    ObjectClass *nextclass;
     GHashTableIter iter;
 };
 

@@ -246,6 +247,16 @@ static void type_initialize_interface(TypeImpl *ti, TypeImpl
 *interface_type,
                                            iface_impl->class);
 }
 
+static void property_free(gpointer data)
+{
+    ObjectProperty *prop = data;
+
+    g_free(prop->name);
+    g_free(prop->type);
+    g_free(prop->description);
+    g_free(prop);
+}
+
 static void type_initialize(TypeImpl *ti)
 {

     TypeImpl *parent;

qom/object.c +211 -15

@@ -268,6 +279,8 @@ static void type_initialize(TypeImpl *ti)
         g_assert(parent->class_size <= ti->class_size);
         memcpy(ti->class, parent->class, parent->class_size);
         ti->class->interfaces = NULL;
+        ti->class->properties = g_hash_table_new_full(
+            g_str_hash, g_str_equal, g_free, property_free);
 
         for (e = parent->class->interfaces; e; e = e->next) {
             InterfaceClass *iface = e->data;

@@ -292,6 +305,9 @@ static void type_initialize(TypeImpl *ti)
 
             type_initialize_interface(ti, t, t);
         }
+    } else {
+        ti->class->properties = g_hash_table_new_full(
+            g_str_hash, g_str_equal, g_free, property_free);
     }
 
     ti->class->type = ti;

@@ -330,16 +346,6 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
-static void 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;

@@ -902,10 +908,11 @@ object_property_add(Object *obj, const char *name, const char *type,
         return ret;
     }
 
-    if (g_hash_table_contains(obj->properties, name)) {
+
+    if (object_property_find(obj, name, NULL) != NULL) {
         error_setg(errp, "attempt to add duplicate property '%s'"
-                       " to object (type '%s')", name,
-                       object_get_typename(obj));
+                   " to object (type '%s')", name,
+                   object_get_typename(obj));
         return NULL;
     }
 

@@ -923,10 +930,50 @@ object_property_add(Object *obj, const char *name, const char *type,
     return prop;
 }
 
+ObjectProperty *
+object_class_property_add(ObjectClass *klass,
+                          const char *name,
+                          const char *type,
+                          ObjectPropertyAccessor *get,
+                          ObjectPropertyAccessor *set,
+                          ObjectPropertyRelease *release,
+                          void *opaque,
+                          Error **errp)
+{
+    ObjectProperty *prop;
+
+    if (object_class_property_find(klass, name, NULL) != NULL) {
+        error_setg(errp, "attempt to add duplicate property '%s'"
+                   " to object (type '%s')", name,
+                   object_class_get_name(klass));
+        return NULL;
+    }
+
+    prop = g_malloc0(sizeof(*prop));
+
+    prop->name = g_strdup(name);
+    prop->type = g_strdup(type);
+
+    prop->get = get;
+    prop->set = set;
+    prop->release = release;
+    prop->opaque = opaque;
+
+    g_hash_table_insert(klass->properties, g_strdup(name), prop);
+
+    return prop;
+}
+
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp)
 {
     ObjectProperty *prop;
+    ObjectClass *klass = object_get_class(obj);
+
+    prop = object_class_property_find(klass, name, NULL);
+    if (prop) {
+        return prop;
+    }
 
     prop = g_hash_table_lookup(obj->properties, name);
     if (prop) {

@@ -941,6 +988,7 @@ ObjectPropertyIterator *object_property_iter_init(Object *obj)
 {
     ObjectPropertyIterator *ret = g_new0(ObjectPropertyIterator, 1);
     g_hash_table_iter_init(&ret->iter, obj->properties);
+    ret->nextclass = object_get_class(obj);
     return ret;
 }
 

@@ -957,12 +1005,37 @@ void object_property_iter_free(ObjectPropertyIterator *iter)
 ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
 {
     gpointer key, val;
-    if (!g_hash_table_iter_next(&iter->iter, &key, &val)) {
-        return NULL;
+    while (!g_hash_table_iter_next(&iter->iter, &key, &val)) {
+        if (!iter->nextclass) {
+            return NULL;
+        }
+        g_hash_table_iter_init(&iter->iter, iter->nextclass->properties);
+        iter->nextclass = object_class_get_parent(iter->nextclass);
     }
     return val;
 }
 
+ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
+                                           Error **errp)
+{
+    ObjectProperty *prop;
+    ObjectClass *parent_klass;
+
+    parent_klass = object_class_get_parent(klass);
+    if (parent_klass) {
+        prop = object_class_property_find(parent_klass, name, NULL);
+        if (prop) {
+            return prop;
+        }
+    }
+
+    prop = g_hash_table_lookup(klass->properties, name);
+    if (!prop) {
+        error_setg(errp, "Property '.%s' not found", name);
+    }
+    return prop;
+}
+
 
 void object_property_del(Object *obj, const char *name, Error **errp)
 {

@@ -1717,6 +1790,29 @@ void object_property_add_str(Object *obj, const char *name,
     }
 }
 
+void object_class_property_add_str(ObjectClass *klass, const char *name,
+                                   char *(*get)(Object *, Error **),
+                                   void (*set)(Object *, const char *,
+                                               Error **),
+                                   Error **errp)
+{
+    Error *local_err = NULL;
+    StringProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->get = get;
+    prop->set = set;
+
+    object_class_property_add(klass, name, "string",
+                              get ? property_get_str : NULL,
+                              set ? property_set_str : NULL,
+                              property_release_str,
+                              prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 typedef struct BoolProperty
 {
     bool (*get)(Object *, Error **);

@@ -1784,6 +1880,28 @@ void object_property_add_bool(Object *obj, const char *name,
     }
 }
 
+void object_class_property_add_bool(ObjectClass *klass, const char *name,
+                                    bool (*get)(Object *, Error **),
+                                    void (*set)(Object *, bool, Error **),
+                                    Error **errp)
+{
+    Error *local_err = NULL;
+    BoolProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->get = get;
+    prop->set = set;
+
+    object_class_property_add(klass, name, "bool",
+                              get ? property_get_bool : NULL,
+                              set ? property_set_bool : NULL,
+                              property_release_bool,
+                              prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 static void property_get_enum(Object *obj, Visitor *v, void *opaque,
                               const char *name, Error **errp)
 {

@@ -1847,6 +1965,31 @@ void object_property_add_enum(Object *obj, const char *name,
     }
 }
 
+void object_class_property_add_enum(ObjectClass *klass, const char *name,
+                                    const char *typename,
+                                    const char * const *strings,
+                                    int (*get)(Object *, Error **),
+                                    void (*set)(Object *, int, Error **),
+                                    Error **errp)
+{
+    Error *local_err = NULL;
+    EnumProperty *prop = g_malloc(sizeof(*prop));
+
+    prop->strings = strings;
+    prop->get = get;
+    prop->set = set;
+
+    object_class_property_add(klass, name, typename,
+                              get ? property_get_enum : NULL,
+                              set ? property_set_enum : NULL,
+                              property_release_enum,
+                              prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 typedef struct TMProperty {
     void (*get)(Object *, struct tm *, Error **);
 } TMProperty;

@@ -1926,6 +2069,25 @@ void object_property_add_tm(Object *obj, const char *name,
     }
 }
 
+void object_class_property_add_tm(ObjectClass *klass, const char *name,
+                                  void (*get)(Object *, struct tm *, Error **),
+                                  Error **errp)
+{
+    Error *local_err = NULL;
+    TMProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->get = get;
+
+    object_class_property_add(klass, name, "struct tm",
+                              get ? property_get_tm : NULL, NULL,
+                              property_release_tm,
+                              prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 static char *qdev_get_type(Object *obj, Error **errp)
 {
     return g_strdup(object_get_typename(obj));

@@ -1970,6 +2132,13 @@ void object_property_add_uint8_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
+                                         const uint8_t *v, Error **errp)
+{
+    object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
+                              NULL, NULL, (void *)v, errp);
+}
+
 void object_property_add_uint16_ptr(Object *obj, const char *name,
                                     const uint16_t *v, Error **errp)
 {

@@ -1977,6 +2146,13 @@ void object_property_add_uint16_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
+                                          const uint16_t *v, Error **errp)
+{
+    object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
+                              NULL, NULL, (void *)v, errp);
+}
+
 void object_property_add_uint32_ptr(Object *obj, const char *name,
                                     const uint32_t *v, Error **errp)
 {

@@ -1984,6 +2160,13 @@ void object_property_add_uint32_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
+                                          const uint32_t *v, Error **errp)
+{
+    object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
+                              NULL, NULL, (void *)v, errp);
+}
+
 void object_property_add_uint64_ptr(Object *obj, const char *name,
                                     const uint64_t *v, Error **errp)
 {

@@ -1991,6 +2174,13 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
+                                          const uint64_t *v, Error **errp)
+{
+    object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
+                              NULL, NULL, (void *)v, errp);
+}
+
 typedef struct {
     Object *target_obj;
     char *target_name;

@@ -2088,6 +2278,23 @@ void object_property_set_description(Object *obj, const char *name,
     op->description = g_strdup(description);
 }
 
+void object_class_property_set_description(ObjectClass *klass,
+                                           const char *name,
+                                           const char *description,
+                                           Error **errp)
+{
+    ObjectProperty *op;
+
+    op = g_hash_table_lookup(klass->properties, name);
+    if (!op) {
+        error_setg(errp, "Property '.%s' not found", name);
+        return;
+    }
+
+    g_free(op->description);
+    op->description = g_strdup(description);
+}
+
 static void object_instance_init(Object *obj)
 {
     object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);

tests/check-qom-proplist.c +21 -11

index 1be8b9e..87de80b 100644
@@ -123,18 +123,28 @@ static void dummy_init(Object *obj)
                              dummy_get_bv,
                              dummy_set_bv,
                              NULL);
-    object_property_add_str(obj, "sv",
-                            dummy_get_sv,
-                            dummy_set_sv,
-                            NULL);
-    object_property_add_enum(obj, "av",
-                             "DummyAnimal",
-                             dummy_animal_map,
-                             dummy_get_av,
-                             dummy_set_av,
-                             NULL);
 }
 
+
+static void dummy_class_init(ObjectClass *cls, void *data)
+{
+    object_class_property_add_bool(cls, "bv",
+                                   dummy_get_bv,
+                                   dummy_set_bv,
+                                   NULL);
+    object_class_property_add_str(cls, "sv",
+                                  dummy_get_sv,
+                                  dummy_set_sv,
+                                  NULL);
+    object_class_property_add_enum(cls, "av",
+                                   "DummyAnimal",
+                                   dummy_animal_map,
+                                   dummy_get_av,
+                                   dummy_set_av,
+                                   NULL);
+}
+
+
 static void dummy_finalize(Object *obj)
 {
     DummyObject *dobj = DUMMY_OBJECT(obj);

@@ -150,6 +160,7 @@ static const TypeInfo dummy_info = {
     .instance_init = dummy_init,
     .instance_finalize = dummy_finalize,
     .class_size = sizeof(DummyObjectClass),
+    .class_init = dummy_class_init,
 };
 
 static void test_dummy_createv(void)
--

2.4.3

Quoted 4 lines

include/qom/object.h +0 -0

index 2a54515..38f41d3 100644

Quoted 6 lines

include/qom/object.h +1 -0

@@ -964,8 +975,14 @@ typedef struct ObjectPropertyIterator ObjectPropertyIterator;
  * object_property_iter_init:
  * @obj: the object
  *
+<<<<<<< HEAD
  * Initializes an iterator for traversing all properties

include/qom/object.h typedef struct ObjectPropertyIterator ObjectPropertyIterator; · line 985

Quoted 6 lines

include/qom/object.h +5 -0

  * registered against an object instance.
+=======
+ * Iterates over all properties defined against the object
+ * instance, its class and all parent classes, calling
+ * @iter for each property.
+>>>>>>> 0ca9307... qom: allow properties to be registered against classes

Conflict markers slipped in the comment

Am 13.10.2015 um 15:18 schrieb Pavel Fedin:

Quoted 4 lines

Quoted 4 lines

include/qom/object.h +0 -0

index 2a54515..38f41d3 100644

[...]

Quoted 6 lines

Quoted 6 lines

include/qom/object.h +1 -0

@@ -964,8 +975,14 @@ typedef struct ObjectPropertyIterator ObjectPropertyIterator;
  * object_property_iter_init:
  * @obj: the object
  *
+<<<<<<< HEAD
  * Initializes an iterator for traversing all properties

Quoted 8 lines

include/qom/object.h typedef struct ObjectPropertyIterator ObjectPropertyIterator; · line 985

Quoted 6 lines

include/qom/object.h +5 -0

  * registered against an object instance.
+=======
+ * Iterates over all properties defined against the object
+ * instance, its class and all parent classes, calling
+ * @iter for each property.
+>>>>>>> 0ca9307... qom: allow properties to be registered against classes

Conflict markers slipped in the comment

There is no v5 fixing this yet? If agreement has been reached, I would at least start applying from the front, to avoid another lengthy review cycle; I could try to resolve the conflict myself if it's the only nit.

Quoted 21 lines

Am 13.10.2015 um 15:18 schrieb Pavel Fedin:

Quoted 4 lines

Quoted 4 lines

include/qom/object.h +0 -0

index 2a54515..38f41d3 100644

[...]

Quoted 14 lines

include/qom/object.h typedef struct ObjectPropertyIterator ObjectPropertyIterator; · line 985

Quoted 12 lines

include/qom/object.h +6 -0

@@ -964,8 +975,14 @@ typedef struct ObjectPropertyIterator ObjectPropertyIterator;
  * object_property_iter_init:
  * @obj: the object
  *
+<<<<<<< HEAD
  * Initializes an iterator for traversing all properties
  * registered against an object instance.
+=======
+ * Iterates over all properties defined against the object
+ * instance, its class and all parent classes, calling
+ * @iter for each property.
+>>>>>>> 0ca9307... qom: allow properties to be registered against classes

Conflict markers slipped in the comment

Quoted 3 lines

There is no v5 fixing this yet? If agreement has been reached, I would at least start applying from the front, to avoid another lengthy review cycle; I could try to resolve the conflict myself if it's the only nit.

Opps, I missed this comment. No, I've not sent a v5, but it looks like it is just this comment that's the problem which is why I didn't catch it during compilation.

This patch series is a followup to

v3: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02024.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05953.html

     +
     https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01455.html

This series introduces a concept of object property iterators to QOM so callers are insulated from the specific data structures used for storing properties against objects/classes. It then converts Object to use a GHashTable for storing properties. Finally it introduces ObjectClass properties.

Probably the only controversial thing is the item Pavel points out about object_child_foreach iterators now being forbidden from modifying the object composition tree.

Daniel,

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:

Quoted 13 lines

This patch series is a followup to

v3: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02024.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05953.html

     +
     https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01455.html

This series introduces a concept of object property iterators to QOM so callers are insulated from the specific data structures used for storing properties against objects/classes. It then converts Object to use a GHashTable for storing properties. Finally it introduces ObjectClass properties.

Quoted 3 lines

Probably the only controversial thing is the item Pavel points out about object_child_foreach iterators now being forbidden from modifying the object composition tree.

Before this grows even larger, can't we just apply your v2 and go on from there? I had reviewed that okay, just have it in my to-test queue...

This series introduces a concept of object property iterators to QOM so callers are insulated from the specific data structures used for storing properties against objects/classes. It then converts Object to use a GHashTable for storing properties. Finally it introduces ObjectClass properties.

Probably the only controversial thing is the item Pavel points out about object_child_foreach iterators now being forbidden from modifying the object composition tree.