From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43500)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VnTCM-0001eE-M7
	for qemu-devel@nongnu.org; Mon, 02 Dec 2013 08:06:11 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VnTCE-0007wk-7A
	for qemu-devel@nongnu.org; Mon, 02 Dec 2013 08:06:02 -0500
Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:50752)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VnTCE-0007wg-0I
	for qemu-devel@nongnu.org; Mon, 02 Dec 2013 08:05:54 -0500
Received: by mail-wi0-f175.google.com with SMTP id hi5so4738197wib.14
	for <qemu-devel@nongnu.org>; Mon, 02 Dec 2013 05:05:52 -0800 (PST)
Date: Mon, 2 Dec 2013 14:05:49 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20131202130549.GB24710@stefanha-thinkpad.redhat.com>
References: <1385757689-23524-1-git-send-email-mreitz@redhat.com>
	<529C0080.7070106@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <529C0080.7070106@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH v2] block: Close backing file early in
 bdrv_img_create
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Max Reitz <mreitz@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Wenchao Xia <xiawenc@linux.vnet.ibm.com>

On Mon, Dec 02, 2013 at 11:37:36AM +0800, Wenchao Xia wrote:
> 于 2013/11/30 4:41, Max Reitz 写道:
> > 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).
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> > v2:
> >   - Minimizing the changes prevents introducing a leak of the
> >     BlockDriverState in case of an error in bdrv_open() (thanks, Kevin).
> > ---
> >   block.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> 
> Minor comments:
>   I think your v1 have better orgnize of code, since it tips reader
> that bs is a variable used only in backing file code. Why not improve
> it by just adding one line in v1:
> 
> line 4587:
>             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;
>                 bdrv_unref(bs);
>                 goto out;
>             }

Agreed, tightening the scope of 'bs' was a good idea.

Max: can you send a final version as suggested by Wenchao?

Thanks,
Stefan