Re: [Qemu-devel] [RFC 1/3] qom: Document reference count ownership rules

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Jul 13, 2018 at 11:07:15AM +0200, Thomas Huth wrote:
> On 12.07.2018 21:45, Eduardo Habkost wrote:
> > The documentation for QOM is not clear about who owns references
> > to objects (i.e. who is responsible for calling object_unref()
> > later).
> > 
> > This is important considering there are a few inconsistencies in
> > the API (e.g. callers of object_new() need to call object_unref()
> > later, but callers of object_new_with_props() must not do it).
> > 
> > Update the documentation so that every mention of object
> > references also mention who exactly owns the reference.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  include/qom/object.h | 73 +++++++++++++++++++++++++-------------------
> >  1 file changed, 42 insertions(+), 31 deletions(-)
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index f3d2308d56..08a1bbba7d 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -376,7 +376,7 @@ typedef void (ObjectUnparent)(Object *obj);
> >   * ObjectFree:
> >   * @obj: the object being freed
> >   *
> > - * Called when an object's last reference is removed.
> > + * Called when an object's last reference is dropped using object_unref().
> >   */
> >  typedef void (ObjectFree)(void *obj);
> >  
> > @@ -601,8 +601,8 @@ struct InterfaceClass
> >   * @typename: The name of the type of the object to instantiate.
> >   *
> >   * This function will initialize a new object using heap allocated memory.
> > - * The returned object has a reference count of 1, and will be freed when
> > - * the last reference is dropped.
> > + * The returned object has a reference count of 1, and the reference will be
> > + * owned by the caller.
> >   *
> >   * Returns: The newly allocated and instantiated object.
> >   */
> > @@ -617,8 +617,8 @@ Object *object_new(const char *typename);
> >   * @...: list of property names and values
> >   *
> >   * This function will initialize a new object using heap allocated memory.
> > - * The returned object has a reference count of 1, and will be freed when
> > - * the last reference is dropped.
> > + * The returned object has a reference count of 1, and the reference will
> > + * be owned by the caller.
> 
> That's the description of object_new_with_props here already, isn't it?
> So the reference will be owned by the parent object, not by the caller.

Oops, you're right.  Thanks!

> 
> >   * The @id parameter will be used when registering the object as a
> >   * child of @parent in the composition tree.
> > @@ -652,8 +652,8 @@ Object *object_new(const char *typename);
> >   *   </programlisting>
> >   * </example>
> >   *
> > - * The returned object will have one stable reference maintained
> > - * for as long as it is present in the object hierarchy.
> > + * The returned object will have one reference, <emphasis>owned by the
> > + * parent object</emphasis> (not by the caller).
> 
> ... and then this information here is somewhat redundant. I suggest to
> remove one of the two spots.

Will change this to:

 /**
  * object_new_with_props:
  * @typename:  The name of the type of the object to instantiate.
  * @parent: the parent object
  * @id: The unique ID of the object
  * @errp: pointer to error object
  * @...: list of property names and values
  *
  * This function will initialize a new object using heap allocated memory.
- * The returned object has a reference count of 1, and will be freed when
- * the last reference is dropped.
+ * The returned object will have one reference, <emphasis>owned by the
+ * parent object</emphasis> (not by the caller).
  *
  * The @id parameter will be used when registering the object as a
  * child of @parent in the composition tree.
  *
  * The variadic parameters are a list of pairs of (propname, propvalue)
  * strings. The propname of %NULL indicates the end of the property
  * list. If the object implements the user creatable interface, the
  * object will be marked complete once all the properties have been
  * processed.
  *
  * <example>
  *   <title>Creating an object with properties</title>
  *   <programlisting>
  *   Error *err = NULL;
  *   Object *obj;
  *
  *   obj = object_new_with_props(TYPE_MEMORY_BACKEND_FILE,
  *                               object_get_objects_root(),
  *                               "hostmem0",
  *                               &err,
  *                               "share", "yes",
  *                               "mem-path", "/dev/shm/somefile",
  *                               "prealloc", "yes",
  *                               "size", "1048576",
  *                               NULL);
  *
  *   if (!obj) {
  *     g_printerr("Cannot create memory backend: %s\n",
  *                error_get_pretty(err));
  *   }
  *   </programlisting>
  * </example>
  *
- * The returned object will have one stable reference maintained
- * for as long as it is present in the object hierarchy.
- *
  * Returns: The newly allocated, instantiated & initialized object.
  */
 Object *object_new_with_props(const char *typename,
                               Object *parent,
                               const char *id,
                               Error **errp,
                               ...) QEMU_SENTINEL;

-- 
Eduardo