On 07/14/2018 12:57 AM, Eduardo Habkost wrote:
> On Fri, Jul 13, 2018 at 10:27:29AM +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().
>>
>> And while we're at object.h, also fix some copy-n-paste errors in the
>> comments there ("to store the area" --> "to store the error").
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>
> Potential candidates for using the new function, found using the
> following Coccinelle patch:
>
> @@
> expression child, size, type, parent, errp, propname;
> @@
> -object_initialize(child, size, type);
> -object_property_add_child(
> +object_initialize_child(
> parent, propname,
> - OBJECT(child),
> + child, size, type,
> errp);
>
> Some of them (very few) already call object_unref() and need to
> be fixed manually.
>
> Most of the remaining ~50 object_initialize() callers are also
> candidates, even if they don't call object_property_add_child()
> today.
>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Care to turn this into a proper patch, now that we left the freeze period?
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d4e4d98b59..acf7b4e40e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2232,8 +2232,8 @@ void virtio_instance_init_common(Object *proxy_obj,
> void *data,
> {
> DeviceState *vdev = data;
>
> - object_initialize(vdev, vdev_size, vdev_name);
> - object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
> NULL);
> + object_initialize_child(proxy_obj, "virtio-backend", vdev, vdev_size,
> + vdev_name, NULL);
> object_unref(OBJECT(vdev));
> qdev_alias_all_properties(vdev, proxy_obj);
> }
> diff --git a/qom/object.c b/qom/object.c
> index 7be7638db1..91ff795b38 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -396,8 +396,7 @@ void object_initialize_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);
> + object_initialize_child(parentobj, propname, childobj, size, type,
> errp);
> /*
> * Since object_property_add_child added a reference to the child object,
> * we can drop the reference added by object_initialize(), so the child
At least these two hunks need manual adjustment.
Thomas
