[Qemu-devel] [PATCH v4 0/7] qom: more efficient object property handling

[Qemu-devel] [PATCH v4 0/7] qom: more efficient object property handling

[Daniel P. Berrange]

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.

Changed in v4:

 - Create ObjectPropertyIterator struct to allow inline
   iteration, instead of using callback functions
 - Convert spapr and net filter code to new iterators too
 - Extend check-qom-proplist.c test to cover new iterators
   and class property usage

Daniel P. Berrange (6):
  qom: introduce ObjectPropertyIterator struct for iteration
  qmp: convert QMP code to use object property iterators
  vl: convert machine help code to use object property iterators
  ppc: convert spapr code to use object property iterators
  net: convert net filter code to use object property iterators
  qom: allow properties to be registered against classes

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

 hw/ppc/spapr_drc.c         |   5 +-
 include/qom/object.h       | 110 ++++++++++++++-
 net/filter.c               |   5 +-
 qmp.c                      |  10 +-
 qom/object.c               | 328 ++++++++++++++++++++++++++++++++++++++++-----
 tests/check-qom-proplist.c |  77 +++++++++--
 vl.c                       |   5 +-
 7 files changed, 485 insertions(+), 55 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 1/7] qom: introduce ObjectPropertyIterator struct for iteration

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

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 <berrange@redhat.com>
---
 include/qom/object.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 31 ++++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index be7280c..761ffec 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -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);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 4805328..7dace59 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -67,6 +67,10 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
+struct ObjectPropertyIterator {
+    ObjectProperty *next;
+};
+
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -917,6 +921,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);
+}
+
+
+ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
+{
+    ObjectProperty *ret = iter->next;
+    if (ret) {
+        iter->next = QTAILQ_NEXT(iter->next, node);
+    }
+    return ret;
+}
+
+
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 7400b1f..1be8b9e 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -283,6 +283,51 @@ static void test_dummy_getenum(void)
                                    &err);
     g_assert(err != NULL);
     error_free(err);
+
+    object_unparent(OBJECT(dobj));
+}
+
+
+static void test_dummy_iterator(void)
+{
+    Object *parent = object_get_objects_root();
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_with_props(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              &error_abort,
+                              "bv", "yes",
+                              "sv", "Hiss hiss hiss",
+                              "av", "platypus",
+                              NULL));
+
+    ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
+    bool seenbv = false, seensv = false, seenav = false, seentype;
+
+    iter = object_property_iter_init(OBJECT(dobj));
+    while ((prop = object_property_iter_next(iter))) {
+        if (g_str_equal(prop->name, "bv")) {
+            seenbv = true;
+        } else if (g_str_equal(prop->name, "sv")) {
+            seensv = true;
+        } else if (g_str_equal(prop->name, "av")) {
+            seenav = true;
+        } else if (g_str_equal(prop->name, "type")) {
+            /* This prop comes from the base Object class */
+            seentype = true;
+        } else {
+            g_printerr("Found prop '%s'\n", prop->name);
+            g_assert_not_reached();
+        }
+    }
+    object_property_iter_free(iter);
+    g_assert(seenbv);
+    g_assert(seenav);
+    g_assert(seensv);
+    g_assert(seentype);
+
+    object_unparent(OBJECT(dobj));
 }
 
 
@@ -297,6 +342,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qom/proplist/createv", test_dummy_createv);
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
+    g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
 
     return g_test_run();
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4 1/7] qom: introduce ObjectPropertyIterator struct for iteration

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb 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.
> 
> 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 <berrange@redhat.com>
> ---
>  include/qom/object.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  qom/object.c               | 31 ++++++++++++++++++++++++++++
>  tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index be7280c..761ffec 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -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;

Shouldn't this be ObjectPropertyIterator?

> + *
> + *   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

Returns:

> + */
> +ObjectPropertyIterator *object_property_iter_init(Object *obj);
> +
> +

Intentionally two lines here?

> +/**
> + * object_property_iter_free:
> + * @iter: the iterator instance
> + *
> + * Release any resources associated with the iterator

Releases ... full stop.

> + */
> +void object_property_iter_free(ObjectPropertyIterator *iter);
> +
> +/**
> + * object_property_iter_next:
> + * @iter: the iterator instance
> + *
> + * Returns the next property, or NULL when all properties

%NULL

> + * have been traversed.
> + */
> +ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
> +
>  void object_unparent(Object *obj);
>  
>  /**
> diff --git a/qom/object.c b/qom/object.c
> index 4805328..7dace59 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -67,6 +67,10 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> +struct ObjectPropertyIterator {
> +    ObjectProperty *next;
> +};
> +
>  static Type type_interface;
>  
>  static GHashTable *type_table_get(void)
> @@ -917,6 +921,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);
> +}
> +
> +
> +ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
> +{
> +    ObjectProperty *ret = iter->next;
> +    if (ret) {
> +        iter->next = QTAILQ_NEXT(iter->next, node);
> +    }
> +    return ret;
> +}
> +
> +

?

>  void object_property_del(Object *obj, const char *name, Error **errp)
>  {
>      ObjectProperty *prop = object_property_find(obj, name, errp);
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index 7400b1f..1be8b9e 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -283,6 +283,51 @@ static void test_dummy_getenum(void)
>                                     &err);
>      g_assert(err != NULL);
>      error_free(err);
> +
> +    object_unparent(OBJECT(dobj));
> +}
> +
> +
> +static void test_dummy_iterator(void)
> +{
> +    Object *parent = object_get_objects_root();
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_with_props(TYPE_DUMMY,
> +                              parent,
> +                              "dummy0",
> +                              &error_abort,
> +                              "bv", "yes",
> +                              "sv", "Hiss hiss hiss",
> +                              "av", "platypus",
> +                              NULL));
> +
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator *iter;
> +    bool seenbv = false, seensv = false, seenav = false, seentype;
> +
> +    iter = object_property_iter_init(OBJECT(dobj));
> +    while ((prop = object_property_iter_next(iter))) {
> +        if (g_str_equal(prop->name, "bv")) {
> +            seenbv = true;
> +        } else if (g_str_equal(prop->name, "sv")) {
> +            seensv = true;
> +        } else if (g_str_equal(prop->name, "av")) {
> +            seenav = true;
> +        } else if (g_str_equal(prop->name, "type")) {
> +            /* This prop comes from the base Object class */
> +            seentype = true;
> +        } else {
> +            g_printerr("Found prop '%s'\n", prop->name);
> +            g_assert_not_reached();
> +        }
> +    }
> +    object_property_iter_free(iter);
> +    g_assert(seenbv);
> +    g_assert(seenav);
> +    g_assert(seensv);
> +    g_assert(seentype);
> +
> +    object_unparent(OBJECT(dobj));
>  }
>  
>  
> @@ -297,6 +342,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/qom/proplist/createv", test_dummy_createv);
>      g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
>      g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
> +    g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
>  
>      return g_test_run();
>  }

Pavel, note that any of you could've found such issues or at least
provided a Reviewed-by rather than just pinging.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 1/7] qom: introduce ObjectPropertyIterator struct for iteration

[Markus Armbruster]

I apologize for the lateness of my review.

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

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

ObjectPropertyIterator *iter;

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

This is a sane way to do iterators.  It combines allocation and
initialization, thus requires a free.  The name _init() suggests it
doesn't, but that's easy enough to fix.  I can't find precedence for
this way in the tree, however.

Another way is

    ObjectProperty *prop;
    ObjectPropertyIterator iter;

    object_property_iter_init(&iter, obj);
    while ((prop = object_property_iter_next(iter))) {
        ... do something with prop ...
    }
    object_property_iter_fini(iter); // usually not needed

Leaves allocation to the caller, which makes it more flexible.
Automartic iter often suffices, and can't leak.  The _fini() is commonly
empty and left out.

Precedence:
* hbitmap_iter_init() and hbitmap_iter_next()
* g_hash_table_iter_init() and g_hash_table_iter_next(), except the
  latter is a bit different because it returns two values

Yet another one is

    for (prop = object_property_first(obj);
         prop;
         prop = object_property_next(prop)) {
        ... do something with prop ...
    }

This works only when the iterator state is exactly what the _next()
returns.  Happens to be the case often enough.  Here, too, but perhaps
you have reasons to prepare for more complex state.

Precedence:
* qdict_first(), qdict_next()
* vma_first(), vma_next()
* ...

Related: bdrv_next(), blk_next(), ...  These guys omit _first() because
the set to iterate over is implied.

I recommend to pick the precedence you like best for this purpose and
follow it.

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qom/object.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  qom/object.c               | 31 ++++++++++++++++++++++++++++
>  tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index be7280c..761ffec 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -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);
>  
>  /**
> diff --git a/qom/object.c b/qom/object.c
> index 4805328..7dace59 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -67,6 +67,10 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> +struct ObjectPropertyIterator {
> +    ObjectProperty *next;
> +};
> +
>  static Type type_interface;
>  
>  static GHashTable *type_table_get(void)
> @@ -917,6 +921,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);

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

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

Re: [Qemu-devel] [PATCH v4 1/7] qom: introduce ObjectPropertyIterator struct for iteration

[Daniel P. Berrange]

On Tue, Nov 17, 2015 at 04:25:22PM +0100, Markus Armbruster wrote:
> I apologize for the lateness of my review.

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

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 v4 1/7] qom: introduce ObjectPropertyIterator struct for iteration

[Markus Armbruster]

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

> On Tue, Nov 17, 2015 at 04:25:22PM +0100, Markus Armbruster wrote:
>> I apologize for the lateness of my review.
>
>> > +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.

I'd add the conditional when it's actually needed, not least to avoid
comments from picky reviewers like myself ;)  Anyway, not worth a respin
by itself.

However, I'd very much prefer the iterators to follow precedence in the
tree.

[Qemu-devel] [PATCH v4 2/7] qmp: convert QMP code to use object property iterators

[Daniel P. Berrange]

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

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

diff --git a/qmp.c b/qmp.c
index d9ecede..5d99439 100644
--- a/qmp.c
+++ b/qmp.c
@@ -208,6 +208,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
     bool ambiguous = false;
     ObjectPropertyInfoList *props = NULL;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
 
     obj = object_resolve_path(path, &ambiguous);
     if (obj == NULL) {
@@ -220,7 +221,8 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         return NULL;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    iter = object_property_iter_init(obj);
+    while ((prop = object_property_iter_next(iter))) {
         ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
 
         entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
@@ -230,6 +232,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         entry->value->name = g_strdup(prop->name);
         entry->value->type = g_strdup(prop->type);
     }
+    object_property_iter_free(iter);
 
     return props;
 }
@@ -500,6 +503,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
     ObjectClass *klass;
     Object *obj;
     ObjectProperty *prop;
+    ObjectPropertyIterator *iter;
     DevicePropertyInfoList *prop_list = NULL;
 
     klass = object_class_by_name(typename);
@@ -528,7 +532,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
 
     obj = object_new(typename);
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    iter = object_property_iter_init(obj);
+    while ((prop = object_property_iter_next(iter))) {
         DevicePropertyInfo *info;
         DevicePropertyInfoList *entry;
 
@@ -559,6 +564,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
         entry->next = prop_list;
         prop_list = entry;
     }
+    object_property_iter_free(iter);
 
     object_unref(obj);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4 2/7] qmp: convert QMP code to use object property iterators

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> Stop directly accessing the Object "properties" field data
> structure and instead use the formal object property iterator
> APIs. This insulates the code from future data structure
> changes in the Object struct.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qmp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

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

Thanks,
Andreas

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

Re: [Qemu-devel] [PATCH v4 2/7] qmp: convert QMP code to use object property iterators

[Markus Armbruster]

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

> Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
>> Stop directly accessing the Object "properties" field data
>> structure and instead use the formal object property iterator
>> APIs. This insulates the code from future data structure
>> changes in the Object struct.
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  qmp.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Markus/Luiz, can you ack this change? Markus requested these iterators,
> I believe.

See my review of PATCH 1.

[Qemu-devel] [PATCH v4 3/7] vl: convert machine help code to use object property iterators

[Daniel P. Berrange]

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH v4 3/7] vl: convert machine help code to use object property iterators

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> Stop directly accessing the Object "properties" field data
> structure and instead use the formal object property iterator
> APIs. This insulates the code from future data structure
> changes in the Object struct.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

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

[Qemu-devel] [PATCH v4 4/7] ppc: convert spapr code to use object property iterators

[Daniel P. Berrange]

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/ppc/spapr_drc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH v4 4/7] ppc: convert spapr code to use object property iterators

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> Stop directly accessing the Object "properties" field data

Object::properties data structure for short.

> structure and instead use the formal object property iterator
> APIs. This insulates the code from future data structure
> changes in the Object struct.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 5d6ea7c..f34bc04 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -657,6 +657,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>  {
>      Object *root_container;
>      ObjectProperty *prop;
> +    ObjectPropertyIterator *iter;
>      uint32_t drc_count = 0;
>      GArray *drc_indexes, *drc_power_domains;
>      GString *drc_names, *drc_types;
> @@ -680,7 +681,8 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>       */
>      root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
>  
> -    QTAILQ_FOREACH(prop, &root_container->properties, node) {
> +    iter = object_property_iter_init(root_container);
> +    while ((prop = object_property_iter_next(iter))) {
>          Object *obj;
>          sPAPRDRConnector *drc;
>          sPAPRDRConnectorClass *drck;
> @@ -721,6 +723,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>                                      spapr_drc_get_type_str(drc->type));
>          drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
>      }
> +    object_property_iter_free(iter);
>  
>      /* now write the drc count into the space we reserved at the
>       * beginning of the arrays previously

Reviewed-by: Andreas Färber <afaerber@suse.de>

CC'ing PPC for ack.

Regards,
Andreas

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

[Qemu-devel] [PATCH v4 5/7] net: convert net filter code to use object property iterators

[Daniel P. Berrange]

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 net/filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH v4 5/7] net: convert net filter code to use object property iterators

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> Stop directly accessing the Object "properties" field data

Object::properties data ...

> structure and instead use the formal object property iterator
> APIs. This insulates the code from future data structure
> changes in the Object struct.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  net/filter.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/filter.c b/net/filter.c
> index 326f2b5..1365bad 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -137,6 +137,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      Error *local_err = NULL;
>      char *str, *info;
>      ObjectProperty *prop;
> +    ObjectPropertyIterator *iter;
>      StringOutputVisitor *ov;
>  
>      if (!nf->netdev_id) {
> @@ -173,7 +174,8 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>  
>      /* generate info str */
> -    QTAILQ_FOREACH(prop, &OBJECT(nf)->properties, node) {
> +    iter = object_property_iter_init(OBJECT(nf));
> +    while ((prop = object_property_iter_next(iter))) {
>          if (!strcmp(prop->name, "type")) {
>              continue;
>          }
> @@ -187,6 +189,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>          g_free(str);
>          g_free(info);
>      }
> +    object_property_iter_free(iter);
>  }
>  
>  static void netfilter_finalize(Object *obj)

Reviewed-by: Andreas Färber <afaerber@suse.de>

CC'ing Stefan.

Regards,
Andreas

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

[Qemu-devel] [PATCH v4 6/7] 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>
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 include/qom/object.h | 10 ++++--
 qom/object.c         | 98 ++++++++++++++++++++++++++++++----------------------
 2 files changed, 64 insertions(+), 44 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 761ffec..2a54515 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;
 };
@@ -1538,6 +1536,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),
@@ -1553,6 +1554,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 7dace59..dd01652 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -68,7 +68,7 @@ struct TypeImpl
 };
 
 struct ObjectPropertyIterator {
-    ObjectProperty *next;
+    GHashTableIter iter;
 };
 
 static Type type_interface;
@@ -330,6 +330,16 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void property_free(gpointer data)
+{
+    ObjectProperty *prop = data;
+
+    g_free(prop->name);
+    g_free(prop->type);
+    g_free(prop->description);
+    g_free(prop);
+}
+
 void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
 {
     Object *obj = data;
@@ -344,7 +354,8 @@ void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
     object_ref(obj);
-    QTAILQ_INIT(&obj->properties);
+    obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
+                                            NULL, property_free);
     object_init_with_type(obj, type);
     object_post_init_with_type(obj, type);
 }
@@ -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;
 
+    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;
         }
     }
@@ -783,10 +800,12 @@ static int do_object_child_foreach(Object *obj,
                                    int (*fn)(Object *child, void *opaque),
                                    void *opaque, bool recurse)
 {
-    ObjectProperty *prop, *next;
+    GHashTableIter iter;
+    ObjectProperty *prop;
     int ret = 0;
 
-    QTAILQ_FOREACH_SAFE(prop, &obj->properties, node, next) {
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         if (object_property_is_child(prop)) {
             Object *child = prop->opaque;
 
@@ -883,13 +902,11 @@ object_property_add(Object *obj, const char *name, const char *type,
         return ret;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (strcmp(prop->name, name) == 0) {
-            error_setg(errp, "attempt to add duplicate property '%s'"
+    if (g_hash_table_contains(obj->properties, name)) {
+        error_setg(errp, "attempt to add duplicate property '%s'"
                        " to object (type '%s')", name,
                        object_get_typename(obj));
-            return NULL;
-        }
+        return NULL;
     }
 
     prop = g_malloc0(sizeof(*prop));
@@ -902,7 +919,7 @@ object_property_add(Object *obj, const char *name, const char *type,
     prop->release = release;
     prop->opaque = opaque;
 
-    QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
+    g_hash_table_insert(obj->properties, prop->name, prop);
     return prop;
 }
 
@@ -911,10 +928,9 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
 {
     ObjectProperty *prop;
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (strcmp(prop->name, name) == 0) {
-            return prop;
-        }
+    prop = g_hash_table_lookup(obj->properties, name);
+    if (prop) {
+        return prop;
     }
 
     error_setg(errp, "Property '.%s' not found", name);
@@ -924,7 +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;
 }
 
@@ -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;
 }
 
 
 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,
@@ -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;
 
     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;
         }
@@ -1572,11 +1586,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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb 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.

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

I believe that limitation is fine.

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  include/qom/object.h | 10 ++++--
>  qom/object.c         | 98 ++++++++++++++++++++++++++++++----------------------
>  2 files changed, 64 insertions(+), 44 deletions(-)
[...]
> diff --git a/qom/object.c b/qom/object.c
> index 7dace59..dd01652 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -68,7 +68,7 @@ struct TypeImpl
>  };
>  
>  struct ObjectPropertyIterator {
> -    ObjectProperty *next;
> +    GHashTableIter iter;
>  };
>  
>  static Type type_interface;
> @@ -330,6 +330,16 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>      }
>  }
>  
> +static void property_free(gpointer data)

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

> +{
> +    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;
[...]
> @@ -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;
>  
> +    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?

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

Ditto?

> +            g_hash_table_iter_remove(&iter);
>              break;
>          }
>      }
[...]
> @@ -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?

> @@ -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;
>  }
>  
>  
>  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?

>          return;
>      }
>  
>      if (prop->release) {
>          prop->release(obj, name, prop->opaque);
>      }

property_free()?

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

Is this cast needed?

>          if (!object_property_is_child(prop)) {
>              continue;
>          }
> @@ -1572,11 +1586,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)) {

Ditto?

>          Object *found;
>  
>          if (!object_property_is_child(prop)) {

Otherwise looks very good, but third pair of eyes appreciated (Markus?).

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Pavel Fedin]

 Hello!

> >  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);
> >          }
> 
> Why is this not in property_free(), too? Is there a timing difference?

 This is what i started from, and got NAKed. property_free() gets only ObjectProperty * as argument, but for our release callback we
need also 'obj'. In my first version i added Object * backpointer to ObjectProperty and Daniel reported a problem with that:
--- cut ---
I have a patch which adds property registration against the
class, so requiring ObjectProperty to have a back-poointer
to an object instance is not desirable.
--- cut ---
 Full message here: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01999.html

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

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Daniel P. Berrange]

On Thu, Nov 05, 2015 at 07:05:48PM +0100, Andreas Färber wrote:
> Am 13.10.2015 um 14:37 schrieb 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.
> 
> "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.

Yeah, that's correct.

> >  2 files changed, 64 insertions(+), 44 deletions(-)
> [...]
> > diff --git a/qom/object.c b/qom/object.c
> > index 7dace59..dd01652 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -68,7 +68,7 @@ struct TypeImpl
> >  };
> >  
> >  struct ObjectPropertyIterator {
> > -    ObjectProperty *next;
> > +    GHashTableIter iter;
> >  };
> >  
> >  static Type type_interface;
> > @@ -330,6 +330,16 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
> >      }
> >  }
> >  
> > +static void property_free(gpointer data)
> 
> Bikeshed: We might call this object_property_free() unless there's a
> precedence for property_...?

Sure, object_property_free() sounds fine.

> 
> > +{
> > +    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;
> [...]
> > @@ -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;
> >  
> > +    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?

To have this be part of property_free() would require that the
ObjectProperty class have a back-pointer to the Object * that
owns it. We want to use ObjectProperty from ObjectClass *
too though, so we don't really want to have such a Object *
backpointer.

> > @@ -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?

If you use the GHashTable approach, then the caller needs to allocate
the ObjectPropertyIterator which means it has to be a public typedef.
I wanted to keep the ObjectPropertyIterator struct contents private,
and have object_property_iter_init() return the allocated struct.

> 
> > @@ -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;
> >  }
> >  
> >  
> >  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?

No,  object_property_find() will return exactly the same error as
this new code does.


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

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

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 v4 6/7] qom: replace object property list with GHashTable

[Pavel Fedin]

 Hello!

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

 But after '&' it becomes "**", not just "*", and this implicit conversion
rule doesn't apply any more. "void **" and "something **" are always considered
different.	

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

Am 13.10.2015 um 14:37 schrieb 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>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

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

diff --git a/qom/object.c b/qom/object.c
index 0ac3bc1..9aa6159 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -864,7 +864,7 @@ void object_unref(Object *obj)
     if (!obj) {
         return;
     }
-    g_assert(obj->ref > 0);
+    g_assert_cmpint(obj->ref, >, 0);

     /* parent always holds a reference to its children */
     if (atomic_fetch_dec(&obj->ref) == 1) {

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Christian Borntraeger]

On 11/13/2015 07:14 PM, Andreas Färber wrote:
> Am 13.10.2015 um 14:37 schrieb 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>
>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> 
> (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.

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

Am 13.11.2015 um 22:00 schrieb Christian Borntraeger:
> On 11/13/2015 07:14 PM, Andreas Färber wrote:
>> Am 13.10.2015 um 14:37 schrieb 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>
>>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>>
>> (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.
> 
> A simpler reproducer is
> s390x-softmmu/qemu-system-s390x  -device sclp,help
> which fails with this patch and succeeds without.

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.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Pavel Fedin]

 Hello!

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

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

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Christian Borntraeger]

On 11/16/2015 08:13 AM, Pavel Fedin wrote:
>  Hello!
> 
>>>> (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.
> 
>> 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:

> 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>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

Am 16.11.2015 um 09:16 schrieb Christian Borntraeger:
> On 11/16/2015 08:13 AM, Pavel Fedin wrote:
>>>>> (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.
>>
>>> 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:
> 
>> 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>
>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> 
> 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.

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

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Pavel Fedin]

 Hello!

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

 Hm...
 May be instead of actually deleting a property, while we are iterating, we should mark it as pending for deletion, and then, after
iteration is done, actually remove them? This would cost one 'bool delete_pending' per property.

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

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

Am 16.11.2015 um 10:38 schrieb Andreas Färber:
> Am 16.11.2015 um 09:16 schrieb Christian Borntraeger:
>> On 11/16/2015 08:13 AM, Pavel Fedin wrote:
>>>>>> (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.
>>>
>>>> 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:
>>
>>> 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>
>>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>>
>> 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.
> 
>>>  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().

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.

Thanks,
Andreas

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

[-- Attachment #2: 00.diff --]
[-- Type: text/x-patch, Size: 2129 bytes --]

diff --git a/qom/object.c b/qom/object.c
index 0ac3bc1..284fa38 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -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)
@@ -864,7 +880,7 @@ void object_unref(Object *obj)
     if (!obj) {
         return;
     }
-    g_assert(obj->ref > 0);
+    g_assert_cmpint(obj->ref, >, 0);
 
     /* parent always holds a reference to its children */
     if (atomic_fetch_dec(&obj->ref) == 1) {

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Daniel P. Berrange]

On Mon, Nov 16, 2015 at 05:44:35PM +0100, Andreas Färber wrote:
> Am 16.11.2015 um 10:38 schrieb Andreas Färber:
> > Am 16.11.2015 um 09:16 schrieb Christian Borntraeger:
> >> On 11/16/2015 08:13 AM, Pavel Fedin wrote:
> >>>>>> (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.
> >>>
> >>>> 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:
> >>
> >>> 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>
> >>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >>
> >> 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.
> > 
> >>>  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().
> 
> 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.

> diff --git a/qom/object.c b/qom/object.c
> index 0ac3bc1..284fa38 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -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)
> @@ -864,7 +880,7 @@ void object_unref(Object *obj)
>      if (!obj) {
>          return;
>      }
> -    g_assert(obj->ref > 0);
> +    g_assert_cmpint(obj->ref, >, 0);
>  
>      /* parent always holds a reference to its children */
>      if (atomic_fetch_dec(&obj->ref) == 1) {

This looks good to me so can add Signed-off-by: Daniel P. Berrange
to this change.

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 v4 6/7] qom: replace object property list with GHashTable

[Paolo Bonzini]

On 13/11/2015 22:25, Andreas Färber wrote:
> 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.

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

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

Paolo

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

Am 16.11.2015 um 09:53 schrieb Paolo Bonzini:
> On 13/11/2015 22:25, Andreas Färber wrote:
>> 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.

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

Luckily that's not the problem here. The unref should account for its
lifetime.

>> and uses a TYPE_... constant as property name.
> 
> That's just a little weird, it doesn't break anything per se.

It risks renaming the type causing the property to change name, which
affects ABI stability. Therefore we always "inline" property names. Will
adjust after I have a fix.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Paolo Bonzini]

On 16/11/2015 10:48, Andreas Färber wrote:
> > > 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.

Why is realize too late to set the parent bus?

Paolo

Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Daniel P. Berrange]

On Fri, Nov 13, 2015 at 10:00:58PM +0100, Christian Borntraeger wrote:
> On 11/13/2015 07:14 PM, Andreas Färber wrote:
> > Am 13.10.2015 um 14:37 schrieb 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>
> >> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> > 
> > (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.
> 
> A simpler reproducer is
> s390x-softmmu/qemu-system-s390x  -device sclp,help
> which fails with this patch and succeeds without.

The problems arise when unref'ing the object, which is where we iterate
over properties deleting them. From my investigation it seems we have
a re-entrancy issue where by child objects are calling back to the
parent to delete properties while iteration is taking place. The glib
hash iterators are not re-entrant safe, so this causes the glib
errors showing we're modifying the hash table while iteration is
taking place.

(qemu-system-s390x:21872): GLib-CRITICAL **: iter_remove_or_steal: assertion 'ri->version == ri->hash_table->version' failed

I'm looking into how to fix this now...

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

[Qemu-devel] [PATCH v4 7/7] 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       |  50 ++++++++++
 qom/object.c               | 237 ++++++++++++++++++++++++++++++++++++++++++---
 tests/check-qom-proplist.c |  31 ++++--
 3 files changed, 293 insertions(+), 25 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 2a54515..38f41d3 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 struct ObjectPropertyIterator ObjectPropertyIterator;
 
@@ -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
  *
  * 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:
diff --git a/qom/object.c b/qom/object.c
index dd01652..f41edf4 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -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;
@@ -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);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1be8b9e..87de80b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -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

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

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: qemu-devel-bounces+p.fedin=samsung.com@nongnu.org [mailto:qemu-devel-
> bounces+p.fedin=samsung.com@nongnu.org] On Behalf Of Daniel P. Berrange
> Sent: Tuesday, October 13, 2015 3:38 PM
> To: qemu-devel@nongnu.org
> 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 <berrange@redhat.com>
> ---
>  include/qom/object.h       |  50 ++++++++++
>  qom/object.c               | 237 ++++++++++++++++++++++++++++++++++++++++++---
>  tests/check-qom-proplist.c |  31 ++++--
>  3 files changed, 293 insertions(+), 25 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 2a54515..38f41d3 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 struct ObjectPropertyIterator ObjectPropertyIterator;
> 
> @@ -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

>   *
>   * 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:
> diff --git a/qom/object.c b/qom/object.c
> index dd01652..f41edf4 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -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;
> @@ -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);
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index 1be8b9e..87de80b 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -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

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

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

[Andreas Färber]

Am 13.10.2015 um 15:18 schrieb Pavel Fedin:
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 2a54515..38f41d3 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
[...]
>> @@ -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

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.

Regards,
Andreas

P.S. Pavel, please delete unneeded quotation from your reply, I only
found that one line of inline comment.

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

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

[Daniel P. Berrange]

On Thu, Nov 05, 2015 at 07:12:39PM +0100, Andreas Färber wrote:
> Am 13.10.2015 um 15:18 schrieb Pavel Fedin:
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index 2a54515..38f41d3 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> [...]
> >> @@ -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
> 
> 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.


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 v4 7/7] qom: allow properties to be registered against classes

[Andreas Färber]

Am 06.11.2015 um 10:32 schrieb Daniel P. Berrange:
> On Thu, Nov 05, 2015 at 07:12:39PM +0100, Andreas Färber wrote:
>> Am 13.10.2015 um 15:18 schrieb Pavel Fedin:
>>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>>> index 2a54515..38f41d3 100644
>>>> --- a/include/qom/object.h
>>>> +++ b/include/qom/object.h
>> [...]
>>>> @@ -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
>>
>> 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.

Fixed up and queued on qom-next for next merge window:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

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

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

[Andreas Färber]

Daniel,

Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> 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.

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

Regards,
Andreas

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

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

[Daniel P. Berrange]

On Tue, Oct 13, 2015 at 02:54:14PM +0200, Andreas Färber wrote:
> Daniel,
> 
> Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> > 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.
> 
> 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...

I don't mind either way - if you want to queue my v2 I can rebase on
top of that. Just beware though that my v2 had a latent bug that will
trigger when any code starts to use class level properties - they don't
get reported by QMP. So we'd have to make sure to apply this new series
I have posted in order to fix that bug before we convert any code to
use class props.

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 v4 0/7] qom: more efficient object property handling

[Pavel Fedin]

 Hello!

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

 Tested-by: Pavel Fedin <p.fedin@samsung.com>
 
> 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.

 As i already wrote, current code does not modify the tree. If necessary, it is possible to work around (e. g. make a decision about modification, stop iteration, then do the modification). I think this would pop up anyway if we change list to anything else. IMHO it's better just to acknowledge that we should not modify our tree inside iterator.

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