* [Qemu-devel] [PATCH 0/2] block: Avoid second open for format probing @ 2012-11-13 14:14 Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 1/2] block: Factor out bdrv_open_flags Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing Kevin Wolf 0 siblings, 2 replies; 6+ messages in thread From: Kevin Wolf @ 2012-11-13 14:14 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf Kevin Wolf (2): block: Factor out bdrv_open_flags block: Avoid second open for format probing block.c | 93 +++++++++++++++++++++++++++++++++++--------------------------- 1 files changed, 52 insertions(+), 41 deletions(-) -- 1.7.6.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] block: Factor out bdrv_open_flags 2012-11-13 14:14 [Qemu-devel] [PATCH 0/2] block: Avoid second open for format probing Kevin Wolf @ 2012-11-13 14:14 ` Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing Kevin Wolf 1 sibling, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2012-11-13 14:14 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 35 +++++++++++++++++++++-------------- 1 files changed, 21 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 854ebd6..a55b06c 100644 --- a/block.c +++ b/block.c @@ -634,6 +634,26 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs) bs->copy_on_read--; } +static int bdrv_open_flags(BlockDriverState *bs, int flags) +{ + int open_flags = flags | BDRV_O_CACHE_WB; + + /* + * Clear flags that are internal to the block layer before opening the + * image. + */ + open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); + + /* + * Snapshots should be writable. + */ + if (bs->is_temporary) { + open_flags |= BDRV_O_RDWR; + } + + return open_flags; +} + /* * Common part for opening disk images and files */ @@ -665,20 +685,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, bs->opaque = g_malloc0(drv->instance_size); bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB); - open_flags = flags | BDRV_O_CACHE_WB; - - /* - * Clear flags that are internal to the block layer before opening the - * image. - */ - open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); - - /* - * Snapshots should be writable. - */ - if (bs->is_temporary) { - open_flags |= BDRV_O_RDWR; - } + open_flags = bdrv_open_flags(bs, flags); bs->read_only = !(open_flags & BDRV_O_RDWR); -- 1.7.6.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing 2012-11-13 14:14 [Qemu-devel] [PATCH 0/2] block: Avoid second open for format probing Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 1/2] block: Factor out bdrv_open_flags Kevin Wolf @ 2012-11-13 14:14 ` Kevin Wolf 2012-11-14 8:32 ` Stefan Hajnoczi 1 sibling, 1 reply; 6+ messages in thread From: Kevin Wolf @ 2012-11-13 14:14 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf This fixes problems that are caused by the additional open/close cycle of the existing format probing, for example related to qemu-nbd without -t option or file descriptor passing. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 58 +++++++++++++++++++++++++++++++--------------------------- 1 files changed, 31 insertions(+), 27 deletions(-) diff --git a/block.c b/block.c index a55b06c..626d6c2 100644 --- a/block.c +++ b/block.c @@ -518,22 +518,16 @@ BlockDriver *bdrv_find_protocol(const char *filename) return NULL; } -static int find_image_format(const char *filename, BlockDriver **pdrv) +static int find_image_format(BlockDriverState *bs, const char *filename, + BlockDriver **pdrv) { - int ret, score, score_max; + int score, score_max; BlockDriver *drv1, *drv; uint8_t buf[2048]; - BlockDriverState *bs; - - ret = bdrv_file_open(&bs, filename, 0); - if (ret < 0) { - *pdrv = NULL; - return ret; - } + int ret = 0; /* Return the raw BlockDriver * to scsi-generic devices or empty drives */ if (bs->sg || !bdrv_is_inserted(bs)) { - bdrv_delete(bs); drv = bdrv_find_format("raw"); if (!drv) { ret = -ENOENT; @@ -543,7 +537,6 @@ static int find_image_format(const char *filename, BlockDriver **pdrv) } ret = bdrv_pread(bs, 0, buf, sizeof(buf)); - bdrv_delete(bs); if (ret < 0) { *pdrv = NULL; return ret; @@ -657,7 +650,8 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags) /* * Common part for opening disk images and files */ -static int bdrv_open_common(BlockDriverState *bs, const char *filename, +static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, + const char *filename, int flags, BlockDriver *drv) { int ret, open_flags; @@ -691,12 +685,15 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, /* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) { + if (file != NULL) { + bdrv_swap(file, bs); + bdrv_delete(file); + } ret = drv->bdrv_file_open(bs, filename, open_flags); } else { - ret = bdrv_file_open(&bs->file, filename, open_flags); - if (ret >= 0) { - ret = drv->bdrv_open(bs, open_flags); - } + assert(file != NULL); + bs->file = file; + ret = drv->bdrv_open(bs, open_flags); } if (ret < 0) { @@ -716,10 +713,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, return 0; free_and_fail: - if (bs->file) { - bdrv_delete(bs->file); - bs->file = NULL; - } + bs->file = NULL; g_free(bs->opaque); bs->opaque = NULL; bs->drv = NULL; @@ -741,7 +735,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags) } bs = bdrv_new(""); - ret = bdrv_open_common(bs, filename, flags, drv); + ret = bdrv_open_common(bs, NULL, filename, flags, drv); if (ret < 0) { bdrv_delete(bs); return ret; @@ -795,6 +789,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, { int ret; char tmp_filename[PATH_MAX]; + BlockDriverState *file = NULL; if (flags & BDRV_O_SNAPSHOT) { BlockDriverState *bs1; @@ -854,21 +849,27 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, bs->is_temporary = 1; } + /* Open image file without format layer */ + if (flags & BDRV_O_RDWR) { + flags |= BDRV_O_ALLOW_RDWR; + } + + ret = bdrv_file_open(&file, filename, bdrv_open_flags(bs, flags)); + if (ret < 0) { + return ret; + } + /* Find the right image format driver */ if (!drv) { - ret = find_image_format(filename, &drv); + ret = find_image_format(file, filename, &drv); } if (!drv) { goto unlink_and_fail; } - if (flags & BDRV_O_RDWR) { - flags |= BDRV_O_ALLOW_RDWR; - } - /* Open the image */ - ret = bdrv_open_common(bs, filename, flags, drv); + ret = bdrv_open_common(bs, file, filename, flags, drv); if (ret < 0) { goto unlink_and_fail; } @@ -894,6 +895,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, return 0; unlink_and_fail: + if (file != NULL) { + bdrv_delete(file); + } if (bs->is_temporary) { unlink(filename); } -- 1.7.6.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing 2012-11-13 14:14 ` [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing Kevin Wolf @ 2012-11-14 8:32 ` Stefan Hajnoczi 2012-11-14 8:51 ` Paolo Bonzini 2012-11-14 9:03 ` Kevin Wolf 0 siblings, 2 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2012-11-14 8:32 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel On Tue, Nov 13, 2012 at 03:14:55PM +0100, Kevin Wolf wrote: > @@ -691,12 +685,15 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > > /* Open the image, either directly or using a protocol */ > if (drv->bdrv_file_open) { > + if (file != NULL) { > + bdrv_swap(file, bs); > + bdrv_delete(file); > + } > ret = drv->bdrv_file_open(bs, filename, open_flags); > } else { [...] > /* Open the image */ > - ret = bdrv_open_common(bs, filename, flags, drv); > + ret = bdrv_open_common(bs, file, filename, flags, drv); > if (ret < 0) { > goto unlink_and_fail; > } > @@ -894,6 +895,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > return 0; > > unlink_and_fail: > + if (file != NULL) { > + bdrv_delete(file); > + } Not sure I understand this code path. We have a protocol (the driver implements .bdrv_file_open()) so we swap file and bs, then delete old bs. Then we call .bdrv_file_open() on the already open file BDS. Is it okay to call .bdrv_file_open() on an already open BDS? Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted file BDS. This is a double-free. Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing 2012-11-14 8:32 ` Stefan Hajnoczi @ 2012-11-14 8:51 ` Paolo Bonzini 2012-11-14 9:03 ` Kevin Wolf 1 sibling, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2012-11-14 8:51 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel Il 14/11/2012 09:32, Stefan Hajnoczi ha scritto: > On Tue, Nov 13, 2012 at 03:14:55PM +0100, Kevin Wolf wrote: >> @@ -691,12 +685,15 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> >> /* Open the image, either directly or using a protocol */ >> if (drv->bdrv_file_open) { >> + if (file != NULL) { >> + bdrv_swap(file, bs); >> + bdrv_delete(file); >> + } >> ret = drv->bdrv_file_open(bs, filename, open_flags); >> } else { > [...] >> /* Open the image */ >> - ret = bdrv_open_common(bs, filename, flags, drv); >> + ret = bdrv_open_common(bs, file, filename, flags, drv); >> if (ret < 0) { >> goto unlink_and_fail; >> } >> @@ -894,6 +895,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >> return 0; >> >> unlink_and_fail: >> + if (file != NULL) { >> + bdrv_delete(file); >> + } > > Not sure I understand this code path. > > We have a protocol (the driver implements .bdrv_file_open()) so we swap > file and bs, then delete old bs. Then we call .bdrv_file_open() on the > already open file BDS. > > Is it okay to call .bdrv_file_open() on an already open BDS? I don't think so. But do the cases where this happen make sense? Can we just fail if drv is not equal to bs->drv if we reach the "if (drv->bdrv_file_open)" case? That would be for cases like "-drive file=test.img,format=host_device" but how does that make sense?... (Plus, it fails to stack the raw format on top). So perhaps we could even assert(drv == bs->drv) if protocols had no .format_name? > Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted > file BDS. This is a double-free. Right, always better to NULL out whatever you delete (which means passing a BDS** to bdrv_open_common. Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing 2012-11-14 8:32 ` Stefan Hajnoczi 2012-11-14 8:51 ` Paolo Bonzini @ 2012-11-14 9:03 ` Kevin Wolf 1 sibling, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2012-11-14 9:03 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel Am 14.11.2012 09:32, schrieb Stefan Hajnoczi: > On Tue, Nov 13, 2012 at 03:14:55PM +0100, Kevin Wolf wrote: >> @@ -691,12 +685,15 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> >> /* Open the image, either directly or using a protocol */ >> if (drv->bdrv_file_open) { >> + if (file != NULL) { >> + bdrv_swap(file, bs); >> + bdrv_delete(file); >> + } >> ret = drv->bdrv_file_open(bs, filename, open_flags); >> } else { > [...] >> /* Open the image */ >> - ret = bdrv_open_common(bs, filename, flags, drv); >> + ret = bdrv_open_common(bs, file, filename, flags, drv); >> if (ret < 0) { >> goto unlink_and_fail; >> } >> @@ -894,6 +895,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >> return 0; >> >> unlink_and_fail: >> + if (file != NULL) { >> + bdrv_delete(file); >> + } > > Not sure I understand this code path. > > We have a protocol (the driver implements .bdrv_file_open()) so we swap > file and bs, then delete old bs. Then we call .bdrv_file_open() on the > already open file BDS. > > Is it okay to call .bdrv_file_open() on an already open BDS? No, that looks buggy, even though in practice it doesn't explode at least with raw-posix. It probably did leak a file descriptor. > Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted > file BDS. This is a double-free. I'll move the bdrv_delete up into bdrv_open (conditional on bs->file != file) and add a file = NULL after it, that should fix it. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-14 9:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-13 14:14 [Qemu-devel] [PATCH 0/2] block: Avoid second open for format probing Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 1/2] block: Factor out bdrv_open_flags Kevin Wolf 2012-11-13 14:14 ` [Qemu-devel] [PATCH 2/2] block: Avoid second open for format probing Kevin Wolf 2012-11-14 8:32 ` Stefan Hajnoczi 2012-11-14 8:51 ` Paolo Bonzini 2012-11-14 9:03 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).