| Message ID | 1444313344-16196-2-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/08/2015 08:09 AM, Daniel P. Berrange wrote: > 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. > > Providing an explicit object_property_foreach method provides > a layer of insulation between the QOM user and the QOM internal > implementation. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/qom/object.h | 23 +++++++++++++++++++++++ > qom/object.c | 17 +++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/include/qom/object.h b/include/qom/object.h > index be7280c..71503af 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp); > ObjectProperty *object_property_find(Object *obj, const char *name, > Error **errp); > > +typedef void (*ObjectPropertyIterator)(Object *obj, > + ObjectProperty *prop, > + Error **errp, > + void *opaque); Do we want the iterator to be able to return a value, and possibly allow a non-zero value to abort iteration? [1] > + > +/** > + * object_property_foreach: > + * @obj: the object > + * @iter: the iterator callback function > + * @errp: returns an error if iterator function fails > + * @opaque: opaque data to pass to @iter > + * > + * Iterates over all properties defined against the object > + * instance calling @iter for each property. Probably should mention that there is an early exit if error gets set [2] > + * > + * It is forbidden to modify the property list from @iter > + * whether removing or adding properties. > + */ > +void object_property_foreach(Object *obj, > + ObjectPropertyIterator iter, > + Error **errp, > + void *opaque); [1] if we allow the iterator to return a non-zero value to abort iteration (particularly if it wants to abort iteration without setting an error, just to save CPU cycles), should this foreach() function return that value? Of course, a caller can always use opaque to achieve the same purpose, but it gets more verbose (the caller has to reserve space in their opaque for tracking whether an interesting exit value is needed, as well as having an early check on whether the value was already set in a previous visit) and burns more CPU cycles (the iterator runs to completion, even though the later callbacks are doing nothing). > +++ b/qom/object.c > @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name, > return NULL; > } > > +void object_property_foreach(Object *obj, > + ObjectPropertyIterator iter, > + Error **errp, > + void *opaque) > +{ > + ObjectProperty *prop; > + Error *local_err = NULL; > + > + QTAILQ_FOREACH(prop, &obj->properties, node) { > + iter(obj, prop, &local_err, opaque); > + if (local_err) { > + error_propagate(errp, local_err); > + return; [2] there's the early exit if an error is set. The code looks fine, but I think we need a documentation improvement for issue [2], and we may want a design change for issue [1] if a non-void return would be useful to any client later in the series.
Eric Blake <eblake@redhat.com> writes: > On 10/08/2015 08:09 AM, Daniel P. Berrange wrote: >> 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. >> >> Providing an explicit object_property_foreach method provides >> a layer of insulation between the QOM user and the QOM internal >> implementation. >> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- >> include/qom/object.h | 23 +++++++++++++++++++++++ >> qom/object.c | 17 +++++++++++++++++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/include/qom/object.h b/include/qom/object.h >> index be7280c..71503af 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp); >> ObjectProperty *object_property_find(Object *obj, const char *name, >> Error **errp); >> >> +typedef void (*ObjectPropertyIterator)(Object *obj, >> + ObjectProperty *prop, >> + Error **errp, >> + void *opaque); > > Do we want the iterator to be able to return a value, and possibly allow > a non-zero value to abort iteration? [1] > >> + >> +/** >> + * object_property_foreach: >> + * @obj: the object >> + * @iter: the iterator callback function >> + * @errp: returns an error if iterator function fails >> + * @opaque: opaque data to pass to @iter >> + * >> + * Iterates over all properties defined against the object >> + * instance calling @iter for each property. > > Probably should mention that there is an early exit if error gets set [2] > >> + * >> + * It is forbidden to modify the property list from @iter >> + * whether removing or adding properties. >> + */ >> +void object_property_foreach(Object *obj, >> + ObjectPropertyIterator iter, >> + Error **errp, >> + void *opaque); > > [1] if we allow the iterator to return a non-zero value to abort > iteration (particularly if it wants to abort iteration without setting > an error, just to save CPU cycles), should this foreach() function > return that value? > > Of course, a caller can always use opaque to achieve the same purpose, > but it gets more verbose (the caller has to reserve space in their > opaque for tracking whether an interesting exit value is needed, as well > as having an early check on whether the value was already set in a > previous visit) and burns more CPU cycles (the iterator runs to > completion, even though the later callbacks are doing nothing). > >> +++ b/qom/object.c >> @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name, >> return NULL; >> } >> >> +void object_property_foreach(Object *obj, >> + ObjectPropertyIterator iter, >> + Error **errp, >> + void *opaque) >> +{ >> + ObjectProperty *prop; >> + Error *local_err = NULL; >> + >> + QTAILQ_FOREACH(prop, &obj->properties, node) { >> + iter(obj, prop, &local_err, opaque); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; > > [2] there's the early exit if an error is set. > > The code looks fine, but I think we need a documentation improvement for > issue [2], and we may want a design change for issue [1] if a non-void > return would be useful to any client later in the series. We have quite a few _foreach-functions to help iterate over various things. They are easy enough to write, but I find them awkward to use. Have a look at this straightforward loop from monitor.c: for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) { name = bdrv_get_device_name(bs); if (str[0] == '\0' || !strncmp(name, str, strlen(str))) { readline_add_completion(mon->rs, name); } } Note that it encapsulates iteration details just fine. The loop body accesses local variables the obvious way. Breaking the loop would also be done the obvious way. Before commit fea68bb, it looked like this: mbs.mon = mon; mbs.input = str; readline_set_completion_index(mon->rs, strlen(str)); bdrv_iterate(block_completion_it, &mbs); To actually figure out what this does, you have to look up * The definition of block_completion_it(), 500 lines up: static void block_completion_it(void *opaque, BlockDriverState *bs) { const char *name = bdrv_get_device_name(bs); MonitorBlockComplete *mbc = opaque; Monitor *mon = mbc->mon; const char *input = mbc->input; if (input[0] == '\0' || !strncmp(name, (char *)input, strlen(input))) { readline_add_completion(mon->rs, name); } } * The definition of mbs, 50 lines up. It's a MonitorBlockComplete, so you get too look that up, too. * Thankfully, it's defined right next to block_completion_it(): typedef struct MonitorBlockComplete { Monitor *mon; const char *input; } MonitorBlockComplete; Twice the code, spread over two distant places (not counting definition of mbs), type punning, and things would get even more complicated if you needed to break the loop. Implementing bdrv_next() is no harder than bdrv_iterate(). Compare: BlockDriverState *bdrv_next(BlockDriverState *bs) { if (!bs) { return QTAILQ_FIRST(&bdrv_states); } return QTAILQ_NEXT(bs, device_list); } void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque) { BlockDriverState *bs; QTAILQ_FOREACH(bs, &bdrv_states, device_list) { it(opaque, bs); } } Higher-order functions are a wonderful tool if the language is equipped for them. In Lisp, for instance, you'd have everything in one place and no need for the awkward marshalling and unmarshalling of arguments, thanks to nested functions. In C, stick to loops. That's what the language supports.
On Fri, Oct 09, 2015 at 10:31:26AM +0200, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 10/08/2015 08:09 AM, Daniel P. Berrange wrote: > >> 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. > >> > >> Providing an explicit object_property_foreach method provides > >> a layer of insulation between the QOM user and the QOM internal > >> implementation. > >> > >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > >> --- > >> include/qom/object.h | 23 +++++++++++++++++++++++ > >> qom/object.c | 17 +++++++++++++++++ > >> 2 files changed, 40 insertions(+) > >> > >> diff --git a/include/qom/object.h b/include/qom/object.h > >> index be7280c..71503af 100644 > >> --- a/include/qom/object.h > >> +++ b/include/qom/object.h > >> @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp); > >> ObjectProperty *object_property_find(Object *obj, const char *name, > >> Error **errp); > >> > >> +typedef void (*ObjectPropertyIterator)(Object *obj, > >> + ObjectProperty *prop, > >> + Error **errp, > >> + void *opaque); > > > > Do we want the iterator to be able to return a value, and possibly allow > > a non-zero value to abort iteration? [1] > > > >> + > >> +/** > >> + * object_property_foreach: > >> + * @obj: the object > >> + * @iter: the iterator callback function > >> + * @errp: returns an error if iterator function fails > >> + * @opaque: opaque data to pass to @iter > >> + * > >> + * Iterates over all properties defined against the object > >> + * instance calling @iter for each property. > > > > Probably should mention that there is an early exit if error gets set [2] > > > >> + * > >> + * It is forbidden to modify the property list from @iter > >> + * whether removing or adding properties. > >> + */ > >> +void object_property_foreach(Object *obj, > >> + ObjectPropertyIterator iter, > >> + Error **errp, > >> + void *opaque); > > > > [1] if we allow the iterator to return a non-zero value to abort > > iteration (particularly if it wants to abort iteration without setting > > an error, just to save CPU cycles), should this foreach() function > > return that value? > > > > Of course, a caller can always use opaque to achieve the same purpose, > > but it gets more verbose (the caller has to reserve space in their > > opaque for tracking whether an interesting exit value is needed, as well > > as having an early check on whether the value was already set in a > > previous visit) and burns more CPU cycles (the iterator runs to > > completion, even though the later callbacks are doing nothing). > > > >> +++ b/qom/object.c > >> @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name, > >> return NULL; > >> } > >> > >> +void object_property_foreach(Object *obj, > >> + ObjectPropertyIterator iter, > >> + Error **errp, > >> + void *opaque) > >> +{ > >> + ObjectProperty *prop; > >> + Error *local_err = NULL; > >> + > >> + QTAILQ_FOREACH(prop, &obj->properties, node) { > >> + iter(obj, prop, &local_err, opaque); > >> + if (local_err) { > >> + error_propagate(errp, local_err); > >> + return; > > > > [2] there's the early exit if an error is set. > > > > The code looks fine, but I think we need a documentation improvement for > > issue [2], and we may want a design change for issue [1] if a non-void > > return would be useful to any client later in the series. > > We have quite a few _foreach-functions to help iterate over various > things. They are easy enough to write, but I find them awkward to use. > > Implementing bdrv_next() is no harder than bdrv_iterate(). Compare: > > BlockDriverState *bdrv_next(BlockDriverState *bs) > { > if (!bs) { > return QTAILQ_FIRST(&bdrv_states); > } > return QTAILQ_NEXT(bs, device_list); > } > > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque) > { > BlockDriverState *bs; > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > it(opaque, bs); > } > } I don't think your example here is a reasonable comparison when you consider the full extent of this patch series. You are only having to iterate over a single data structure here. At the end of this patch series we have to iterate over multiple data structures spread across the object instance and class hierarchy, so writing a 'next' like method is not as trivial as you suggest with this comparison. > Higher-order functions are a wonderful tool if the language is equipped > for them. In Lisp, for instance, you'd have everything in one place and > no need for the awkward marshalling and unmarshalling of arguments, > thanks to nested functions. > > In C, stick to loops. That's what the language supports. You're really arguing against use of function callbacks in general with this comparison to LISP. I don't think that's really practical in the real world, as any integration with event loop needs callbacks, which share al the same limitations as callbacks used with this foreach() style iterator. Given this I don't think banning use of callbacks in one specific scenario is really beneficial in general - its really just a personal style choice. Regards, Daniel
On 12/10/2015 12:00, Daniel P. Berrange wrote: > I don't think your example here is a reasonable comparison when you consider > the full extent of this patch series. You are only having to iterate over a > single data structure here. At the end of this patch series we have to > iterate over multiple data structures spread across the object instance > and class hierarchy, so writing a 'next' like method is not as trivial > as you suggest with this comparison. > > > Higher-order functions are a wonderful tool if the language is equipped > > for them. In Lisp, for instance, you'd have everything in one place and > > no need for the awkward marshalling and unmarshalling of arguments, > > thanks to nested functions. > > > > In C, stick to loops. That's what the language supports. > > You're really arguing against use of function callbacks in general with > this comparison to LISP. I don't think that's really practical in the > real world, as any integration with event loop needs callbacks, which > share al the same limitations as callbacks used with this foreach() > style iterator. Given this I don't think banning use of callbacks in > one specific scenario is really beneficial in general - its really > just a personal style choice. I agree with Eric that in general loops are better than callbacks. However, there are cases where iterators are just as awkward to write. Considering that we have very few iterations on properties, I think your patches are fine. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 12/10/2015 12:00, Daniel P. Berrange wrote: >> I don't think your example here is a reasonable comparison when you consider >> the full extent of this patch series. You are only having to iterate over a >> single data structure here. At the end of this patch series we have to >> iterate over multiple data structures spread across the object instance >> and class hierarchy, so writing a 'next' like method is not as trivial >> as you suggest with this comparison. >> >> > Higher-order functions are a wonderful tool if the language is equipped >> > for them. In Lisp, for instance, you'd have everything in one place and >> > no need for the awkward marshalling and unmarshalling of arguments, >> > thanks to nested functions. >> > >> > In C, stick to loops. That's what the language supports. >> >> You're really arguing against use of function callbacks in general with >> this comparison to LISP. I don't think that's really practical in the >> real world, as any integration with event loop needs callbacks, which >> share al the same limitations as callbacks used with this foreach() >> style iterator. Given this I don't think banning use of callbacks in >> one specific scenario is really beneficial in general - its really >> just a personal style choice. You're right, I do think callbacks in C are relatively awkward. But then you're attacking a strawman --- I did not advocate *banning* callbacks. That would be silly indeed. I did advocate for using them *sparingly*. There are cases where callbacks are simply required, and you quoted one. There are cases where callbacks compete with other techniques, and I quoted one. > I agree with Eric that in general loops are better than callbacks. Me. Eric's innocent :) > However, there are cases where iterators are just as awkward to write. > Considering that we have very few iterations on properties, I think your > patches are fine. Mind, I didn't actually object to them. I merely voiced my reasoned opinion on idiomatic iteration in C: stick to loops. Just to deter strawmen: that's a rule of thumb, not a law :)
On Mon, Oct 12, 2015 at 11:00:04AM +0100, Daniel P. Berrange wrote: > On Fri, Oct 09, 2015 at 10:31:26AM +0200, Markus Armbruster wrote: > > We have quite a few _foreach-functions to help iterate over various > > things. They are easy enough to write, but I find them awkward to use. > > > > Implementing bdrv_next() is no harder than bdrv_iterate(). Compare: > > > > BlockDriverState *bdrv_next(BlockDriverState *bs) > > { > > if (!bs) { > > return QTAILQ_FIRST(&bdrv_states); > > } > > return QTAILQ_NEXT(bs, device_list); > > } > > > > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque) > > { > > BlockDriverState *bs; > > > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > > it(opaque, bs); > > } > > } > > I don't think your example here is a reasonable comparison when you consider > the full extent of this patch series. You are only having to iterate over a > single data structure here. At the end of this patch series we have to > iterate over multiple data structures spread across the object instance > and class hierarchy, so writing a 'next' like method is not as trivial > as you suggest with this comparison. So it turns out I was wrong here. After a little more thinking I found it was in fact fairly trivial to support a "next" like iterator in this QOM property scenario, even when taking class properties into account. So I'll re-spin this patch series with that approach, since it makes the diffs much smaller Regards, Daniel
diff --git a/include/qom/object.h b/include/qom/object.h index be7280c..71503af 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -960,6 +960,29 @@ void object_property_del(Object *obj, const char *name, Error **errp); ObjectProperty *object_property_find(Object *obj, const char *name, Error **errp); +typedef void (*ObjectPropertyIterator)(Object *obj, + ObjectProperty *prop, + Error **errp, + void *opaque); + +/** + * object_property_foreach: + * @obj: the object + * @iter: the iterator callback function + * @errp: returns an error if iterator function fails + * @opaque: opaque data to pass to @iter + * + * Iterates over all properties defined against the object + * instance calling @iter for each property. + * + * It is forbidden to modify the property list from @iter + * whether removing or adding properties. + */ +void object_property_foreach(Object *obj, + ObjectPropertyIterator iter, + Error **errp, + void *opaque); + void object_unparent(Object *obj); /** diff --git a/qom/object.c b/qom/object.c index 4805328..f8b27af 100644 --- a/qom/object.c +++ b/qom/object.c @@ -917,6 +917,23 @@ ObjectProperty *object_property_find(Object *obj, const char *name, return NULL; } +void object_property_foreach(Object *obj, + ObjectPropertyIterator iter, + Error **errp, + void *opaque) +{ + ObjectProperty *prop; + Error *local_err = NULL; + + QTAILQ_FOREACH(prop, &obj->properties, node) { + iter(obj, prop, &local_err, opaque); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } +} + void object_property_del(Object *obj, const char *name, Error **errp) { ObjectProperty *prop = object_property_find(obj, name, errp);
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. Providing an explicit object_property_foreach method provides a layer of insulation between the QOM user and the QOM internal implementation. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/qom/object.h | 23 +++++++++++++++++++++++ qom/object.c | 17 +++++++++++++++++ 2 files changed, 40 insertions(+)