[1/5] qom/object: Add a new function object_initialize_as_child()

Message ID 1531409463-3843-2-git-send-email-thuth@redhat.com
State New
Headers show
Series
  • Fix crashes with introspection
Related show

Commit Message

Thomas Huth July 12, 2018, 3:30 p.m.
A lot of code is using the object_initialize() function followed by a call to
object_property_add_child() to add the newly initialized object as a child of
the current object. Both functions increase the reference counter of the new
object, but many spots that call these two functions then forget to drop one
of the superfluous references. So the newly created object is often not cleaned
up correctly when the parent is destroyed. In the worst case, this can cause
crashes, e.g. because device objects are not correctly removed from their
parent_bus.
Since this is a common pattern between many code spots, let's introdcue a
new function that takes care of calling all three required initialization
functions, first object_initialize(), then object_property_add_child() and
finally object_unref().

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qom/object.h | 19 +++++++++++++++++++
 qom/object.c         | 14 ++++++++++++++
 2 files changed, 33 insertions(+)

Comments

Eduardo Habkost July 12, 2018, 4:52 p.m. | #1
On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call to
> object_property_add_child() to add the newly initialized object as a child of
> the current object. Both functions increase the reference counter of the new
> object, but many spots that call these two functions then forget to drop one
> of the superfluous references. So the newly created object is often not cleaned
> up correctly when the parent is destroyed. In the worst case, this can cause
> crashes, e.g. because device objects are not correctly removed from their
> parent_bus.
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 19 +++++++++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..c1b254c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_as_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the area
> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1, and will be finalized when the last reference is
> + * dropped.
> + */
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp);

Why did you use void* instead of Object*?
Thomas Huth July 12, 2018, 4:55 p.m. | #2
On 12.07.2018 18:52, Eduardo Habkost wrote:
> On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
>> A lot of code is using the object_initialize() function followed by a call to
>> object_property_add_child() to add the newly initialized object as a child of
>> the current object. Both functions increase the reference counter of the new
>> object, but many spots that call these two functions then forget to drop one
>> of the superfluous references. So the newly created object is often not cleaned
>> up correctly when the parent is destroyed. In the worst case, this can cause
>> crashes, e.g. because device objects are not correctly removed from their
>> parent_bus.
>> Since this is a common pattern between many code spots, let's introdcue a
>> new function that takes care of calling all three required initialization
>> functions, first object_initialize(), then object_property_add_child() and
>> finally object_unref().
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  include/qom/object.h | 19 +++++++++++++++++++
>>  qom/object.c         | 14 ++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index f3d2308..c1b254c 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>>  void object_initialize(void *obj, size_t size, const char *typename);
>>  
>>  /**
>> + * object_initialize_as_child:
>> + * @parentobj: The parent object to add a property to
>> + * @propname: The name of the property
>> + * @childobj: A pointer to the memory to be used for the object.
>> + * @size: The maximum size available at @obj for the object.
>> + * @type: The name of the type of the object to instantiate.
>> + * @errp: If an error occurs, a pointer to an area to store the area
>> + *
>> + * This function will initialize an object. The memory for the object should
>> + * have already been allocated. The object will then be added as child property
>> + * to a parent with object_property_add_child() function. The returned object
>> + * has a reference count of 1, and will be finalized when the last reference is
>> + * dropped.
>> + */
>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>> +                                void *childobj, size_t size, const char *type,
>> +                                Error **errp);
> 
> Why did you use void* instead of Object*?

That's the same what object_initialize() is doing (see above). Otherwise
all the callers have to cast their pointers with OBJECT() first.

 Thomas
Eduardo Habkost July 12, 2018, 5:02 p.m. | #3
On Thu, Jul 12, 2018 at 06:55:20PM +0200, Thomas Huth wrote:
> On 12.07.2018 18:52, Eduardo Habkost wrote:
> > On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> >> A lot of code is using the object_initialize() function followed by a call to
> >> object_property_add_child() to add the newly initialized object as a child of
> >> the current object. Both functions increase the reference counter of the new
> >> object, but many spots that call these two functions then forget to drop one
> >> of the superfluous references. So the newly created object is often not cleaned
> >> up correctly when the parent is destroyed. In the worst case, this can cause
> >> crashes, e.g. because device objects are not correctly removed from their
> >> parent_bus.
> >> Since this is a common pattern between many code spots, let's introdcue a
> >> new function that takes care of calling all three required initialization
> >> functions, first object_initialize(), then object_property_add_child() and
> >> finally object_unref().
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  include/qom/object.h | 19 +++++++++++++++++++
> >>  qom/object.c         | 14 ++++++++++++++
> >>  2 files changed, 33 insertions(+)
> >>
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index f3d2308..c1b254c 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
> >>  void object_initialize(void *obj, size_t size, const char *typename);
> >>  
> >>  /**
> >> + * object_initialize_as_child:
> >> + * @parentobj: The parent object to add a property to
> >> + * @propname: The name of the property
> >> + * @childobj: A pointer to the memory to be used for the object.
> >> + * @size: The maximum size available at @obj for the object.
> >> + * @type: The name of the type of the object to instantiate.
> >> + * @errp: If an error occurs, a pointer to an area to store the area
> >> + *
> >> + * This function will initialize an object. The memory for the object should
> >> + * have already been allocated. The object will then be added as child property
> >> + * to a parent with object_property_add_child() function. The returned object
> >> + * has a reference count of 1, and will be finalized when the last reference is
> >> + * dropped.
> >> + */
> >> +void object_initialize_as_child(Object *parentobj, const char *propname,
> >> +                                void *childobj, size_t size, const char *type,
> >> +                                Error **errp);
> > 
> > Why did you use void* instead of Object*?
> 
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Why wouldn't the same argument apply to every single function
that takes Object* as argument?  Why the OBJECT macro exists?
Peter Maydell July 12, 2018, 5:09 p.m. | #4
On 12 July 2018 at 17:55, Thomas Huth <thuth@redhat.com> wrote:
> On 12.07.2018 18:52, Eduardo Habkost wrote:
>> On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
>>> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>>>  void object_initialize(void *obj, size_t size, const char *typename);
>>>
>>>  /**
>>> + * object_initialize_as_child:
>>> + * @parentobj: The parent object to add a property to
>>> + * @propname: The name of the property
>>> + * @childobj: A pointer to the memory to be used for the object.
>>> + * @size: The maximum size available at @obj for the object.
>>> + * @type: The name of the type of the object to instantiate.
>>> + * @errp: If an error occurs, a pointer to an area to store the area
>>> + *
>>> + * This function will initialize an object. The memory for the object should
>>> + * have already been allocated. The object will then be added as child property
>>> + * to a parent with object_property_add_child() function. The returned object
>>> + * has a reference count of 1, and will be finalized when the last reference is
>>> + * dropped.
>>> + */
>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>> +                                void *childobj, size_t size, const char *type,
>>> +                                Error **errp);
>>
>> Why did you use void* instead of Object*?
>
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Casting with OBJECT() would not work, because it does a typecheck.
At the point where something is calling object_initialize() or
object_initialize_with_child(), the memory at 'childobj' has not
been initialized, so a typecheck is impossible.

It's only once the object has actually been initialized that
the memory is a valid Object* that you can put through OBJECT().

thanks
-- PMM
Eduardo Habkost July 12, 2018, 5:15 p.m. | #5
On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call to
> object_property_add_child() to add the newly initialized object as a child of
> the current object. Both functions increase the reference counter of the new
> object, but many spots that call these two functions then forget to drop one
> of the superfluous references. So the newly created object is often not cleaned
> up correctly when the parent is destroyed. In the worst case, this can cause
> crashes, e.g. because device objects are not correctly removed from their
> parent_bus.
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 19 +++++++++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..c1b254c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_as_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the area

"to store the error"

> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1, and will be finalized when the last reference is
> + * dropped.

I think we need to document clearly who owns the reference and is
responsible for dropping it.  In this case, we need to make it
clear that the child property will own the reference, and nobody
should drop it except object_finalize_child_property().  This is
a subtle but important difference from object_initialize().


> + */
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp);
> +
> +/**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
>   * @typename: The @typename to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 4609e34..de2ded0 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -392,6 +392,20 @@ void object_initialize(void *data, size_t size, const char *typename)
>      object_initialize_with_type(data, size, type);
>  }
>  
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp)
> +{
> +    object_initialize(childobj, size, type);
> +    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
> +    /*
> +     * Since object_property_add_child added a reference to the child object,
> +     * we can drop the initial reference from object_initialize now.
> +     */
> +    object_unref(OBJECT(childobj));

I suggest "drop the reference added by object_initialize(), the
child property will own the only reference to the object".
Paolo Bonzini July 13, 2018, 6:26 a.m. | #6
On 12/07/2018 18:55, Thomas Huth wrote:
>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>> +                                void *childobj, size_t size, const char *type,
>>> +                                Error **errp);
>> Why did you use void* instead of Object*?
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Actually I had forgotten about object_new_with_props, which is very
similar to the new function above.  I think we should follow the model
and define the new function as

void object_initialize_with_props(Object *parent, const char *id,
                           void *childobj,
                           size_t size, const char *type,
                           Error **errp,
                           ...) QEMU_SENTINEL;
void object_initialize_with_propv(Object *parent, const char *id,
                           void *childobj,
                           size_t size, const char *type,
                           Error **errp,
                           va_list vargs);

I actually prefer Thomas's name (or maybe even
object_initialize_child/object_initialize_childv), but if we adopt it we
should also rename object_new_with_props and object_new_with_propv, for
consistency.

Thanks,

Paolo
Paolo Bonzini July 13, 2018, 6:27 a.m. | #7
On 12/07/2018 19:09, Peter Maydell wrote:
>> That's the same what object_initialize() is doing (see above). Otherwise
>> all the callers have to cast their pointers with OBJECT() first.
> Casting with OBJECT() would not work, because it does a typecheck.
> At the point where something is calling object_initialize() or
> object_initialize_with_child(), the memory at 'childobj' has not
> been initialized, so a typecheck is impossible.
> 
> It's only once the object has actually been initialized that
> the memory is a valid Object* that you can put through OBJECT().

OBJECT is special and just does a cast, because every pointer to a QOM
object would pass.  However, your reasoning still applies: you should
not put through OBJECT() an object that hasn't been initialized yet,
just like you should not put it through DEVICE() or any of the macros
that _do_ perform the typecheck.

Paolo
Thomas Huth July 13, 2018, 7:57 a.m. | #8
On 13.07.2018 08:26, Paolo Bonzini wrote:
> On 12/07/2018 18:55, Thomas Huth wrote:
>>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>>> +                                void *childobj, size_t size, const char *type,
>>>> +                                Error **errp);
>>> Why did you use void* instead of Object*?
>> That's the same what object_initialize() is doing (see above). Otherwise
>> all the callers have to cast their pointers with OBJECT() first.
> 
> Actually I had forgotten about object_new_with_props, which is very
> similar to the new function above.  I think we should follow the model
> and define the new function as
> 
> void object_initialize_with_props(Object *parent, const char *id,
>                            void *childobj,
>                            size_t size, const char *type,
>                            Error **errp,
>                            ...) QEMU_SENTINEL;
> void object_initialize_with_propv(Object *parent, const char *id,
>                            void *childobj,
>                            size_t size, const char *type,
>                            Error **errp,
>                            va_list vargs);
> 
> I actually prefer Thomas's name (or maybe even
> object_initialize_child/object_initialize_childv), but if we adopt it we
> should also rename object_new_with_props and object_new_with_propv, for
> consistency.

Since we are in hard freeze already, I'd rather prefer to keep it
simple, without the variable args for now (we don't need them yet).
Especially since we can easily add them later when we need them,
changing the prototype from

void object_initialize_child(Object *parentobj, const char *propname,
                             void *childobj, size_t size,
                             const char *type, Error **errp);

to

void object_initialize_child(Object *parentobj, const char *propname,
                             void *childobj, size_t size,
                             const char *type, Error **errp, ...);

should not affect any of the existing caller sites later.

And yes, I think these functions should have "child" in their names, so
I'll continue with object_initialize_child now. Renaming
object_new_with_props to object_new_child also sounds like a good idea
to me, but that's something for the time when the hard freeze is over.

 Thomas
Paolo Bonzini July 13, 2018, 10:12 a.m. | #9
On 13/07/2018 09:57, Thomas Huth wrote:
> Since we are in hard freeze already, I'd rather prefer to keep it
> simple, without the variable args for now (we don't need them yet).
> Especially since we can easily add them later when we need them,
> changing the prototype from
> 
> void object_initialize_child(Object *parentobj, const char *propname,
>                              void *childobj, size_t size,
>                              const char *type, Error **errp);
> 
> to
> 
> void object_initialize_child(Object *parentobj, const char *propname,
>                              void *childobj, size_t size,
>                              const char *type, Error **errp, ...);
> 
> should not affect any of the existing caller sites later.
> 
> And yes, I think these functions should have "child" in their names, so
> I'll continue with object_initialize_child now. Renaming
> object_new_with_props to object_new_child also sounds like a good idea
> to me, but that's something for the time when the hard freeze is over.

It's true that we are in hard freeze, but now that I've noticed the
parallel with object_new_with_props, it seems to me that making the
functions subtly different (in naming or functionality) is just being
sloppy.  A search-and-replace patch is acceptable for hard freeze if
there is a good reason for it, and the rest is just copying code from
object_new_with_props into the no-malloc version.  The variable
arguments are handled simply by

    if (object_set_propv(obj, &local_err, vargs) < 0) {
        goto error;
    }

and another thing to do is

    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
        user_creatable_complete(obj, &local_err);
        if (local_err) {
            object_unparent(obj);
            goto error;
        }
    }

which is included in object_new_with_propv.

Thanks,

Paolo
Thomas Huth July 13, 2018, 11:20 a.m. | #10
On 13.07.2018 12:12, Paolo Bonzini wrote:
> On 13/07/2018 09:57, Thomas Huth wrote:
>> Since we are in hard freeze already, I'd rather prefer to keep it
>> simple, without the variable args for now (we don't need them yet).
>> Especially since we can easily add them later when we need them,
>> changing the prototype from
>>
>> void object_initialize_child(Object *parentobj, const char *propname,
>>                              void *childobj, size_t size,
>>                              const char *type, Error **errp);
>>
>> to
>>
>> void object_initialize_child(Object *parentobj, const char *propname,
>>                              void *childobj, size_t size,
>>                              const char *type, Error **errp, ...);
>>
>> should not affect any of the existing caller sites later.
>>
>> And yes, I think these functions should have "child" in their names, so
>> I'll continue with object_initialize_child now. Renaming
>> object_new_with_props to object_new_child also sounds like a good idea
>> to me, but that's something for the time when the hard freeze is over.
> 
> It's true that we are in hard freeze, but now that I've noticed the
> parallel with object_new_with_props, it seems to me that making the
> functions subtly different (in naming or functionality) is just being
> sloppy.  A search-and-replace patch is acceptable for hard freeze if
> there is a good reason for it, and the rest is just copying code from
> object_new_with_props into the no-malloc version.  The variable
> arguments are handled simply by
> 
>     if (object_set_propv(obj, &local_err, vargs) < 0) {
>         goto error;
>     }
> 
> and another thing to do is
> 
>     if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
>         user_creatable_complete(obj, &local_err);
>         if (local_err) {
>             object_unparent(obj);
>             goto error;
>         }
>     }
Maybe. But I doubt that I've got enough spare time to implement and test
this before the next rc. So if anybody wants to see the fixes in the
next rc already, please pick up the v2 series. Otherwise, I'll have a
closer look next week.

 Thomas

Patch

diff --git a/include/qom/object.h b/include/qom/object.h
index f3d2308..c1b254c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -749,6 +749,25 @@  int object_set_propv(Object *obj,
 void object_initialize(void *obj, size_t size, const char *typename);
 
 /**
+ * object_initialize_as_child:
+ * @parentobj: The parent object to add a property to
+ * @propname: The name of the property
+ * @childobj: A pointer to the memory to be used for the object.
+ * @size: The maximum size available at @obj for the object.
+ * @type: The name of the type of the object to instantiate.
+ * @errp: If an error occurs, a pointer to an area to store the area
+ *
+ * This function will initialize an object. The memory for the object should
+ * have already been allocated. The object will then be added as child property
+ * to a parent with object_property_add_child() function. The returned object
+ * has a reference count of 1, and will be finalized when the last reference is
+ * dropped.
+ */
+void object_initialize_as_child(Object *parentobj, const char *propname,
+                                void *childobj, size_t size, const char *type,
+                                Error **errp);
+
+/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
diff --git a/qom/object.c b/qom/object.c
index 4609e34..de2ded0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -392,6 +392,20 @@  void object_initialize(void *data, size_t size, const char *typename)
     object_initialize_with_type(data, size, type);
 }
 
+void object_initialize_as_child(Object *parentobj, const char *propname,
+                                void *childobj, size_t size, const char *type,
+                                Error **errp)
+{
+    object_initialize(childobj, size, type);
+    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
+    /*
+     * Since object_property_add_child added a reference to the child object,
+     * we can drop the initial reference from object_initialize now.
+     */
+    object_unref(OBJECT(childobj));
+}
+
+
 static inline bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);