From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC4Ul-0001ME-A3 for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:46:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WC4Uf-0008N7-8l for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:46:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10592) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC4Uf-0008N3-00 for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:46:37 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s189kYmt028511 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 8 Feb 2014 04:46:35 -0500 Message-ID: <52F5FCF8.2070107@redhat.com> Date: Sat, 08 Feb 2014 10:46:32 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1391849670-10352-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1391849670-10352-1-git-send-email-kwolf@redhat.com> Content-Type: multipart/mixed; boundary="------------090709070502050308050904" Subject: Re: [Qemu-devel] [PATCH] 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 This is a multi-part message in MIME format. --------------090709070502050308050904 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit comments below On 02/08/14 09:54, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf > --- > block/blkdebug.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 56c4cd0..519b483 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -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 fail_unref; > } This block appears to run when the bdrv_open_image(&bs->file, ...) call fails. If we can't open the backing file, are you sure we need to unref it under fail_unref? I thought that this block needed no update (one only needs to extend the error handling when allocation of a new resource is successful), but I may have been wrong of course. (Generally in qemu it's always anyone's best guess who is responsible for releasing stuff on the error path, caller or callee.) > > /* Set request alignment */ > @@ -424,10 +424,13 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, > } else { > error_setg(errp, "Invalid alignment"); > ret = -EINVAL; > - goto fail; > + goto fail_unref; > } I do agree with this. > > - ret = 0; > + return 0; > + > +fail_unref: > + bdrv_unref(bs->file); > fail: > qemu_opts_del(opts); > return ret; > I believe this change causes us to leak "opts" on success. ... Unless we should have hung on to it to begin with, of course, in which case this is a silent bugfix. But I don't believe so; I think we only need "opts" temporarily, to absorb options from the QDict, and for ease of parsing into local variables. How about the attached patch instead? Releasing stuff in specific error handler blocks quickly becomes intractable if there are many resources to allocate in succession, but in this case I think we can easily get away with it. Thanks, Laszlo --------------090709070502050308050904 Content-Type: text/plain; charset=ISO-8859-2; name="blkdebug_open.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="blkdebug_open.patch" diff --git a/block/blkdebug.c b/block/blkdebug.c index 56c4cd0..1a8ccc4 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 done; } /* 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 done; } /* 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 done; } /* Set request alignment */ @@ -423,12 +423,13 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, bs->request_alignment = align; } else { error_setg(errp, "Invalid alignment"); + bdrv_unref(bs->file); ret = -EINVAL; - goto fail; + goto done; } ret = 0; -fail: +done: qemu_opts_del(opts); return ret; } --------------090709070502050308050904--