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" <address@hidden> 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 <address@hidden>
> ---
>  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);
[...]