From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60252) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsVDZ-0002DU-MS for qemu-devel@nongnu.org; Wed, 13 May 2015 07:52:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsVDV-0006Ei-E9 for qemu-devel@nongnu.org; Wed, 13 May 2015 07:52:53 -0400 Message-ID: <55533B07.70504@redhat.com> Date: Wed, 13 May 2015 13:52:39 +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> In-Reply-To: <1431105726-3682-19-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; 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 , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org 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)? 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. But since nothing is wrong: Reviewed-by: Max Reitz