| Message ID | 1385757689-23524-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
于 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.
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>
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
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;
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(+)