diff mbox

[PULL,02/10] qom: Introduce ObjectPropertyIterator struct for iteration

Message ID 1447879178-5440-3-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber Nov. 18, 2015, 8:39 p.m. UTC
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);

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>
---
 include/qom/object.h       | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c               | 28 ++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 46 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)

Comments

Markus Armbruster Nov. 19, 2015, 9:20 a.m. UTC | #1
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>
Daniel P. Berrangé Nov. 19, 2015, 9:25 a.m. UTC | #2
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
Markus Armbruster Nov. 19, 2015, 9:49 a.m. UTC | #3
"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 :)
Andreas Färber Nov. 19, 2015, 10:24 a.m. UTC | #4
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
Markus Armbruster Nov. 19, 2015, 1:42 p.m. UTC | #5
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 mbox

Patch

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();
 }