All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create
@ 2013-11-27 20:05 Max Reitz
  2013-11-28 10:28 ` Kevin Wolf
  0 siblings, 1 reply; 3+ messages in thread
From: Max Reitz @ 2013-11-27 20:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, 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>
---
 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;
@@ -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;
@@ -4638,9 +4640,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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create
  2013-11-27 20:05 [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create Max Reitz
@ 2013-11-28 10:28 ` Kevin Wolf
  2013-11-29 20:35   ` Max Reitz
  0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2013-11-28 10:28 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi

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 <mreitz@redhat.com>
> ---
>  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.

Kevin

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create
  2013-11-28 10:28 ` Kevin Wolf
@ 2013-11-29 20:35   ` Max Reitz
  0 siblings, 0 replies; 3+ messages in thread
From: Max Reitz @ 2013-11-29 20:35 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, 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 <mreitz@redhat.com>
>> ---
>>   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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-29 20:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-27 20:05 [Qemu-devel] [PATCH] block: Close backing file early in bdrv_img_create Max Reitz
2013-11-28 10:28 ` Kevin Wolf
2013-11-29 20:35   ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.