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

["Andreas Färber" <afaerber@suse.de>]

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)