[Qemu-devel] [PATCH v3 0/5] qom: more efficient object property handling

[Qemu-devel] [PATCH v3 0/5] qom: more efficient object property handling

[Daniel P. Berrange]

This patch series is a combination of my own previous patch to
add support for object properties against classes:

  https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05953.html

And Pavel Fedin's patch to use a hash table instead of list

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

I pulled Pavel's patch in to my series, since both our patches
touch the same code and thus generate nasty merge conflicts.

In resolving these conflicts I decided we needed a new helper
method object_property_foreach to deal with iteration over
properties, hence we now have a short series of patches.

I made a few changes to Pavel's patch but the guts of it are
still his work, so I left him as GIT author, but removed his
Signed-off-by, since that would not apply to my own additions

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 P. Berrange (4):
  qom: introduce object_property_foreach method
  qmp: convert to use object_property_foreach iterators
  vl: convert machine help to use object_property_foreach
  qom: allow properties to be registered against classes

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

 include/qom/object.h |  78 ++++++++++++-
 qmp.c                | 103 +++++++++-------
 qom/object.c         | 325 +++++++++++++++++++++++++++++++++++++++++++++------
 vl.c                 |  37 +++---
 4 files changed, 444 insertions(+), 99 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/5] qom: introduce object_property_foreach method

[Daniel P. Berrange]

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.

Providing an explicit object_property_foreach method provides
a layer of insulation between the QOM user and the QOM internal
implementation.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qom/object.h | 23 +++++++++++++++++++++++
 qom/object.c         | 17 +++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index be7280c..71503af 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp);
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp);
 
+typedef void (*ObjectPropertyIterator)(Object *obj,
+                                       ObjectProperty *prop,
+                                       Error **errp,
+                                       void *opaque);
+
+/**
+ * object_property_foreach:
+ * @obj: the object
+ * @iter: the iterator callback function
+ * @errp: returns an error if iterator function fails
+ * @opaque: opaque data to pass to @iter
+ *
+ * Iterates over all properties defined against the object
+ * instance calling @iter for each property.
+ *
+ * It is forbidden to modify the property list from @iter
+ * whether removing or adding properties.
+ */
+void object_property_foreach(Object *obj,
+                             ObjectPropertyIterator iter,
+                             Error **errp,
+                             void *opaque);
+
 void object_unparent(Object *obj);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 4805328..f8b27af 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
     return NULL;
 }
 
+void object_property_foreach(Object *obj,
+                             ObjectPropertyIterator iter,
+                             Error **errp,
+                             void *opaque)
+{
+    ObjectProperty *prop;
+    Error *local_err = NULL;
+
+    QTAILQ_FOREACH(prop, &obj->properties, node) {
+        iter(obj, prop, &local_err, opaque);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+}
+
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);
-- 
2.4.3

[Qemu-devel] [PATCH v3 2/5] qmp: convert to use object_property_foreach iterators

[Daniel P. Berrange]

Rather than directly traversing the object property list,
use the object_property_foreach iterator. This removes a
dependancy on the implementation approach for properties.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qmp.c | 103 ++++++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 59 insertions(+), 44 deletions(-)

diff --git a/qmp.c b/qmp.c
index 057a7cb..9baad84 100644
--- a/qmp.c
+++ b/qmp.c
@@ -202,12 +202,29 @@ void qmp_system_wakeup(Error **errp)
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 }
 
+
+static void qmp_qom_list_iter(Object *obj,
+                              ObjectProperty *prop,
+                              Error **errp,
+                              void *opaque)
+{
+    ObjectPropertyInfoList **props = opaque;
+
+    ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
+
+    entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
+    entry->next = *props;
+    *props = entry;
+
+    entry->value->name = g_strdup(prop->name);
+    entry->value->type = g_strdup(prop->type);
+}
+
 ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
 {
     Object *obj;
     bool ambiguous = false;
     ObjectPropertyInfoList *props = NULL;
-    ObjectProperty *prop;
 
     obj = object_resolve_path(path, &ambiguous);
     if (obj == NULL) {
@@ -220,17 +237,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         return NULL;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
-
-        entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
-        entry->next = props;
-        props = entry;
-
-        entry->value->name = g_strdup(prop->name);
-        entry->value->type = g_strdup(prop->type);
-    }
-
+    object_property_foreach(obj, qmp_qom_list_iter, NULL, &props);
     return props;
 }
 
@@ -494,12 +501,49 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
     return info;
 }
 
+static void qmp_device_list_properties_iter(Object *obj,
+                                            ObjectProperty *prop,
+                                            Error **errp,
+                                            void *opaque)
+{
+    DevicePropertyInfoList **props = opaque;
+    DevicePropertyInfo *info;
+    DevicePropertyInfoList *entry;
+
+    /* Skip Object and DeviceState properties */
+    if (strcmp(prop->name, "type") == 0 ||
+        strcmp(prop->name, "realized") == 0 ||
+        strcmp(prop->name, "hotpluggable") == 0 ||
+        strcmp(prop->name, "hotplugged") == 0 ||
+        strcmp(prop->name, "parent_bus") == 0) {
+        return;
+    }
+
+    /* Skip legacy properties since they are just string versions of
+     * properties that we already list.
+     */
+    if (strstart(prop->name, "legacy-", NULL)) {
+        return;
+    }
+
+    info = make_device_property_info(object_get_class(obj),
+                                     prop->name, prop->type,
+                                     prop->description);
+    if (!info) {
+        return;
+    }
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+    entry->next = *props;
+    *props = entry;
+}
+
 DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
                                                    Error **errp)
 {
     ObjectClass *klass;
     Object *obj;
-    ObjectProperty *prop;
     DevicePropertyInfoList *prop_list = NULL;
 
     klass = object_class_by_name(typename);
@@ -517,37 +561,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
 
     obj = object_new(typename);
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        DevicePropertyInfo *info;
-        DevicePropertyInfoList *entry;
-
-        /* Skip Object and DeviceState properties */
-        if (strcmp(prop->name, "type") == 0 ||
-            strcmp(prop->name, "realized") == 0 ||
-            strcmp(prop->name, "hotpluggable") == 0 ||
-            strcmp(prop->name, "hotplugged") == 0 ||
-            strcmp(prop->name, "parent_bus") == 0) {
-            continue;
-        }
-
-        /* Skip legacy properties since they are just string versions of
-         * properties that we already list.
-         */
-        if (strstart(prop->name, "legacy-", NULL)) {
-            continue;
-        }
-
-        info = make_device_property_info(klass, prop->name, prop->type,
-                                         prop->description);
-        if (!info) {
-            continue;
-        }
-
-        entry = g_malloc0(sizeof(*entry));
-        entry->value = info;
-        entry->next = prop_list;
-        prop_list = entry;
-    }
+    object_property_foreach(obj, qmp_device_list_properties_iter,
+                            NULL, &prop_list);
 
     object_unref(obj);
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/5] vl: convert machine help to use object_property_foreach

[Daniel P. Berrange]

Rather than directly traversing the object property list,
use the object_property_foreach iterator. This removes a
dependancy on the implementation approach for properties.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 vl.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/vl.c b/vl.c
index f2bd8d2..a29fb82 100644
--- a/vl.c
+++ b/vl.c
@@ -1511,27 +1511,34 @@ MachineInfoList *qmp_query_machines(Error **errp)
     return mach_list;
 }
 
-static int machine_help_func(QemuOpts *opts, MachineState *machine)
+
+static void machine_help_func_iter(Object *obj,
+                                   ObjectProperty *prop,
+                                   Error **errp,
+                                   void *opaque)
 {
-    ObjectProperty *prop;
+    MachineState *machine = MACHINE(obj);
+    if (!prop->set) {
+        return;
+    }
+
+    error_printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
+                 prop->name, prop->type);
+    if (prop->description) {
+        error_printf(" (%s)\n", prop->description);
+    } else {
+        error_printf("\n");
+    }
+}
 
+static int machine_help_func(QemuOpts *opts, MachineState *machine)
+{
     if (!qemu_opt_has_help_opt(opts)) {
         return 0;
     }
 
-    QTAILQ_FOREACH(prop, &OBJECT(machine)->properties, node) {
-        if (!prop->set) {
-            continue;
-        }
-
-        error_printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
-                     prop->name, prop->type);
-        if (prop->description) {
-            error_printf(" (%s)\n", prop->description);
-        } else {
-            error_printf("\n");
-        }
-    }
+    object_property_foreach(OBJECT(machine),
+                            machine_help_func_iter, NULL, NULL);
 
     return 1;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v3 4/5] qom: replace object property list with GHashTable

[Daniel P. Berrange]

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 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 <berrange@redhat.com>
---
 include/qom/object.h | 10 ++++--
 qom/object.c         | 94 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 63 insertions(+), 41 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 71503af..7645f5f 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;
 };
@@ -1511,6 +1509,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),
@@ -1526,6 +1527,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 f8b27af..7cec1c9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -326,6 +326,16 @@ 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;
@@ -340,7 +350,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, property_free);
     object_init_with_type(obj, type);
     object_post_init_with_type(obj, type);
 }
@@ -359,29 +370,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;
 
+    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);
         }
-
-        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) {
-            object_property_del(obj, prop->name, errp);
+            if (prop->release) {
+                prop->release(obj, prop->name, prop->opaque);
+            }
+            g_hash_table_iter_remove(&iter);
             break;
         }
     }
@@ -779,10 +796,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;
 
@@ -879,13 +898,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));
@@ -898,7 +915,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;
 }
 
@@ -907,10 +924,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);
@@ -922,11 +938,13 @@ void object_property_foreach(Object *obj,
                              Error **errp,
                              void *opaque)
 {
-    ObjectProperty *prop;
     Error *local_err = NULL;
+    GHashTableIter props;
+    gpointer key, value;
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        iter(obj, prop, &local_err, opaque);
+    g_hash_table_iter_init(&props, obj->properties);
+    while (g_hash_table_iter_next(&props, &key, &value)) {
+        iter(obj, value, &local_err, opaque);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -936,21 +954,17 @@ void object_property_foreach(Object *obj,
 
 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,
@@ -1470,11 +1484,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;
         }
@@ -1558,11 +1574,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.4.3

[Qemu-devel] [PATCH v3 5/5] qom: allow properties to be registered against classes

[Daniel P. Berrange]

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 <berrange@redhat.com>
---
 include/qom/object.h |  47 +++++++++-
 qom/object.c         | 242 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 275 insertions(+), 14 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 7645f5f..c39008b 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -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 void (*ObjectPropertyIterator)(Object *obj,
                                        ObjectProperty *prop,
@@ -971,7 +982,8 @@ typedef void (*ObjectPropertyIterator)(Object *obj,
  * @opaque: opaque data to pass to @iter
  *
  * Iterates over all properties defined against the object
- * instance calling @iter for each property.
+ * instance, its class and all parent classes, calling
+ * @iter for each property.
  *
  * It is forbidden to modify the property list from @iter
  * whether removing or adding properties.
@@ -1348,6 +1360,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
@@ -1364,6 +1382,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
@@ -1383,6 +1406,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
@@ -1397,6 +1427,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
@@ -1409,6 +1443,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:
@@ -1422,6 +1458,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:
@@ -1435,6 +1473,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:
@@ -1448,6 +1488,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:
@@ -1499,6 +1541,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:
diff --git a/qom/object.c b/qom/object.c
index 7cec1c9..6ca3f4c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -242,6 +242,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;
@@ -264,6 +274,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;
@@ -288,6 +300,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;
@@ -326,16 +341,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;
@@ -898,10 +903,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;
     }
 
@@ -919,10 +925,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) {
@@ -938,6 +984,7 @@ void object_property_foreach(Object *obj,
                              Error **errp,
                              void *opaque)
 {
+    ObjectClass *klass;
     Error *local_err = NULL;
     GHashTableIter props;
     gpointer key, value;
@@ -950,6 +997,41 @@ void object_property_foreach(Object *obj,
             return;
         }
     }
+
+    klass = object_get_class(obj);
+    while (klass) {
+        g_hash_table_iter_init(&props, klass->properties);
+        while (g_hash_table_iter_next(&props, &key, &value)) {
+            iter(obj, value, &local_err, opaque);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+
+        klass = object_class_get_parent(klass);
+    }
+}
+
+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)
@@ -1705,6 +1787,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 **);
@@ -1772,6 +1877,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)
 {
@@ -1835,6 +1962,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;
@@ -1914,6 +2066,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));
@@ -1958,6 +2129,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)
 {
@@ -1965,6 +2143,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)
 {
@@ -1972,6 +2157,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)
 {
@@ -1979,6 +2171,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;
@@ -2076,6 +2275,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);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 4/5] qom: replace object property list with GHashTable

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: Daniel P. Berrange [mailto:berrange@redhat.com]
> Sent: Thursday, October 08, 2015 5:09 PM
> To: qemu-devel@nongnu.org
> Cc: Andreas Färber; Pavel Fedin; Daniel P. Berrange
> Subject: [PATCH v3 4/5] qom: replace object property list with GHashTable
> 
> 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 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 <berrange@redhat.com>
> ---
>  include/qom/object.h | 10 ++++--
>  qom/object.c         | 94 +++++++++++++++++++++++++++++++---------------------
>  2 files changed, 63 insertions(+), 41 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 71503af..7645f5f 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;
>  };
> @@ -1511,6 +1509,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),
> @@ -1526,6 +1527,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 f8b27af..7cec1c9 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -326,6 +326,16 @@ 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;
> @@ -340,7 +350,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, property_free);
>      object_init_with_type(obj, type);
>      object_post_init_with_type(obj, type);
>  }
> @@ -359,29 +370,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;
> 
> +    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);
>          }
> -
> -        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) {
> -            object_property_del(obj, prop->name, errp);
> +            if (prop->release) {
> +                prop->release(obj, prop->name, prop->opaque);
> +            }
> +            g_hash_table_iter_remove(&iter);
>              break;
>          }
>      }
> @@ -779,10 +796,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;
> 
> @@ -879,13 +898,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));
> @@ -898,7 +915,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;
>  }
> 
> @@ -907,10 +924,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);
> @@ -922,11 +938,13 @@ void object_property_foreach(Object *obj,
>                               Error **errp,
>                               void *opaque)
>  {
> -    ObjectProperty *prop;
>      Error *local_err = NULL;
> +    GHashTableIter props;
> +    gpointer key, value;
> 
> -    QTAILQ_FOREACH(prop, &obj->properties, node) {
> -        iter(obj, prop, &local_err, opaque);
> +    g_hash_table_iter_init(&props, obj->properties);
> +    while (g_hash_table_iter_next(&props, &key, &value)) {
> +        iter(obj, value, &local_err, opaque);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
> @@ -936,21 +954,17 @@ void object_property_foreach(Object *obj,
> 
>  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,
> @@ -1470,11 +1484,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;
>          }
> @@ -1558,11 +1574,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.4.3

 Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v3 0/5] qom: more efficient object property handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On 10/08/2015 08:08 AM, Daniel P. Berrange wrote:
> This patch series is a combination of my own previous patch to
> add support for object properties against classes:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05953.html
> 
> And Pavel Fedin's patch to use a hash table instead of list
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01455.html
> 
> I pulled Pavel's patch in to my series, since both our patches
> touch the same code and thus generate nasty merge conflicts.
> 
> In resolving these conflicts I decided we needed a new helper
> method object_property_foreach to deal with iteration over
> properties, hence we now have a short series of patches.
> 
> I made a few changes to Pavel's patch but the guts of it are
> still his work, so I left him as GIT author, but removed his
> Signed-off-by, since that would not apply to my own additions

Actually, I think what is typically done in that case is leaving both
Signed-off-by lines (you both have contributions to the final product);
often with two-stage commit messages:

original comments....
Signed-off-by: person A
additional changes...
Signed-off-by: person B

At any rate, Pavel should have an S-o-B line if he remains git author,
regardless of who else also has a S-o-B line.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] qom: more efficient object property handling

[Pavel Fedin]

 Hi!

 Just got test report from my colleague, he forgot to specify --target-list=aarch64-softmmu to configure and tried to build everything. Looks like there are more property iterations done by hand in PowerPC code:
--- cut ---
In file included from /home/igor/qemu/include/qom/object.h:20:0,
                  from /home/igor/qemu/include/hw/ppc/spapr_drc.h:15,
                  from /home/igor/qemu/hw/ppc/spapr_drc.c:13:
/home/igor/qemu/hw/ppc/spapr_drc.c: In function ‘spapr_drc_populate_dt’:
/home/igor/qemu/include/qemu/queue.h:419:29: error: request for member 
‘tqh_first’ in something not a structure or union
          for ((var) = ((head)->tqh_first);                               \
                              ^
/home/igor/qemu/hw/ppc/spapr_drc.c:683:5: note: in expansion of macro 
‘QTAILQ_FOREACH’
      QTAILQ_FOREACH(prop, &root_container->properties, node) {
      ^
/home/igor/qemu/include/qemu/queue.h:421:31: error: ‘ObjectProperty’ has 
no member named ‘node’
                  (var) = ((var)->field.tqe_next))
                                ^
/home/igor/qemu/hw/ppc/spapr_drc.c:683:5: note: in expansion of macro 
‘QTAILQ_FOREACH’
      QTAILQ_FOREACH(prop, &root_container->properties, node) {
      ^
/home/igor/qemu/rules.mak:57: recipe for target 'hw/ppc/spapr_drc.o' failed
make[1]: *** [hw/ppc/spapr_drc.o] Error 1
Makefile:184: recipe for target 'subdir-ppc64-softmmu' failed
--- cut ---

 Actually, he was building version with my standalone patch, but i have checked your series, it also doesn't touch SPAPR code.

 Tested-by: Igor Skalkin <i.skalkin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v3 0/5] qom: more efficient object property handling

[Daniel P. Berrange]

On Thu, Oct 08, 2015 at 06:27:39PM +0300, Pavel Fedin wrote:
>  Hi!
> 
>  Just got test report from my colleague, he forgot to specify --target-list=aarch64-softmmu to configure and tried to build everything. Looks like there are more property iterations done by hand in PowerPC code:
> --- cut ---
> In file included from /home/igor/qemu/include/qom/object.h:20:0,
>                   from /home/igor/qemu/include/hw/ppc/spapr_drc.h:15,
>                   from /home/igor/qemu/hw/ppc/spapr_drc.c:13:
> /home/igor/qemu/hw/ppc/spapr_drc.c: In function ‘spapr_drc_populate_dt’:
> /home/igor/qemu/include/qemu/queue.h:419:29: error: request for member 
> ‘tqh_first’ in something not a structure or union
>           for ((var) = ((head)->tqh_first);                               \
>                               ^
> /home/igor/qemu/hw/ppc/spapr_drc.c:683:5: note: in expansion of macro 
> ‘QTAILQ_FOREACH’
>       QTAILQ_FOREACH(prop, &root_container->properties, node) {
>       ^
> /home/igor/qemu/include/qemu/queue.h:421:31: error: ‘ObjectProperty’ has 
> no member named ‘node’
>                   (var) = ((var)->field.tqe_next))
>                                 ^
> /home/igor/qemu/hw/ppc/spapr_drc.c:683:5: note: in expansion of macro 
> ‘QTAILQ_FOREACH’
>       QTAILQ_FOREACH(prop, &root_container->properties, node) {
>       ^
> /home/igor/qemu/rules.mak:57: recipe for target 'hw/ppc/spapr_drc.o' failed
> make[1]: *** [hw/ppc/spapr_drc.o] Error 1
> Makefile:184: recipe for target 'subdir-ppc64-softmmu' failed
> --- cut ---
> 
>  Actually, he was building version with my standalone patch, but i have checked your series, it also doesn't touch SPAPR code.

Ah yes, I'll do an all-targets build and check this out.


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] [PATCH v3 1/5] qom: introduce object_property_foreach method

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> 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.
> 
> Providing an explicit object_property_foreach method provides
> a layer of insulation between the QOM user and the QOM internal
> implementation.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qom/object.h | 23 +++++++++++++++++++++++
>  qom/object.c         | 17 +++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index be7280c..71503af 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp);
>  ObjectProperty *object_property_find(Object *obj, const char *name,
>                                       Error **errp);
>  
> +typedef void (*ObjectPropertyIterator)(Object *obj,
> +                                       ObjectProperty *prop,
> +                                       Error **errp,
> +                                       void *opaque);

Do we want the iterator to be able to return a value, and possibly allow
a non-zero value to abort iteration? [1]

> +
> +/**
> + * object_property_foreach:
> + * @obj: the object
> + * @iter: the iterator callback function
> + * @errp: returns an error if iterator function fails
> + * @opaque: opaque data to pass to @iter
> + *
> + * Iterates over all properties defined against the object
> + * instance calling @iter for each property.

Probably should mention that there is an early exit if error gets set [2]

> + *
> + * It is forbidden to modify the property list from @iter
> + * whether removing or adding properties.
> + */
> +void object_property_foreach(Object *obj,
> +                             ObjectPropertyIterator iter,
> +                             Error **errp,
> +                             void *opaque);

[1] if we allow the iterator to return a non-zero value to abort
iteration (particularly if it wants to abort iteration without setting
an error, just to save CPU cycles), should this foreach() function
return that value?

Of course, a caller can always use opaque to achieve the same purpose,
but it gets more verbose (the caller has to reserve space in their
opaque for tracking whether an interesting exit value is needed, as well
as having an early check on whether the value was already set in a
previous visit) and burns more CPU cycles (the iterator runs to
completion, even though the later callbacks are doing nothing).

> +++ b/qom/object.c
> @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
>      return NULL;
>  }
>  
> +void object_property_foreach(Object *obj,
> +                             ObjectPropertyIterator iter,
> +                             Error **errp,
> +                             void *opaque)
> +{
> +    ObjectProperty *prop;
> +    Error *local_err = NULL;
> +
> +    QTAILQ_FOREACH(prop, &obj->properties, node) {
> +        iter(obj, prop, &local_err, opaque);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;

[2] there's the early exit if an error is set.

The code looks fine, but I think we need a documentation improvement for
issue [2], and we may want a design change for issue [1] if a non-void
return would be useful to any client later in the series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] qmp: convert to use object_property_foreach iterators

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> Rather than directly traversing the object property list,
> use the object_property_foreach iterator. This removes a
> dependancy on the implementation approach for properties.

s/dependancy/dependency/

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qmp.c | 103 ++++++++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 59 insertions(+), 44 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] vl: convert machine help to use object_property_foreach

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> Rather than directly traversing the object property list,
> use the object_property_foreach iterator. This removes a
> dependancy on the implementation approach for properties.

s/dependancy/dependency/

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  vl.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 4/5] qom: replace object property list with GHashTable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> 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 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 <berrange@redhat.com>
> ---
>  include/qom/object.h | 10 ++++--
>  qom/object.c         | 94 +++++++++++++++++++++++++++++++---------------------
>  2 files changed, 63 insertions(+), 41 deletions(-)
> 
> @@ -879,13 +898,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));

Indentation is now off on the unchanged lines.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qom: allow properties to be registered against classes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> 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 <berrange@redhat.com>
> ---
>  include/qom/object.h |  47 +++++++++-
>  qom/object.c         | 242 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 275 insertions(+), 14 deletions(-)
> 
> @@ -898,10 +903,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));

Ah, the indentation hunk I noticed in 4/5.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] Stick to loops (was: [PATCH v3 1/5] qom: introduce object_property_foreach method)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
>> 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.
>> 
>> Providing an explicit object_property_foreach method provides
>> a layer of insulation between the QOM user and the QOM internal
>> implementation.
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  include/qom/object.h | 23 +++++++++++++++++++++++
>>  qom/object.c         | 17 +++++++++++++++++
>>  2 files changed, 40 insertions(+)
>> 
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index be7280c..71503af 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp);
>>  ObjectProperty *object_property_find(Object *obj, const char *name,
>>                                       Error **errp);
>>  
>> +typedef void (*ObjectPropertyIterator)(Object *obj,
>> +                                       ObjectProperty *prop,
>> +                                       Error **errp,
>> +                                       void *opaque);
>
> Do we want the iterator to be able to return a value, and possibly allow
> a non-zero value to abort iteration? [1]
>
>> +
>> +/**
>> + * object_property_foreach:
>> + * @obj: the object
>> + * @iter: the iterator callback function
>> + * @errp: returns an error if iterator function fails
>> + * @opaque: opaque data to pass to @iter
>> + *
>> + * Iterates over all properties defined against the object
>> + * instance calling @iter for each property.
>
> Probably should mention that there is an early exit if error gets set [2]
>
>> + *
>> + * It is forbidden to modify the property list from @iter
>> + * whether removing or adding properties.
>> + */
>> +void object_property_foreach(Object *obj,
>> +                             ObjectPropertyIterator iter,
>> +                             Error **errp,
>> +                             void *opaque);
>
> [1] if we allow the iterator to return a non-zero value to abort
> iteration (particularly if it wants to abort iteration without setting
> an error, just to save CPU cycles), should this foreach() function
> return that value?
>
> Of course, a caller can always use opaque to achieve the same purpose,
> but it gets more verbose (the caller has to reserve space in their
> opaque for tracking whether an interesting exit value is needed, as well
> as having an early check on whether the value was already set in a
> previous visit) and burns more CPU cycles (the iterator runs to
> completion, even though the later callbacks are doing nothing).
>
>> +++ b/qom/object.c
>> @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
>>      return NULL;
>>  }
>>  
>> +void object_property_foreach(Object *obj,
>> +                             ObjectPropertyIterator iter,
>> +                             Error **errp,
>> +                             void *opaque)
>> +{
>> +    ObjectProperty *prop;
>> +    Error *local_err = NULL;
>> +
>> +    QTAILQ_FOREACH(prop, &obj->properties, node) {
>> +        iter(obj, prop, &local_err, opaque);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>
> [2] there's the early exit if an error is set.
>
> The code looks fine, but I think we need a documentation improvement for
> issue [2], and we may want a design change for issue [1] if a non-void
> return would be useful to any client later in the series.

We have quite a few _foreach-functions to help iterate over various
things.  They are easy enough to write, but I find them awkward to use.

Have a look at this straightforward loop from monitor.c:

            for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
                name = bdrv_get_device_name(bs);
                if (str[0] == '\0' ||
                    !strncmp(name, str, strlen(str))) {
                    readline_add_completion(mon->rs, name);
                }
            }

Note that it encapsulates iteration details just fine.  The loop body
accesses local variables the obvious way.  Breaking the loop would also
be done the obvious way.

Before commit fea68bb, it looked like this:

            mbs.mon = mon;
            mbs.input = str;
            readline_set_completion_index(mon->rs, strlen(str));
            bdrv_iterate(block_completion_it, &mbs);

To actually figure out what this does, you have to look up

* The definition of block_completion_it(), 500 lines up:

    static void block_completion_it(void *opaque, BlockDriverState *bs)
    {
        const char *name = bdrv_get_device_name(bs);
        MonitorBlockComplete *mbc = opaque;
        Monitor *mon = mbc->mon;
        const char *input = mbc->input;

        if (input[0] == '\0' ||
            !strncmp(name, (char *)input, strlen(input))) {
            readline_add_completion(mon->rs, name);
        }
    }

* The definition of mbs, 50 lines up.  It's a MonitorBlockComplete, so
  you get too look that up, too.

* Thankfully, it's defined right next to block_completion_it():

    typedef struct MonitorBlockComplete {
        Monitor *mon;
        const char *input;
    } MonitorBlockComplete;

Twice the code, spread over two distant places (not counting definition
of mbs), type punning, and things would get even more complicated if you
needed to break the loop.

Implementing bdrv_next() is no harder than bdrv_iterate().  Compare:

    BlockDriverState *bdrv_next(BlockDriverState *bs)
    {
        if (!bs) {
            return QTAILQ_FIRST(&bdrv_states);
        }
        return QTAILQ_NEXT(bs, device_list);
    }

    void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
    {
        BlockDriverState *bs;

        QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
            it(opaque, bs);
        }
    }

Higher-order functions are a wonderful tool if the language is equipped
for them.  In Lisp, for instance, you'd have everything in one place and
no need for the awkward marshalling and unmarshalling of arguments,
thanks to nested functions.

In C, stick to loops.  That's what the language supports.

Re: [Qemu-devel] Stick to loops (was: [PATCH v3 1/5] qom: introduce object_property_foreach method)

[Daniel P. Berrange]

On Fri, Oct 09, 2015 at 10:31:26AM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 10/08/2015 08:09 AM, Daniel P. Berrange wrote:
> >> 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.
> >> 
> >> Providing an explicit object_property_foreach method provides
> >> a layer of insulation between the QOM user and the QOM internal
> >> implementation.
> >> 
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >> ---
> >>  include/qom/object.h | 23 +++++++++++++++++++++++
> >>  qom/object.c         | 17 +++++++++++++++++
> >>  2 files changed, 40 insertions(+)
> >> 
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index be7280c..71503af 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp);
> >>  ObjectProperty *object_property_find(Object *obj, const char *name,
> >>                                       Error **errp);
> >>  
> >> +typedef void (*ObjectPropertyIterator)(Object *obj,
> >> +                                       ObjectProperty *prop,
> >> +                                       Error **errp,
> >> +                                       void *opaque);
> >
> > Do we want the iterator to be able to return a value, and possibly allow
> > a non-zero value to abort iteration? [1]
> >
> >> +
> >> +/**
> >> + * object_property_foreach:
> >> + * @obj: the object
> >> + * @iter: the iterator callback function
> >> + * @errp: returns an error if iterator function fails
> >> + * @opaque: opaque data to pass to @iter
> >> + *
> >> + * Iterates over all properties defined against the object
> >> + * instance calling @iter for each property.
> >
> > Probably should mention that there is an early exit if error gets set [2]
> >
> >> + *
> >> + * It is forbidden to modify the property list from @iter
> >> + * whether removing or adding properties.
> >> + */
> >> +void object_property_foreach(Object *obj,
> >> +                             ObjectPropertyIterator iter,
> >> +                             Error **errp,
> >> +                             void *opaque);
> >
> > [1] if we allow the iterator to return a non-zero value to abort
> > iteration (particularly if it wants to abort iteration without setting
> > an error, just to save CPU cycles), should this foreach() function
> > return that value?
> >
> > Of course, a caller can always use opaque to achieve the same purpose,
> > but it gets more verbose (the caller has to reserve space in their
> > opaque for tracking whether an interesting exit value is needed, as well
> > as having an early check on whether the value was already set in a
> > previous visit) and burns more CPU cycles (the iterator runs to
> > completion, even though the later callbacks are doing nothing).
> >
> >> +++ b/qom/object.c
> >> @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
> >>      return NULL;
> >>  }
> >>  
> >> +void object_property_foreach(Object *obj,
> >> +                             ObjectPropertyIterator iter,
> >> +                             Error **errp,
> >> +                             void *opaque)
> >> +{
> >> +    ObjectProperty *prop;
> >> +    Error *local_err = NULL;
> >> +
> >> +    QTAILQ_FOREACH(prop, &obj->properties, node) {
> >> +        iter(obj, prop, &local_err, opaque);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            return;
> >
> > [2] there's the early exit if an error is set.
> >
> > The code looks fine, but I think we need a documentation improvement for
> > issue [2], and we may want a design change for issue [1] if a non-void
> > return would be useful to any client later in the series.
> 
> We have quite a few _foreach-functions to help iterate over various
> things.  They are easy enough to write, but I find them awkward to use.
> 
> Implementing bdrv_next() is no harder than bdrv_iterate().  Compare:
> 
>     BlockDriverState *bdrv_next(BlockDriverState *bs)
>     {
>         if (!bs) {
>             return QTAILQ_FIRST(&bdrv_states);
>         }
>         return QTAILQ_NEXT(bs, device_list);
>     }
> 
>     void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
>     {
>         BlockDriverState *bs;
> 
>         QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>             it(opaque, bs);
>         }
>     }

I don't think your example here is a reasonable comparison when you consider
the full extent of this patch series. You are only having to iterate over a
single data structure here. At the end of this patch series we have to
iterate over multiple data structures spread across the object instance
and class hierarchy, so writing a 'next' like method is not as trivial
as you suggest with this comparison.

> Higher-order functions are a wonderful tool if the language is equipped
> for them.  In Lisp, for instance, you'd have everything in one place and
> no need for the awkward marshalling and unmarshalling of arguments,
> thanks to nested functions.
> 
> In C, stick to loops.  That's what the language supports.

You're really arguing against use of function callbacks in general with
this comparison to LISP. I don't think that's really practical in the
real world, as any integration with event loop needs callbacks, which
share al the same limitations as callbacks used with this foreach()
style iterator. Given this I don't think banning use of callbacks in
one specific scenario is really beneficial in general - its really
just a personal style choice.

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] Stick to loops

[Paolo Bonzini]

On 12/10/2015 12:00, Daniel P. Berrange wrote:
> I don't think your example here is a reasonable comparison when you consider
> the full extent of this patch series. You are only having to iterate over a
> single data structure here. At the end of this patch series we have to
> iterate over multiple data structures spread across the object instance
> and class hierarchy, so writing a 'next' like method is not as trivial
> as you suggest with this comparison.
> 
> > Higher-order functions are a wonderful tool if the language is equipped
> > for them.  In Lisp, for instance, you'd have everything in one place and
> > no need for the awkward marshalling and unmarshalling of arguments,
> > thanks to nested functions.
> > 
> > In C, stick to loops.  That's what the language supports.
> 
> You're really arguing against use of function callbacks in general with
> this comparison to LISP. I don't think that's really practical in the
> real world, as any integration with event loop needs callbacks, which
> share al the same limitations as callbacks used with this foreach()
> style iterator. Given this I don't think banning use of callbacks in
> one specific scenario is really beneficial in general - its really
> just a personal style choice.

I agree with Eric that in general loops are better than callbacks.
However, there are cases where iterators are just as awkward to write.
Considering that we have very few iterations on properties, I think your
patches are fine.

Paolo

Re: [Qemu-devel] Stick to loops

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/10/2015 12:00, Daniel P. Berrange wrote:
>> I don't think your example here is a reasonable comparison when you consider
>> the full extent of this patch series. You are only having to iterate over a
>> single data structure here. At the end of this patch series we have to
>> iterate over multiple data structures spread across the object instance
>> and class hierarchy, so writing a 'next' like method is not as trivial
>> as you suggest with this comparison.
>> 
>> > Higher-order functions are a wonderful tool if the language is equipped
>> > for them.  In Lisp, for instance, you'd have everything in one place and
>> > no need for the awkward marshalling and unmarshalling of arguments,
>> > thanks to nested functions.
>> > 
>> > In C, stick to loops.  That's what the language supports.
>> 
>> You're really arguing against use of function callbacks in general with
>> this comparison to LISP. I don't think that's really practical in the
>> real world, as any integration with event loop needs callbacks, which
>> share al the same limitations as callbacks used with this foreach()
>> style iterator. Given this I don't think banning use of callbacks in
>> one specific scenario is really beneficial in general - its really
>> just a personal style choice.

You're right, I do think callbacks in C are relatively awkward.  But
then you're attacking a strawman --- I did not advocate *banning*
callbacks.  That would be silly indeed.  I did advocate for using them
*sparingly*.

There are cases where callbacks are simply required, and you quoted one.

There are cases where callbacks compete with other techniques, and I
quoted one.

> I agree with Eric that in general loops are better than callbacks.

Me.  Eric's innocent :)

> However, there are cases where iterators are just as awkward to write.
> Considering that we have very few iterations on properties, I think your
> patches are fine.

Mind, I didn't actually object to them.  I merely voiced my reasoned
opinion on idiomatic iteration in C: stick to loops.

Just to deter strawmen: that's a rule of thumb, not a law :)

Re: [Qemu-devel] Stick to loops (was: [PATCH v3 1/5] qom: introduce object_property_foreach method)

[Daniel P. Berrange]

On Mon, Oct 12, 2015 at 11:00:04AM +0100, Daniel P. Berrange wrote:
> On Fri, Oct 09, 2015 at 10:31:26AM +0200, Markus Armbruster wrote:
> > We have quite a few _foreach-functions to help iterate over various
> > things.  They are easy enough to write, but I find them awkward to use.
> > 
> > Implementing bdrv_next() is no harder than bdrv_iterate().  Compare:
> > 
> >     BlockDriverState *bdrv_next(BlockDriverState *bs)
> >     {
> >         if (!bs) {
> >             return QTAILQ_FIRST(&bdrv_states);
> >         }
> >         return QTAILQ_NEXT(bs, device_list);
> >     }
> > 
> >     void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> >     {
> >         BlockDriverState *bs;
> > 
> >         QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> >             it(opaque, bs);
> >         }
> >     }
> 
> I don't think your example here is a reasonable comparison when you consider
> the full extent of this patch series. You are only having to iterate over a
> single data structure here. At the end of this patch series we have to
> iterate over multiple data structures spread across the object instance
> and class hierarchy, so writing a 'next' like method is not as trivial
> as you suggest with this comparison.

So it turns out I was wrong here. After a little more thinking I found
it was in fact fairly trivial to support a "next" like iterator in this
QOM property scenario, even when taking class properties into account.
So I'll re-spin this patch series with that approach, since it makes
the diffs much smaller

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