From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RT87C-0006iA-6R for qemu-devel@nongnu.org; Wed, 23 Nov 2011 03:23:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RT87A-0002MO-Ub for qemu-devel@nongnu.org; Wed, 23 Nov 2011 03:23:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60052) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RT87A-0002MK-Nf for qemu-devel@nongnu.org; Wed, 23 Nov 2011 03:23:32 -0500 Message-ID: <4ECCAE40.1040404@redhat.com> Date: Wed, 23 Nov 2011 09:26:40 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1321978604-4858-1-git-send-email-kwolf@redhat.com> <4ECBEBE8.1000709@weilnetz.de> In-Reply-To: <4ECBEBE8.1000709@weilnetz.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vdi: Fix memory leak List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: qemu-devel@nongnu.org Am 22.11.2011 19:37, schrieb Stefan Weil: > Am 22.11.2011 17:16, schrieb Kevin Wolf: >> The block map is allocated in vdi_open, but was never freed. >> >> Signed-off-by: Kevin Wolf >> --- >> Applies on top if the migration blocker series. >> >> block/vdi.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/block/vdi.c b/block/vdi.c >> index 7dda522..02da6b4 100644 >> --- a/block/vdi.c >> +++ b/block/vdi.c >> @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) >> static void vdi_close(BlockDriverState *bs) >> { >> BDRVVdiState *s = bs->opaque; >> + >> + g_free(s->bmap); >> + >> migrate_del_blocker(s->migration_blocker); >> error_free(s->migration_blocker); >> } > > If vdi_close is called after a jump to label fail_free_bmap, > g_free(s->bmap) will be called twice. bdrv_close() may only ever be called on a successfully opened image. In fact, block.c already ensures this. The BDRVVdiState would already be freed in the failing bdrv_open_common() and bs->drv would be set to NULL, so that an invalid bdrv_close() would end up doing nothing. > Setting s->bmap = NULL after g_free in fail_free_bmap > would be safer. > > Otherwise your patch is fine. If you send an update, you > can add > > Reviewed-by: Stefan Weil Thanks. Kevin