Message ID | 1447879178-5440-3-git-send-email-afaerber@suse.de |
---|---|
State | New |
Headers | show |
Andreas Färber <afaerber@suse.de> writes: > From: "Daniel P. Berrange" <berrange@redhat.com> > > 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; > ObjectPropertyIterator *iter; > > iter = object_property_iter_init(obj); > while ((prop = object_property_iter_next(iter))) { > ... do something with prop ... > } > object_property_iter_free(iter); I see my review hasn't been addressed, probably because it came late. Would you accept a follow-up patch to bring the iterator into line with existing ones? > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > Tested-by: Pavel Fedin <p.fedin@samsung.com> > [AF: Fixed examples, style cleanups] > Signed-off-by: Andreas Färber <afaerber@suse.de>
On Thu, Nov 19, 2015 at 10:20:22AM +0100, Markus Armbruster wrote: > Andreas Färber <afaerber@suse.de> writes: > > > From: "Daniel P. Berrange" <berrange@redhat.com> > > > > 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; > > ObjectPropertyIterator *iter; > > > > iter = object_property_iter_init(obj); > > while ((prop = object_property_iter_next(iter))) { > > ... do something with prop ... > > } > > object_property_iter_free(iter); > > I see my review hasn't been addressed, probably because it came late. > Would you accept a follow-up patch to bring the iterator into line with > existing ones? I'll write such a patch if you like, but i guess waiting for it to merge till 2.6 is no big deal ? Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Thu, Nov 19, 2015 at 10:20:22AM +0100, Markus Armbruster wrote: >> Andreas Färber <afaerber@suse.de> writes: >> >> > From: "Daniel P. Berrange" <berrange@redhat.com> >> > >> > 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; >> > ObjectPropertyIterator *iter; >> > >> > iter = object_property_iter_init(obj); >> > while ((prop = object_property_iter_next(iter))) { >> > ... do something with prop ... >> > } >> > object_property_iter_free(iter); >> >> I see my review hasn't been addressed, probably because it came late. >> Would you accept a follow-up patch to bring the iterator into line with >> existing ones? > > I'll write such a patch if you like, but i guess waiting for it to merge > till 2.6 is no big deal ? Not even a little deal :)
Am 19.11.2015 um 10:20 schrieb Markus Armbruster: > Andreas Färber <afaerber@suse.de> writes: > >> From: "Daniel P. Berrange" <berrange@redhat.com> >> >> 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; >> ObjectPropertyIterator *iter; >> >> iter = object_property_iter_init(obj); >> while ((prop = object_property_iter_next(iter))) { >> ... do something with prop ... >> } >> object_property_iter_free(iter); > > I see my review hasn't been addressed, Well, it has, I double-checked that the missing "Iterator" above was already on my branch, therefore my IRC comment pointing you to qom-next. > probably because it came late. Other than that you only seemed to discuss design alternatives, for which neither you nor Daniel provided any actual patch I could've applied. While I regularly do style fixups myself, and with the series missing -rc0 also functional fixes, posting a diff for review/record, I do not see redesigning a 6-patch series as something I can silently do last-minute without full respin, for which -rc1 did not leave time. There was a v3 with iterators, and Pavel pinged v4 twice, I did once too, and the last delay after getting the series to work was only due to me inserting Daniel's test case (legit hardfreeze material), so ... > Would you accept a follow-up patch to bring the iterator into line with > existing ones? ... yes, from my perspective any such cleanups can be done post-2.5. Please note that both patch 6/7 (included) and 7/7 (not in this pull) enhance the iterator, so follow-up patches should be based on qom-next please. Thanks, Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 19.11.2015 um 10:20 schrieb Markus Armbruster: >> Andreas Färber <afaerber@suse.de> writes: >> >>> From: "Daniel P. Berrange" <berrange@redhat.com> >>> >>> 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; >>> ObjectPropertyIterator *iter; >>> >>> iter = object_property_iter_init(obj); >>> while ((prop = object_property_iter_next(iter))) { >>> ... do something with prop ... >>> } >>> object_property_iter_free(iter); >> >> I see my review hasn't been addressed, > > Well, it has, I double-checked that the missing "Iterator" above was > already on my branch, therefore my IRC comment pointing you to qom-next. > >> probably because it came late. > > Other than that you only seemed to discuss design alternatives, for > which neither you nor Daniel provided any actual patch I could've > applied. While I regularly do style fixups myself, and with the series > missing -rc0 also functional fixes, posting a diff for review/record, I > do not see redesigning a 6-patch series as something I can silently do > last-minute without full respin, for which -rc1 did not leave time. I certainly didn't expect you to address my review yourself. You could've replied with a short note asking Dan to address the remainder of my review in a follow-up patch. But no harm done, because I'm not shy following up about remainders of my reviews myself. > There was a v3 with iterators, and Pavel pinged v4 twice, I did once > too, and the last delay after getting the series to work was only due to > me inserting Daniel's test case (legit hardfreeze material), so ... > >> Would you accept a follow-up patch to bring the iterator into line with >> existing ones? > > ... yes, from my perspective any such cleanups can be done post-2.5. By now should be done, even. > Please note that both patch 6/7 (included) and 7/7 (not in this pull) > enhance the iterator, so follow-up patches should be based on qom-next > please. > > Thanks, > Andreas
diff --git a/include/qom/object.h b/include/qom/object.h index 0bb89d4..9f70314 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -960,6 +960,55 @@ 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; + * ObjectPropertyIterator *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 + * + * Releases 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 c0decb6..1c926ce 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,30 @@ 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(); }