I apologize for the lateness of my review.
"Daniel P. Berrange" <berra...@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 <berra...@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);
[...]
