Re: [Qemu-devel] [PATCH v2 02/23] block: New BlockBackend

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Benoît Canet <address@hidden> writes:
>>
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -2119,10 +2119,11 @@ static void bdrv_delete(BlockDriverState *bs)
>>>>  
>>>>      bdrv_close(bs);
>>>>  
>>>
>>>
>>>> +    drive_info_del(drive_get_by_blockdev(bs));
>>>> +
>>>>      /* remove from list, if necessary */
>>>>      bdrv_make_anon(bs);
>>>>  
>>>> -    drive_info_del(drive_get_by_blockdev(bs));
>>>
>>> Do we really want this move ?
>>
>> Yes, we do.  If bdrv_make_anon() runs before drive_info_del(), this
>> conditional in drive_info_del() is always false:
>>
>>     if (dinfo->bdrv->device_name[0]) {
>>         blk_unref(blk_by_name(dinfo->bdrv->device_name));
>>     }
>>
>> I apologize for the hairiness.  Things will become *way* simpler in
>> PATCH 4.
>
> It's not just temporarily hairy, it's temporarily wrong: double unref is
> possible.

Clarification: wrong in my tree, after I tried to plug the leak Max
found in PATCH 3.  v2 as posted has no double unref.

I'm afraid getting this exactly right at every step is too hard to be
worth it.  I think I'll go for a temporary memory leak in v3.

> drive_del, the gift that keeps on giving...
>
> [...]