Paolo Bonzini <pbonz...@redhat.com> 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 <pbonz...@redhat.com>
> ---
> 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);
> }
> }
