Re: [Qemu-devel] [PATCH 05/23] block: Make BlockBackend own its BlockDriverState

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
>> On BlockBackend destruction, unref its BlockDriverState.  Replaces the
>> callers' unrefs.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block/block-backend.c |  9 ++-------
>>  blockdev.c            | 11 +++--------
>>  hw/block/xen_disk.c   |  6 +++---
>>  qemu-img.c            | 35 +----------------------------------
>>  qemu-io.c             |  5 -----
>>  5 files changed, 9 insertions(+), 57 deletions(-)
>> 
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 2a22660..ae51f7f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -58,10 +58,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
>>   * @errp: return location for an error to be set on failure, or %NULL
>>   *
>>   * Create a new BlockBackend, with a reference count of one, and
>> - * attach a new BlockDriverState to it, also with a reference count of
>> - * one.  Caller owns *both* references.
>> - * TODO Let caller own only the BlockBackend reference
>> - * Fail if @name already exists.
>> + * a new BlockDriverState attached.  Fail if @name already exists.
>>   *
>>   * Returns: the BlockBackend on success, %NULL on error
>>   */
>> @@ -88,6 +85,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error 
>> **errp)
>>  static void blk_delete(BlockBackend *blk)
>>  {
>>      assert(!blk->refcnt);
>> +    bdrv_unref(blk->bs);
>>      blk_detach_bs(blk);
>
> I think the bdrv_unref() should really be part of blk_detach_bs().
>
> The same way it would be more logical to have bdrv_ref() as part of
> blk_attach_bs(). For blk_new_with_bs() this might mean bdrv_new,
> blk_attach_bs, bdrv_unref, which looks a bit odd, but if blk_attach_bs()
> is ever called from somewhere else, it probably makes more sense (if it
> isn't, it should be static).

My thinking was that you usually want to acquire a reference when you
detach, and you're usually ready to give yours up when you attach.

However, I now think I got a use-after-free hidden around there.  I'll
look into it tomorrow with a fresh mind.  Might lead to me accepting
your suggestion without further ado :)