| Message ID | 1364401698-21402-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote: > bdrv_open_common() implements direct use of protocols by copying the > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however > first set some fields in bs, which end up in file after the swap. When > bdrv_open() destroys file, it appears to be open, and because it isn't, > qemu could segfault while trying to close it. > > Reorder the operations to return immediately in such cases so that file > is correctly detected as closed. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 31 +++++++++++++++---------------- > 1 file changed, 15 insertions(+), 16 deletions(-) Didn't apply cleanly, please check that I resolved the conflict correctly. Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben: > On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote: > > bdrv_open_common() implements direct use of protocols by copying the > > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however > > first set some fields in bs, which end up in file after the swap. When > > bdrv_open() destroys file, it appears to be open, and because it isn't, > > qemu could segfault while trying to close it. > > > > Reorder the operations to return immediately in such cases so that file > > is correctly detected as closed. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block.c | 31 +++++++++++++++---------------- > > 1 file changed, 15 insertions(+), 16 deletions(-) > > Didn't apply cleanly, please check that I resolved the conflict > correctly. Sorry, my bad, this was accidentally on top of a temporary quick fix... You conflict resolution is correct. Kevin
On Thu, Mar 28, 2013 at 12:13:40PM +0100, Kevin Wolf wrote: > Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben: > > On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote: > > > bdrv_open_common() implements direct use of protocols by copying the > > > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however > > > first set some fields in bs, which end up in file after the swap. When > > > bdrv_open() destroys file, it appears to be open, and because it isn't, > > > qemu could segfault while trying to close it. > > > > > > Reorder the operations to return immediately in such cases so that file > > > is correctly detected as closed. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > block.c | 31 +++++++++++++++---------------- > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > Didn't apply cleanly, please check that I resolved the conflict > > correctly. > > Sorry, my bad, this was accidentally on top of a temporary quick fix... > You conflict resolution is correct. Thanks. Stefan
diff --git a/block.c b/block.c index 3cc3eb0..0ae2e93 100644 --- a/block.c +++ b/block.c @@ -680,6 +680,18 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, trace_bdrv_open_common(bs, filename, flags, drv->format_name); + if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) { + return -ENOTSUP; + } + + /* bdrv_open() with directly using a protocol as drv. This layer is already + * opened, so assign it to bs (while file becomes a closed BlockDriverState) + * and return immediately. */ + if (file != NULL && drv->bdrv_file_open) { + bdrv_swap(file, bs); + return 0; + } + bs->open_flags = flags; bs->buffer_alignment = 512; @@ -694,15 +706,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bs->filename[0] = '\0'; } - if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) { - return -ENOTSUP; - } - - if (file != NULL && drv->bdrv_file_open) { - bdrv_swap(file, bs); - return 0; - } - bs->drv = drv; bs->opaque = g_malloc0(drv->instance_size); @@ -713,13 +716,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, /* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) { - if (file != NULL) { - bdrv_swap(file, bs); - ret = 0; - } else { - assert(drv->bdrv_parse_filename || filename != NULL); - ret = drv->bdrv_file_open(bs, filename, options, open_flags); - } + assert(file == NULL); + assert(drv->bdrv_parse_filename || filename != NULL); + ret = drv->bdrv_file_open(bs, filename, options, open_flags); } else { assert(file != NULL); bs->file = file;
bdrv_open_common() implements direct use of protocols by copying the pre-opened BlockDriverStates to bs using bdrv_swap(). It did however first set some fields in bs, which end up in file after the swap. When bdrv_open() destroys file, it appears to be open, and because it isn't, qemu could segfault while trying to close it. Reorder the operations to return immediately in such cases so that file is correctly detected as closed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)