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

[Max Reitz <mreitz@redhat.com>]

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