* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.