From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsVNB-0006Ln-Oo for qemu-devel@nongnu.org; Wed, 13 May 2015 08:02:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsVN5-00036y-D7 for qemu-devel@nongnu.org; Wed, 13 May 2015 08:02:49 -0400 Date: Wed, 13 May 2015 14:02:29 +0200 From: Kevin Wolf Message-ID: <20150513120229.GG4263@noname.str.redhat.com> References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-19-git-send-email-kwolf@redhat.com> <55533B07.70504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55533B07.70504@redhat.com> 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: Max Reitz Cc: armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org 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. > 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. Kevin