Re: [Qemu-devel] [PATCH 1/3] block: detach devices from DriveInfo at unrealize time

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/03/2016 18:19, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 21/03/2016 16:13, Markus Armbruster wrote:
>>>> Before your patch, we leave finalization of the property to its
>>>> release() callback release_drive(), as we should.  All we do here is
>>>> schedule warty deletion.  And that we must do here, because only here we
>>>> know that warty deletion is wanted.
>>>> 
>>>> Your patch inserts a copy of release_drive() and hacks it up a bit.  Two
>>>> hunks down, release_drive() gets hacked up to conditionally avoid
>>>> repeating the job.
>>>> 
>>>> This feels rather dirty to me.
>>>
>>> The other possibility is to make blk_detach_dev do nothing if blk->dev
>>> == NULL, i.e. make it idempotent.  On one hand, who doesn't like
>>> idempotency; on the other hand, removing an assertion is also dirty.
>>>
>>> I chose the easy way here (changing as fewer contracts as possible).
>>
>> Why can't we keep the work in the property release() method
>> release_drive()?
>> 
>> The only reason blockdev_mark_auto_del() isn't there is that the device

s/isn't there/exists/ (oops)

>> decides whether to call it, not the property.
>
> DEVICE_DELETED is currently sent right after setting unrealized to false
> (see device_unparent), and you cannnot send it later than that.  In
> particular release_drive would mean sending the drive when properties
> are removed in instance_finalize; by that time you don't have anymore a
> QOM path to include in the event.

I see.  To delay DEVICE_DELETED, we'd have to save the QOM path, and
that would be bothersome.

Still, copying code from property to devices with that property is
undesirable.  It's not that bad in this patch, because we copy only to
the devices that do warty backend deletion, and that's just the two
places where we call blockdev_mark_auto_del() now.  However, it gets
worse if we decide to extend warty backend deletion to *all* devices:
more places, and a new need to consistently copy it to every new user of
the drive property.

When you find yourself copying code from a property callback into every
device using it, the real problem might be you're missing a callback.
In this case, one that runs at unrealize time.  The existing release()
runs at finalize time.