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

[Qemu-devel] [PATCH v3] 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).

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 <mreitz@redhat.com>
---
v3:
 - Reverted the revert in v2 and limited the BlockDriverState once again
   to the scope its used in (addressing Xia's comment); added a missing
   bdrv_unref() in contrast to v1 (addressing Kevin's comment to v1)

v2:
 - Minimizing the changes prevents introducing a leak of the
   BlockDriverState in case of an error in bdrv_open() (thanks, Kevin).
---
 block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 382ea71..0468765 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;
@@ -4601,6 +4601,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
                                  error_get_pretty(local_err));
                 error_free(local_err);
                 local_err = NULL;
+                bdrv_unref(bs);
                 goto out;
             }
             bdrv_get_geometry(bs, &size);
@@ -4608,6 +4609,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;
@@ -4638,9 +4641,6 @@ out:
     free_option_parameters(create_options);
     free_option_parameters(param);
 
-    if (bs) {
-        bdrv_unref(bs);
-    }
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
     }
-- 
1.8.4.2

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

[Kevin Wolf]

Am 03.12.2013 um 14:57 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 <mreitz@redhat.com>

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

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

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

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

[Stefan Hajnoczi]

On Tue, Dec 03, 2013 at 02:57:52PM +0100, Max Reitz wrote:
> 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 <mreitz@redhat.com>
> ---
> v3:
>  - Reverted the revert in v2 and limited the BlockDriverState once again
>    to the scope its used in (addressing Xia's comment); added a missing
>    bdrv_unref() in contrast to v1 (addressing Kevin's comment to v1)
> 
> v2:
>  - Minimizing the changes prevents introducing a leak of the
>    BlockDriverState in case of an error in bdrv_open() (thanks, Kevin).
> ---
>  block.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan