Re: [Qemu-devel] [PATCH] qdev: fix device_del by refactoring reference counting

[Anthony Liguori]

On 01/13/2012 07:45 AM, Anthony Liguori wrote:
> Commit 8eb0283 broken device_del by having too overzealous reference counting
> checks.  Move the reference count checks to qdev_free(), make sure to remove
> the parent link on free, and decrement the reference count on property removal.
>
> Reported-by: Markus Armbruster<address@hidden>
> Signed-off-by: Anthony Liguori<address@hidden>

Applied.

Regards,

Anthony Liguori

> ---
>   hw/qdev.c |   45 +++++++++++++++++++++++++++++++++++++++------
>   1 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d0cf66d..e59f345 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -376,11 +376,6 @@ int qdev_unplug(DeviceState *dev)
>       }
>       assert(dev->info->unplug != NULL);
>
> -    if (dev->ref != 0) {
> -        qerror_report(QERR_DEVICE_IN_USE, dev->id?:"");
> -        return -1;
> -    }
> -
>       qdev_hot_removed = true;
>
>       return dev->info->unplug(dev);
> @@ -465,6 +460,29 @@ static void qdev_property_del_all(DeviceState *dev)
>       }
>   }
>
> +static void qdev_property_del_child(DeviceState *dev, DeviceState *child, 
> Error **errp)
> +{
> +    DeviceProperty *prop;
> +
> +    QTAILQ_FOREACH(prop,&dev->properties, node) {
> +        if (strstart(prop->type, "child<", NULL)&&  prop->opaque == child) {
> +            break;
> +        }
> +    }
> +
> +    g_assert(prop != NULL);
> +
> +    QTAILQ_REMOVE(&dev->properties, prop, node);
> +
> +    if (prop->release) {
> +        prop->release(dev, prop->name, prop->opaque);
> +    }
> +
> +    g_free(prop->name);
> +    g_free(prop->type);
> +    g_free(prop);
> +}
> +
>   /* Unlink device from bus and free the structure.  */
>   void qdev_free(DeviceState *dev)
>   {
> @@ -491,6 +509,12 @@ void qdev_free(DeviceState *dev)
>               prop->info->free(dev, prop);
>           }
>       }
> +    if (dev->parent) {
> +        qdev_property_del_child(dev->parent, dev, NULL);
> +    }
> +    if (dev->ref != 0) {
> +        qerror_report(QERR_DEVICE_IN_USE, dev->id?:"");
> +    }
>       g_free(dev);
>   }
>
> @@ -1239,6 +1263,14 @@ static void qdev_get_child_property(DeviceState *dev, 
> Visitor *v, void *opaque,
>       g_free(path);
>   }
>
> +static void qdev_release_child_property(DeviceState *dev, const char *name,
> +                                        void *opaque)
> +{
> +    DeviceState *child = opaque;
> +
> +    qdev_unref(child);
> +}
> +
>   void qdev_property_add_child(DeviceState *dev, const char *name,
>                                DeviceState *child, Error **errp)
>   {
> @@ -1247,7 +1279,8 @@ void qdev_property_add_child(DeviceState *dev, const char 
> *name,
>       type = g_strdup_printf("child<%s>", child->info->name);
>
>       qdev_property_add(dev, name, type, qdev_get_child_property,
> -                      NULL, NULL, child, errp);
> +                      NULL, qdev_release_child_property,
> +                      child, errp);
>
>       qdev_ref(child);
>       g_assert(child->parent == NULL);