From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsVOs-0007ee-Pj for qemu-devel@nongnu.org; Wed, 13 May 2015 08:04:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsVOr-00043N-IZ for qemu-devel@nongnu.org; Wed, 13 May 2015 08:04:34 -0400 Message-ID: <55533DC5.7040104@redhat.com> Date: Wed, 13 May 2015 14:04:21 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-19-git-send-email-kwolf@redhat.com> <55533B07.70504@redhat.com> <20150513120229.GG4263@noname.str.redhat.com> In-Reply-To: <20150513120229.GG4263@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 18/34] qcow2: Fix memory leak in qcow2_update_options() error path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org On 13.05.2015 14:02, Kevin Wolf wrote: > Am 13.05.2015 um 13:52 hat Max Reitz geschrieben: >> On 08.05.2015 19:21, Kevin Wolf wrote: >>> Signed-off-by: Kevin Wolf >>> --- >>> block/qcow2.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index abe22f3..84d6e0f 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -546,8 +546,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, >>> const char *opt_overlap_check, *opt_overlap_check_template; >>> int overlap_check_template = 0; >>> uint64_t l2_cache_size, refcount_cache_size; >>> - Qcow2Cache* l2_table_cache; >>> - Qcow2Cache* refcount_block_cache; >>> + Qcow2Cache* l2_table_cache = NULL; >>> + Qcow2Cache* refcount_block_cache = NULL; >>> bool use_lazy_refcounts; >>> int i; >>> Error *local_err = NULL; >>> @@ -670,6 +670,14 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, >>> ret = 0; >>> fail: >>> + if (ret < 0) { >>> + if (l2_table_cache) { >>> + qcow2_cache_destroy(bs, l2_table_cache); >>> + } >>> + if (refcount_block_cache) { >>> + qcow2_cache_destroy(bs, refcount_block_cache); >>> + } >>> + } >>> qemu_opts_del(opts); >>> opts = NULL; >> Why don't you squash this into patch 17 (I guess there is a reason, >> but I fail to see it)? > It's a preexisting bug and its fix is unrelated to any of the > refactoring or preparation for reopen. So I think it deserves its own > commit, and if it doesn't, it's not clear to me why it should belong to > patch 17 of all patches. Because patch 17 is introducing the {l2_table,refcount_block}_cache as local variables. >> Also, I think it might make sense to either set >> {l2_table,refcount_block}_cache to NULL once they have been moved to >> s->{l2_table,refcount_block}_cache, or, even better, exchange them >> with their respective field in *s. Thus, you could drop the "if (ret >> < 0)", I think I'd find the code easier to read, and with the >> transation, we'd even be safe if the options had been set before and >> are now to be updated. > The if (ret < 0) disappears shortly after this patch when this is moved > into the abort part of a transaction. Right, I just saw this. :-) Max