Re: [Qemu-arm] [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Thomas Huth]

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 <address@hidden>
> 
> 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 <address@hidden>

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