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

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

[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(+)

diff --git a/block.c b/block.c
index 382ea71..6d571ad 100644
--- a/block.c
+++ b/block.c
@@ -4608,6 +4608,9 @@ 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);
+            bs = NULL;
         } else {
             error_setg(errp, "Image creation needs a size parameter");
             goto out;
-- 
1.8.4.2

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

[Benoît Canet]

Le Friday 29 Nov 2013 à 21:41:29 (+0100), Max Reitz a écrit :
> 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(+)
> 
> diff --git a/block.c b/block.c
> index 382ea71..6d571ad 100644
> --- a/block.c
> +++ b/block.c
> @@ -4608,6 +4608,9 @@ 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);
> +            bs = NULL;
>          } else {
>              error_setg(errp, "Image creation needs a size parameter");
>              goto out;
Reviewed-by: Benoit Canet <benoit@irqsave.net>
> -- 
> 1.8.4.2
> 
> 

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

[Wenchao Xia]

于 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;
            }

  It is not a big problem, I am OK if you stick to this version.

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

[Kevin Wolf]

Am 29.11.2013 um 21:41 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).
> 
> 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).

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Stefan Hajnoczi]

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