Re: [Qemu-devel] [PATCH] qom: change object property iterator API contract

[Daniel P. Berrange]

On Tue, Dec 08, 2015 at 09:05:52AM -0700, Eric Blake wrote:
> On 11/27/2015 08:27 AM, Daniel P. Berrange wrote:
> > Currently the object property iterator API works as follows
> > 
> >   ObjectPropertyIterator *iter;
> > 
> >   iter = object_property_iter_init(obj);
> >   while ((prop = object_property_iter_next(iter))) {
> >      ...
> >   }
> >   object_property_iter_free(iter);
> > 
> > This has the benefit that the ObjectPropertyIterator struct
> > can be opaque, but has the downside that callers need to
> > explicitly call a free function. It is also not in keeping
> > with iterator style used elsewhere in QEMU/glib2
> > 
> > This patch changes the API to use stack allocation instead
> > 
> >   ObjectPropertyIterator iter;
> > 
> >   object_property_iter_init(&iter, obj);
> >   while ((prop = object_property_iter_next(&iter))) {
> >      ...
> >   }
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > 
> > NB, this patch is not against master, it is intended to apply
> > after
> > 
> >   "qom: allow properties to be registered against classes"
> > 
> > which is queued in qom-next for 2.6
> > 
> >  hw/ppc/spapr_drc.c         |  7 +++----
> >  include/qom/object.h       | 42 +++++++++++++++++++++++++++---------------
> >  net/filter.c               |  7 +++----
> >  qmp.c                      | 14 ++++++--------
> >  qom/object.c               | 22 ++++------------------
> >  tests/check-qom-proplist.c |  7 +++----
> >  vl.c                       |  7 +++----
> >  7 files changed, 49 insertions(+), 57 deletions(-)
> > 
> 
> > +++ b/include/qom/object.h
> > @@ -346,6 +346,7 @@ typedef struct ObjectProperty
> >      void *opaque;
> >  } ObjectProperty;
> >  
> > +
> >  /**
> >   * ObjectUnparent:
> >   * @obj: the object that is being removed from the composition tree
> 
> Spurious whitespace change?
> 
> 
> >  
> > + /**
> > + * object_property_iter_free:
> > + * @iter: the iterator instance
> > + *
> > + * Releases any resources associated with the iterator. It is
> > + * not necessary to call this method if object_property_iter_next
> > + * has returned %NULL. It is only required if an application wishes
> > + * to abort iteration before it is complete
> > + */
> > +void object_property_iter_free(ObjectPropertyIterator *iter);
> > +
> 
> Huh? Why is this being added?  I thought the point was to get rid of the
> need for object_property_iter_free().

Yeh, forgot to remove the header declaration.

> 
> > +++ b/qom/object.c
> > @@ -67,11 +67,6 @@ struct TypeImpl
> 
> Other than that snafu, everything else looked fine.   If that's all you
> fix for v2, you can add:
> 
> Reviewed-by: Eric Blake <address@hidden>

Ok, thanks for the review


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|