| Message ID | 1444739866-14798-2-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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); [...]
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
"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.
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(); }
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(+)