| Message ID | 1531409463-3843-2-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix crashes with introspection | expand |
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*?
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
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?
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
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".
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
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
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
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
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
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);
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(+)