Re: [Qemu-devel] [PATCH 2/3] block: keep BlockBackend alive until device finalize time

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> While the next patch will anticipate the death of the DriveInfo
> data structure, the BlockBackend must survive after unrealize,
> for example in case there are outstanding operations on it.
> The good thing is that we can just use reference counting to
> do it.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/core/qdev-properties-system.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 469ba8a..5e84b55 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, 
> void **ptr,
       if (blk_attach_dev(blk, dev) < 0) {
           DriveInfo *dinfo = blk_legacy_dinfo(blk);

           if (dinfo->type != IF_NONE) {
               error_setg(errp, "Drive '%s' is already in use because "
                          "it has been automatically connected to another "
                          "device (did you need 'if=none' in the drive 
options?)",
                          str);
           } else {
               error_setg(errp, "Drive '%s' is already in use by another 
device",
                          str);
           }
>          return;
>      }
>      *ptr = blk;
> +    blk_ref(blk);

blk_attach_dev() already takes a reference.  I'm not sure I understand
why you need to take a second one.  You say "in case there are
outstanding operations on it."  What operations could that be?  And
shouldn't they take their own reference?

I hasten to add that I'm not going to demand you fix them to take their
own references.  It's okay to take a hacky second reference here, then
fix "them" at our leisure.  But I need to understand what exactly this
second reference protects.  It probably needs to be explained in the
source, too.

>  }
>  
>  static void release_drive(Object *obj, const char *name, void *opaque)
> @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char 
> *name, void *opaque)
>      Property *prop = opaque;
>      BlockBackend **ptr = qdev_get_prop_ptr(dev, prop);
>  
> -    if (*ptr && blk_get_attached_dev(*ptr) != NULL) {
> -        /* Unrealize has already called blk_detach_dev and blockdev_del_drive
> -         * if the device has been realized; in that case, 
> blk_get_attached_dev
> -         * returns NULL.  Thus, we get here if the device failed to realize,
> -         * and the -drive must not be released.
> -         */
> -        blk_detach_dev(*ptr, dev);
> +    if (*ptr) {
> +        if (blk_get_attached_dev(*ptr) != NULL) {
> +            /* Unrealize has already called blk_detach_dev and
> +             * blockdev_del_drive if the device has been realized;
> +             * in that case, blk_get_attached_dev returns NULL.  Thus,
> +             * we get here if the device failed to realize, and the
> +             * -drive must not be released.
> +             */
> +            blk_detach_dev(*ptr, dev);
> +        }
> +        blk_unref(*ptr);
>      }
>  }