From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57670) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC5WI-0003eJ-WA for qemu-devel@nongnu.org; Sat, 08 Feb 2014 05:52:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WC5WD-00018i-0C for qemu-devel@nongnu.org; Sat, 08 Feb 2014 05:52:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39874) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC5WC-00018d-N4 for qemu-devel@nongnu.org; Sat, 08 Feb 2014 05:52:16 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s18AqFB3029679 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 8 Feb 2014 05:52:15 -0500 Message-ID: <52F60C5D.5040006@redhat.com> Date: Sat, 08 Feb 2014 11:52:13 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1391853624-14369-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1391853624-14369-1-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] blkdebug: Don't leak bs->file on failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@redhat.com On 02/08/14 11:00, Kevin Wolf wrote: > Reported-by: Laszlo Ersek > Signed-off-by: Kevin Wolf > --- > block/blkdebug.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 56c4cd0..8eb0db0 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -396,14 +396,14 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, > if (error_is_set(&local_err)) { > error_propagate(errp, local_err); > ret = -EINVAL; > - goto fail; > + goto out; > } > > /* Read rules from config file or command line options */ > config = qemu_opt_get(opts, "config"); > ret = read_config(s, config, options, errp); > if (ret) { > - goto fail; > + goto out; > } > > /* Set initial state */ > @@ -414,7 +414,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, > flags, true, false, &local_err); > if (ret < 0) { > error_propagate(errp, local_err); > - goto fail; > + goto out; > } > > /* Set request alignment */ > @@ -424,11 +424,15 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, > } else { > error_setg(errp, "Invalid alignment"); > ret = -EINVAL; > - goto fail; > + goto fail_unref; > } > > ret = 0; > -fail: > + goto out; > + > +fail_unref: > + bdrv_unref(bs->file); > +out: > qemu_opts_del(opts); > return ret; > } > I think this is correct, but the standalone "goto out" looks awful :) In such cases, when I have to combine unconditional teardown with release-on-error-only, I apply a "nested ifs" pattern, where the release-on-error-only bits (on the way out) depend on the function exit status. But "nested ifs" is foreign for qemu, so I think these goto's actually match the qemu style. Reviewed-by: Laszlo Ersek Thanks! Laszlo