On 21/03/2016 16:22, Markus Armbruster wrote:
> 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?
There could be asynchronous I/O operations which are still active after
unrealize. The device would not be finalized until they are completed.
> And shouldn't they take their own reference?
Generally the block layer doesn't try to ref/unref on every use. It
assumes that someone else does it for you. A better justification for
this patch is that blk_attach_dev/blk_detach_dev actually does not need
to take a reference, so we can add it to parse_drive/release_drive and
remove it from blk_attach_dev/blk_detach_dev instead.
Paolo
> 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);
>> }
>> }
