Re: [Qemu-devel] [PATCH v2] block: Close backing file early in bdrv_img_create

[Stefan Hajnoczi <stefanha@gmail.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