From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:37159)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1XTYbl-0003gq-09
	for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:54:34 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1XTYbf-0007kk-SZ
	for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:54:28 -0400
Received: from mx1.redhat.com ([209.132.183.28]:51507)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1XTYbf-0007kc-KM
	for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:54:23 -0400
From: Markus Armbruster <armbru@redhat.com>
References: <1410620427-20089-1-git-send-email-armbru@redhat.com>
	<1410620427-20089-3-git-send-email-armbru@redhat.com>
	<20140915130216.GD22086@nodalink.com>
	<87mwa1c7me.fsf@blackfin.pond.sub.org>
	<87zje17x1z.fsf@blackfin.pond.sub.org>
Date: Mon, 15 Sep 2014 17:54:17 +0200
In-Reply-To: <87zje17x1z.fsf@blackfin.pond.sub.org> (Markus Armbruster's
	message of "Mon, 15 Sep 2014 16:55:36 +0200")
Message-ID: <87oaug6frq.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH v2 02/23] block: New BlockBackend
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?utf-8?Q?Beno=C3=AEt?= Canet <benoit.canet@nodalink.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Beno=C3=AEt Canet <benoit.canet@nodalink.com> writes:
>>
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -2119,10 +2119,11 @@ static void bdrv_delete(BlockDriverState *bs)
>>>>=20=20
>>>>      bdrv_close(bs);
>>>>=20=20
>>>
>>>
>>>> +    drive_info_del(drive_get_by_blockdev(bs));
>>>> +
>>>>      /* remove from list, if necessary */
>>>>      bdrv_make_anon(bs);
>>>>=20=20
>>>> -    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...
>
> [...]