From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC4dW-0004Wx-V6 for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:55:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WC4dQ-0002S7-Vb for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:55:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61989) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WC4dQ-0002S3-Jj for qemu-devel@nongnu.org; Sat, 08 Feb 2014 04:55:40 -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 s189tdW5030201 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 8 Feb 2014 04:55:39 -0500 Date: Sat, 8 Feb 2014 10:55:37 +0100 From: Kevin Wolf Message-ID: <20140208095537.GD2954@dhcp-200-207.str.redhat.com> References: <1391849670-10352-1-git-send-email-kwolf@redhat.com> <52F5FCF8.2070107@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52F5FCF8.2070107@redhat.com> 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: Laszlo Ersek Cc: qemu-devel@nongnu.org, stefanha@redhat.com Am 08.02.2014 um 10:46 hat Laszlo Ersek geschrieben: > 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. As always, you're right with all you're saying. Sorry for being sloppy. > 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. I'd prefer to avoid this because it's too easy to miss it when you add new code to the function. Let me send a v2 that keeps fail_unref. Kevin