| Message ID | 1385582728-17932-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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); }
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(-)