From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmUmB-000254-FX for qemu-devel@nongnu.org; Fri, 29 Nov 2013 15:35:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmUm5-0002AD-Fv for qemu-devel@nongnu.org; Fri, 29 Nov 2013 15:34:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62515) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmUm5-0002A8-7S for qemu-devel@nongnu.org; Fri, 29 Nov 2013 15:34:53 -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 rATKYp6V010564 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 29 Nov 2013 15:34:51 -0500 Message-ID: <5298FA8C.7030200@redhat.com> Date: Fri, 29 Nov 2013 21:35:24 +0100 From: Max Reitz MIME-Version: 1.0 References: <1385582728-17932-1-git-send-email-mreitz@redhat.com> <20131128102843.GA4805@dhcp-200-207.str.redhat.com> In-Reply-To: <20131128102843.GA4805@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 28.11.2013 11:28, Kevin Wolf wrote: > Am 27.11.2013 um 21:05 hat Max Reitz geschrieben: >> Leaving the backing file open although it is not needed anymore can >> cause problems if it is opened through a block driver which allows >> exclusive access only and if the create function of the block driver >> used for the top image (the one being created) tries to close and reopen >> the image file (which will include opening the backing file a second >> time). >> >> In particular, this will happen with a backing file opened through >> qemu-nbd and using qcow2 as the top image file format (which reopens the >> image to flush it to disk). >> >> In addition, the BlockDriverState in bdrv_img_create() is used for the >> backing file only; it should therefore be made local to the respective >> block. >> >> Signed-off-by: Max Reitz >> --- >> block.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/block.c b/block.c >> index 382ea71..e5a8a4c 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4504,7 +4504,6 @@ void bdrv_img_create(const char *filename, const char *fmt, >> { >> QEMUOptionParameter *param = NULL, *create_options = NULL; >> QEMUOptionParameter *backing_fmt, *backing_file, *size; >> - BlockDriverState *bs = NULL; >> BlockDriver *drv, *proto_drv; >> BlockDriver *backing_drv = NULL; >> Error *local_err = NULL; >> @@ -4583,6 +4582,7 @@ void bdrv_img_create(const char *filename, const char *fmt, >> size = get_option_parameter(param, BLOCK_OPT_SIZE); >> if (size && size->value.n == -1) { >> if (backing_file && backing_file->value.s) { >> + BlockDriverState *bs; >> uint64_t size; >> char buf[32]; >> int back_flags; > More context: > > /* backing files always opened read-only */ > back_flags = > flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); > > bs = bdrv_new(""); > > ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags, > backing_drv, &local_err); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not open '%s': %s", > backing_file->value.s, > error_get_pretty(local_err)); > error_free(local_err); > local_err = NULL; > goto out; > } > bdrv_get_geometry(bs, &size); > size *= 512; > >> @@ -4608,6 +4608,8 @@ void bdrv_img_create(const char *filename, const char *fmt, >> >> snprintf(buf, sizeof(buf), "%" PRId64, size); >> set_option_parameter(param, BLOCK_OPT_SIZE, buf); >> + >> + bdrv_unref(bs); >> } else { >> error_setg(errp, "Image creation needs a size parameter"); >> goto out; > bs is now leaked if bdrv_open() fails. Ah, right. Thanks. Max