* [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
@ 2013-08-05 18:44 Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn Charlie Shepherd
` (7 more replies)
0 siblings, 8 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, gabriel, Charlie Shepherd, stefanha
This patch series is a follow up to a previous RFC about converting functions
that dynamically yield execution depending on whether they are in executing in
a coroutine context or not to be explicitly statically annotated. This change
is necessary for the GSoC CPC project, but was also agreed in an IRC
conversation on #qemu to be benefical overall if it can be upstream before the
end of the project. This is an update to see if the approach I'm taking to
implementing this conversion is correct.
In order to statically check the tree to ensure the annotations are correct,
I've been using CPC to compile the QEMU tree. This does a source to source
translation to convert coroutine code to continuation-passing style (the
purpose of the GSoC project), but as a side benefit statically checks
annotations (any functions annotated with coroutine_fn are transformed into
CPS, so have a different "calling style" to the standard C convention).
In order to compile the tree with CPC:
$ git clone git://github.com/kerneis/cpc.git
$ cd cpc
$ make
$ ./configure
$ make
$ cd ..
$ export CPC=$(pwd)/cpc/bin/cpc
$ cd qemu
$ mkdir -p bin/cpc
$ cd bin/cpc
$ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --warnall "
$ make
Charlie Shepherd (5):
Add an explanation of when a function should be marked
qemu_coroutine_self should not be marked coroutine_fn
Convert BlockDriver to explicit coroutine annotations
Convert block functions to coroutine versions
Convert block layer callers' annotations
block.c | 247 ++++++++++++++++++++++--------------------
block/blkdebug.c | 10 +-
block/blkverify.c | 4 +-
block/bochs.c | 12 +-
block/cloop.c | 10 +-
block/cow.c | 20 ++--
block/curl.c | 12 +-
block/dmg.c | 14 +--
block/mirror.c | 4 +-
block/nbd.c | 28 ++---
block/parallels.c | 6 +-
block/qcow.c | 14 +--
block/qcow2-cluster.c | 8 +-
block/qcow2.c | 66 ++++++++---
block/qcow2.h | 6 +-
block/qed.c | 8 +-
block/raw-posix.c | 34 +++---
block/raw.c | 8 +-
block/sheepdog.c | 24 ++--
block/snapshot.c | 32 +++++-
block/ssh.c | 14 +--
block/vdi.c | 16 +--
block/vhdx.c | 4 +-
block/vmdk.c | 46 ++++----
block/vpc.c | 16 +--
block/vvfat.c | 32 +++---
blockjob.c | 2 +-
coroutine-ucontext.c | 2 +-
include/block/block.h | 37 ++++---
include/block/block_int.h | 10 +-
include/block/blockjob.h | 2 +-
include/block/coroutine.h | 9 +-
include/block/coroutine_int.h | 2 +-
nbd.c | 4 +-
qemu-coroutine-lock.c | 4 +-
qemu-img.c | 54 +++++++--
qemu-io-cmds.c | 12 +-
37 files changed, 477 insertions(+), 356 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
@ 2013-08-05 18:44 ` Charlie Shepherd
2013-08-06 8:39 ` Stefan Hajnoczi
2013-08-05 18:44 ` [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield Charlie Shepherd
` (6 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, Charlie Shepherd, gabriel, stefanha
From: Charlie Shepherd <cs648@cam.ac.uk>
Coroutine functions that can yield directly or indirectly should be annotated
with a coroutine_fn annotation. Add an explanation to that effect in
include/block/coroutine.h.
---
include/block/coroutine.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 377805a..3b94b6d 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -37,6 +37,9 @@
* static checker support for catching such errors. This annotation might make
* it possible and in the meantime it serves as documentation.
*
+ * A function must be marked with coroutine_fn if it can yield execution, either
+ * directly or indirectly.
+ *
* For example:
*
* static void coroutine_fn foo(void) {
--
1.8.3.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn Charlie Shepherd
@ 2013-08-05 18:44 ` Charlie Shepherd
2013-08-07 19:18 ` Stefan Hajnoczi
2013-08-05 18:44 ` [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations Charlie Shepherd
` (5 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, Charlie Shepherd, gabriel, stefanha
From: Charlie Shepherd <cs648@cam.ac.uk>
While it only really makes sense to call qemu_coroutine_self() in a coroutine
context, it cannot actually yield execution itself, so remove the coroutine_fn
annotation.
---
include/block/coroutine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 3b94b6d..563dcde 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -87,7 +87,7 @@ void coroutine_fn qemu_coroutine_yield(void);
/**
* Get the currently executing coroutine
*/
-Coroutine *coroutine_fn qemu_coroutine_self(void);
+Coroutine *qemu_coroutine_self(void);
/**
* Return whether or not currently inside a coroutine
--
1.8.3.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield Charlie Shepherd
@ 2013-08-05 18:44 ` Charlie Shepherd
2013-08-05 19:23 ` Gabriel Kerneis
2013-08-06 9:24 ` Kevin Wolf
2013-08-05 18:44 ` [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions Charlie Shepherd
` (4 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, gabriel, Charlie Shepherd, stefanha
This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
they are executed in a coroutine context or not.
---
block.c | 16 +++++++--------
block/blkdebug.c | 10 ++++-----
block/blkverify.c | 4 ++--
block/bochs.c | 8 ++++----
block/cloop.c | 6 +++---
block/cow.c | 12 +++++------
block/curl.c | 12 +++++------
block/dmg.c | 6 +++---
block/nbd.c | 28 ++++++++++++-------------
block/parallels.c | 6 +++---
block/qcow.c | 8 ++++----
block/qcow2-cluster.c | 8 ++++----
block/qcow2.c | 48 +++++++++++++++++++++++++++++++++++++------
block/qcow2.h | 4 ++--
block/qed.c | 8 ++++----
block/raw-posix.c | 34 +++++++++++++++---------------
block/raw.c | 8 ++++----
block/sheepdog.c | 24 +++++++++++-----------
block/snapshot.c | 32 ++++++++++++++++++++++++++++-
block/ssh.c | 14 ++++++-------
block/vdi.c | 12 +++++------
block/vhdx.c | 4 ++--
block/vmdk.c | 12 +++++------
block/vpc.c | 12 +++++------
block/vvfat.c | 12 +++++------
include/block/block_int.h | 10 ++++-----
include/block/coroutine.h | 4 ++--
include/block/coroutine_int.h | 2 +-
qemu-coroutine-lock.c | 4 ++--
30 files changed, 218 insertions(+), 152 deletions(-)
diff --git a/block.c b/block.c
index 6c493ad..aaa122c 100644
--- a/block.c
+++ b/block.c
@@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
CreateCo *cco = opaque;
assert(cco->drv);
- cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
+ cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
}
int bdrv_create(BlockDriver *drv, const char* filename,
@@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
.ret = NOT_DONE,
};
- if (!drv->bdrv_create) {
+ if (!drv->bdrv_co_create) {
ret = -ENOTSUP;
goto out;
}
@@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
/* 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) {
+ if (file != NULL && drv->bdrv_co_file_open) {
bdrv_swap(file, bs);
return 0;
}
@@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
/* Open the image, either directly or using a protocol */
- if (drv->bdrv_file_open) {
+ if (drv->bdrv_co_file_open) {
assert(file == NULL);
assert(drv->bdrv_parse_filename || filename != NULL);
- ret = drv->bdrv_file_open(bs, options, open_flags);
+ ret = drv->bdrv_co_file_open(bs, options, open_flags);
} else {
if (file == NULL) {
qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
@@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
}
assert(file != NULL);
bs->file = file;
- ret = drv->bdrv_open(bs, options, open_flags);
+ ret = drv->bdrv_co_open(bs, options, open_flags);
}
if (ret < 0) {
@@ -3759,9 +3759,9 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
if (is_write) {
qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
- acb->ret = bs->drv->bdrv_write(bs, sector_num, acb->bounce, nb_sectors);
+ acb->ret = bs->drv->bdrv_co_write(bs, sector_num, acb->bounce, nb_sectors);
} else {
- acb->ret = bs->drv->bdrv_read(bs, sector_num, acb->bounce, nb_sectors);
+ acb->ret = bs->drv->bdrv_co_read(bs, sector_num, acb->bounce, nb_sectors);
}
qemu_bh_schedule(acb->bh);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index ccb627a..9dd5697 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -349,7 +349,7 @@ static QemuOptsList runtime_opts = {
},
};
-static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVBlkdebugState *s = bs->opaque;
QemuOpts *opts;
@@ -494,7 +494,7 @@ static void blkdebug_close(BlockDriverState *bs)
}
}
-static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
+static void coroutine_fn suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
{
BDRVBlkdebugState *s = bs->opaque;
BlkdebugSuspendedReq r;
@@ -515,7 +515,7 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
g_free(r.tag);
}
-static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+static bool coroutine_fn process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
bool injected)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -546,7 +546,7 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
return injected;
}
-static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
+static void coroutine_fn blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
{
BDRVBlkdebugState *s = bs->opaque;
struct BlkdebugRule *rule, *next;
@@ -626,7 +626,7 @@ static BlockDriver bdrv_blkdebug = {
.instance_size = sizeof(BDRVBlkdebugState),
.bdrv_parse_filename = blkdebug_parse_filename,
- .bdrv_file_open = blkdebug_open,
+ .bdrv_co_file_open = blkdebug_open,
.bdrv_close = blkdebug_close,
.bdrv_getlength = blkdebug_getlength,
diff --git a/block/blkverify.c b/block/blkverify.c
index 1d58cc3..0b89cfe 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,7 +116,7 @@ static QemuOptsList runtime_opts = {
},
};
-static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn blkverify_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVBlkverifyState *s = bs->opaque;
QemuOpts *opts;
@@ -405,7 +405,7 @@ static BlockDriver bdrv_blkverify = {
.instance_size = sizeof(BDRVBlkverifyState),
.bdrv_parse_filename = blkverify_parse_filename,
- .bdrv_file_open = blkverify_open,
+ .bdrv_co_file_open = blkverify_co_open,
.bdrv_close = blkverify_close,
.bdrv_getlength = blkverify_getlength,
diff --git a/block/bochs.c b/block/bochs.c
index d7078c0..c827bd4 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -108,7 +108,7 @@ static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int bochs_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn bochs_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVBochsState *s = bs->opaque;
int i;
@@ -228,7 +228,7 @@ static coroutine_fn int bochs_co_read(BlockDriverState *bs, int64_t sector_num,
return ret;
}
-static void bochs_close(BlockDriverState *bs)
+static void coroutine_fn bochs_close(BlockDriverState *bs)
{
BDRVBochsState *s = bs->opaque;
g_free(s->catalog_bitmap);
@@ -238,8 +238,8 @@ static BlockDriver bdrv_bochs = {
.format_name = "bochs",
.instance_size = sizeof(BDRVBochsState),
.bdrv_probe = bochs_probe,
- .bdrv_open = bochs_open,
- .bdrv_read = bochs_co_read,
+ .bdrv_co_open = bochs_open,
+ .bdrv_co_read = bochs_co_read,
.bdrv_close = bochs_close,
};
diff --git a/block/cloop.c b/block/cloop.c
index 6ea7cf4..ef5555f 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -53,7 +53,7 @@ static int cloop_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int cloop_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cloop_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVCloopState *s = bs->opaque;
uint32_t offsets_size, max_compressed_block_size = 1, i;
@@ -191,8 +191,8 @@ static BlockDriver bdrv_cloop = {
.format_name = "cloop",
.instance_size = sizeof(BDRVCloopState),
.bdrv_probe = cloop_probe,
- .bdrv_open = cloop_open,
- .bdrv_read = cloop_co_read,
+ .bdrv_co_open = cloop_open,
+ .bdrv_co_read = cloop_co_read,
.bdrv_close = cloop_close,
};
diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..c68c5ae 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -58,7 +58,7 @@ static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int cow_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cow_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVCowState *s = bs->opaque;
struct cow_header_v2 cow_header;
@@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs)
{
}
-static int cow_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn cow_co_create(const char *filename, QEMUOptionParameter *options)
{
struct cow_header_v2 cow_header;
struct stat st;
@@ -337,13 +337,13 @@ static BlockDriver bdrv_cow = {
.instance_size = sizeof(BDRVCowState),
.bdrv_probe = cow_probe,
- .bdrv_open = cow_open,
+ .bdrv_co_open = cow_co_open,
.bdrv_close = cow_close,
- .bdrv_create = cow_create,
+ .bdrv_co_create = cow_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_read = cow_co_read,
- .bdrv_write = cow_co_write,
+ .bdrv_co_read = cow_co_read,
+ .bdrv_co_write = cow_co_write,
.bdrv_co_is_allocated = cow_co_is_allocated,
.create_options = cow_create_options,
diff --git a/block/curl.c b/block/curl.c
index 6af8cb7..e2e34f4 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -395,7 +395,7 @@ static QemuOptsList runtime_opts = {
},
};
-static int curl_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn curl_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVCURLState *s = bs->opaque;
CURLState *state = NULL;
@@ -622,7 +622,7 @@ static BlockDriver bdrv_http = {
.instance_size = sizeof(BDRVCURLState),
.bdrv_parse_filename = curl_parse_filename,
- .bdrv_file_open = curl_open,
+ .bdrv_co_file_open = curl_co_open,
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
@@ -635,7 +635,7 @@ static BlockDriver bdrv_https = {
.instance_size = sizeof(BDRVCURLState),
.bdrv_parse_filename = curl_parse_filename,
- .bdrv_file_open = curl_open,
+ .bdrv_co_file_open = curl_co_open,
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
@@ -648,7 +648,7 @@ static BlockDriver bdrv_ftp = {
.instance_size = sizeof(BDRVCURLState),
.bdrv_parse_filename = curl_parse_filename,
- .bdrv_file_open = curl_open,
+ .bdrv_co_file_open = curl_co_open,
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
@@ -661,7 +661,7 @@ static BlockDriver bdrv_ftps = {
.instance_size = sizeof(BDRVCURLState),
.bdrv_parse_filename = curl_parse_filename,
- .bdrv_file_open = curl_open,
+ .bdrv_co_file_open = curl_co_open,
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
@@ -674,7 +674,7 @@ static BlockDriver bdrv_tftp = {
.instance_size = sizeof(BDRVCURLState),
.bdrv_parse_filename = curl_parse_filename,
- .bdrv_file_open = curl_open,
+ .bdrv_co_file_open = curl_co_open,
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
diff --git a/block/dmg.c b/block/dmg.c
index 3141cb5..346aa7d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -92,7 +92,7 @@ static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
return 0;
}
-static int dmg_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn dmg_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVDMGState *s = bs->opaque;
uint64_t info_begin,info_end,last_in_offset,last_out_offset;
@@ -378,8 +378,8 @@ static BlockDriver bdrv_dmg = {
.format_name = "dmg",
.instance_size = sizeof(BDRVDMGState),
.bdrv_probe = dmg_probe,
- .bdrv_open = dmg_open,
- .bdrv_read = dmg_co_read,
+ .bdrv_co_open = dmg_co_open,
+ .bdrv_co_read = dmg_co_read,
.bdrv_close = dmg_close,
};
diff --git a/block/nbd.c b/block/nbd.c
index 9c480b8..3e037ba 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -256,7 +256,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
}
-static void nbd_coroutine_start(BDRVNBDState *s, struct nbd_request *request)
+static void coroutine_fn nbd_coroutine_start(BDRVNBDState *s, struct nbd_request *request)
{
int i;
@@ -334,7 +334,7 @@ static void nbd_restart_write(void *opaque)
qemu_coroutine_enter(s->send_coroutine, NULL);
}
-static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
+static int coroutine_fn nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
QEMUIOVector *qiov, int offset)
{
int rc, ret;
@@ -368,7 +368,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
return rc;
}
-static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
+static void coroutine_fn nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
struct nbd_reply *reply,
QEMUIOVector *qiov, int offset)
{
@@ -394,7 +394,7 @@ static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
}
}
-static void nbd_coroutine_end(BDRVNBDState *s, struct nbd_request *request)
+static void coroutine_fn nbd_coroutine_end(BDRVNBDState *s, struct nbd_request *request)
{
int i = HANDLE_TO_INDEX(s, request->handle);
s->recv_coroutine[i] = NULL;
@@ -463,7 +463,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
closesocket(s->sock);
}
-static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn nbd_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVNBDState *s = bs->opaque;
int result;
@@ -485,7 +485,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
return result;
}
-static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov,
int offset)
{
@@ -510,7 +510,7 @@ static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
}
-static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov,
int offset)
{
@@ -542,7 +542,7 @@ static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
* remain aligned to 4K. */
#define NBD_MAX_SECTORS 2040
-static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov)
{
int offset = 0;
@@ -559,7 +559,7 @@ static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
}
-static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov)
{
int offset = 0;
@@ -576,7 +576,7 @@ static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
return nbd_co_writev_1(bs, sector_num, nb_sectors, qiov, offset);
}
-static int nbd_co_flush(BlockDriverState *bs)
+static int coroutine_fn nbd_co_flush(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
struct nbd_request request;
@@ -606,7 +606,7 @@ static int nbd_co_flush(BlockDriverState *bs)
return -reply.error;
}
-static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
int nb_sectors)
{
BDRVNBDState *s = bs->opaque;
@@ -653,7 +653,7 @@ static BlockDriver bdrv_nbd = {
.protocol_name = "nbd",
.instance_size = sizeof(BDRVNBDState),
.bdrv_parse_filename = nbd_parse_filename,
- .bdrv_file_open = nbd_open,
+ .bdrv_co_file_open = nbd_co_open,
.bdrv_co_readv = nbd_co_readv,
.bdrv_co_writev = nbd_co_writev,
.bdrv_close = nbd_close,
@@ -667,7 +667,7 @@ static BlockDriver bdrv_nbd_tcp = {
.protocol_name = "nbd+tcp",
.instance_size = sizeof(BDRVNBDState),
.bdrv_parse_filename = nbd_parse_filename,
- .bdrv_file_open = nbd_open,
+ .bdrv_co_file_open = nbd_co_open,
.bdrv_co_readv = nbd_co_readv,
.bdrv_co_writev = nbd_co_writev,
.bdrv_close = nbd_close,
@@ -681,7 +681,7 @@ static BlockDriver bdrv_nbd_unix = {
.protocol_name = "nbd+unix",
.instance_size = sizeof(BDRVNBDState),
.bdrv_parse_filename = nbd_parse_filename,
- .bdrv_file_open = nbd_open,
+ .bdrv_co_file_open = nbd_co_open,
.bdrv_co_readv = nbd_co_readv,
.bdrv_co_writev = nbd_co_writev,
.bdrv_close = nbd_close,
diff --git a/block/parallels.c b/block/parallels.c
index 18b3ac0..1f9aaf9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -68,7 +68,7 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
return 0;
}
-static int parallels_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn parallels_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVParallelsState *s = bs->opaque;
int i;
@@ -164,8 +164,8 @@ static BlockDriver bdrv_parallels = {
.format_name = "parallels",
.instance_size = sizeof(BDRVParallelsState),
.bdrv_probe = parallels_probe,
- .bdrv_open = parallels_open,
- .bdrv_read = parallels_co_read,
+ .bdrv_co_open = parallels_co_open,
+ .bdrv_co_read = parallels_co_read,
.bdrv_close = parallels_close,
};
diff --git a/block/qcow.c b/block/qcow.c
index 5239bd6..04f59f2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -92,7 +92,7 @@ static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int qcow_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn qcow_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVQcowState *s = bs->opaque;
int len, i, shift, ret;
@@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
-static int qcow_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn qcow_create(const char *filename, QEMUOptionParameter *options)
{
int header_size, backing_filename_len, l1_size, shift, i;
QCowHeader header;
@@ -888,10 +888,10 @@ static BlockDriver bdrv_qcow = {
.format_name = "qcow",
.instance_size = sizeof(BDRVQcowState),
.bdrv_probe = qcow_probe,
- .bdrv_open = qcow_open,
+ .bdrv_co_open = qcow_co_open,
.bdrv_close = qcow_close,
.bdrv_reopen_prepare = qcow_reopen_prepare,
- .bdrv_create = qcow_create,
+ .bdrv_co_create = qcow_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_readv = qcow_co_readv,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cca76d4..50262d3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -628,7 +628,7 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
return cluster_offset;
}
-static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
+static int coroutine_fn perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
{
BDRVQcowState *s = bs->opaque;
int ret;
@@ -657,7 +657,7 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
return 0;
}
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
+int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
{
BDRVQcowState *s = bs->opaque;
int i, j = 0, l2_index, ret;
@@ -783,7 +783,7 @@ out:
* information on cluster allocation may be invalid now. The caller
* must start over anyway, so consider *cur_bytes undefined.
*/
-static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
+static int coroutine_fn handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
uint64_t *cur_bytes, QCowL2Meta **m)
{
BDRVQcowState *s = bs->opaque;
@@ -1149,7 +1149,7 @@ fail:
*
* Return 0 on success and -errno in error cases
*/
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
+int coroutine_fn qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
{
BDRVQcowState *s = bs->opaque;
diff --git a/block/qcow2.c b/block/qcow2.c
index 0eceefe..2ed0bb6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -58,6 +58,10 @@ typedef struct {
#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
#define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+
+#define NOT_DONE 0x7fffffff
+
+
static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
{
const QCowHeader *cow_header = (const void *)buf;
@@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = {
},
};
-static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVQcowState *s = bs->opaque;
int len, i, ret = 0;
@@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
return ret;
}
+struct QOpenCo {
+ BlockDriverState *bs;
+ QDict *options;
+ int flags;
+ int ret;
+};
+
+static void coroutine_fn qcow2_co_open_entry(void *opaque)
+{
+ struct QOpenCo *qo = opaque;
+
+ qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags);
+}
+
+static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
+{
+ Coroutine *co;
+ struct QOpenCo qo = {
+ .bs = bs,
+ .options = options,
+ .flags = flags,
+ .ret = NOT_DONE,
+ };
+
+ co = qemu_coroutine_create(qcow2_co_open_entry);
+ qemu_coroutine_enter(co, &qo);
+ while (qo.ret == NOT_DONE) {
+ qemu_aio_wait();
+ }
+ return qo.ret;
+}
+
static int qcow2_set_key(BlockDriverState *bs, const char *key)
{
BDRVQcowState *s = bs->opaque;
@@ -1201,7 +1237,7 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
return qcow2_update_header(bs);
}
-static int preallocate(BlockDriverState *bs)
+static int coroutine_fn preallocate(BlockDriverState *bs)
{
uint64_t nb_sectors;
uint64_t offset;
@@ -1257,7 +1293,7 @@ static int preallocate(BlockDriverState *bs)
return 0;
}
-static int qcow2_create2(const char *filename, int64_t total_size,
+static int coroutine_fn qcow2_create2(const char *filename, int64_t total_size,
const char *backing_file, const char *backing_format,
int flags, size_t cluster_size, int prealloc,
QEMUOptionParameter *options, int version)
@@ -1394,7 +1430,7 @@ out:
return ret;
}
-static int qcow2_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn qcow2_co_create(const char *filename, QEMUOptionParameter *options)
{
const char *backing_file = NULL;
const char *backing_fmt = NULL;
@@ -1781,10 +1817,10 @@ static BlockDriver bdrv_qcow2 = {
.format_name = "qcow2",
.instance_size = sizeof(BDRVQcowState),
.bdrv_probe = qcow2_probe,
- .bdrv_open = qcow2_open,
+ .bdrv_co_open = qcow2_co_open,
.bdrv_close = qcow2_close,
.bdrv_reopen_prepare = qcow2_reopen_prepare,
- .bdrv_create = qcow2_create,
+ .bdrv_co_create = qcow2_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_is_allocated = qcow2_co_is_allocated,
.bdrv_set_key = qcow2_set_key,
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..0b482ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -397,13 +397,13 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
int *num, uint64_t *cluster_offset);
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
+int coroutine_fn qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size);
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
+int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
int nb_sectors);
int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
diff --git a/block/qed.c b/block/qed.c
index f767b05..5f4ba79 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -373,7 +373,7 @@ static void bdrv_qed_rebind(BlockDriverState *bs)
s->bs = bs;
}
-static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn bdrv_qed_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVQEDState *s = bs->opaque;
QEDHeader le_header;
@@ -603,7 +603,7 @@ out:
return ret;
}
-static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn bdrv_qed_co_create(const char *filename, QEMUOptionParameter *options)
{
uint64_t image_size = 0;
uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
@@ -1570,10 +1570,10 @@ static BlockDriver bdrv_qed = {
.bdrv_probe = bdrv_qed_probe,
.bdrv_rebind = bdrv_qed_rebind,
- .bdrv_open = bdrv_qed_open,
+ .bdrv_co_open = bdrv_qed_co_open,
.bdrv_close = bdrv_qed_close,
.bdrv_reopen_prepare = bdrv_qed_reopen_prepare,
- .bdrv_create = bdrv_qed_create,
+ .bdrv_co_create = bdrv_qed_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_is_allocated = bdrv_qed_co_is_allocated,
.bdrv_make_empty = bdrv_qed_make_empty,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ba721d3..e9f0890 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -335,7 +335,7 @@ fail:
return ret;
}
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn raw_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVRawState *s = bs->opaque;
@@ -1040,7 +1040,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
return (int64_t)st.st_blocks * 512;
}
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn raw_co_create(const char *filename, QEMUOptionParameter *options)
{
int fd;
int result = 0;
@@ -1193,12 +1193,12 @@ static BlockDriver bdrv_file = {
.protocol_name = "file",
.instance_size = sizeof(BDRVRawState),
.bdrv_probe = NULL, /* no probe for protocols */
- .bdrv_file_open = raw_open,
+ .bdrv_co_file_open = raw_co_open,
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
.bdrv_close = raw_close,
- .bdrv_create = raw_create,
+ .bdrv_co_create = raw_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_is_allocated = raw_co_is_allocated,
@@ -1325,7 +1325,7 @@ static int check_hdev_writable(BDRVRawState *s)
return 0;
}
-static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn hdev_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVRawState *s = bs->opaque;
int ret;
@@ -1498,7 +1498,7 @@ static coroutine_fn BlockDriverAIOCB *hdev_aio_discard(BlockDriverState *bs,
cb, opaque, QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
}
-static int hdev_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn hdev_co_create(const char *filename, QEMUOptionParameter *options)
{
int fd;
int ret = 0;
@@ -1533,12 +1533,12 @@ static BlockDriver bdrv_host_device = {
.protocol_name = "host_device",
.instance_size = sizeof(BDRVRawState),
.bdrv_probe_device = hdev_probe_device,
- .bdrv_file_open = hdev_open,
+ .bdrv_co_file_open = hdev_co_open,
.bdrv_close = raw_close,
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_create = hdev_create,
+ .bdrv_co_create = hdev_co_create,
.create_options = raw_create_options,
.bdrv_aio_readv = raw_aio_readv,
@@ -1559,7 +1559,7 @@ static BlockDriver bdrv_host_device = {
};
#ifdef __linux__
-static int floppy_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn floppy_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVRawState *s = bs->opaque;
int ret;
@@ -1657,12 +1657,12 @@ static BlockDriver bdrv_host_floppy = {
.protocol_name = "host_floppy",
.instance_size = sizeof(BDRVRawState),
.bdrv_probe_device = floppy_probe_device,
- .bdrv_file_open = floppy_open,
+ .bdrv_co_file_open = floppy_co_open,
.bdrv_close = raw_close,
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_create = hdev_create,
+ .bdrv_co_create = hdev_co_create,
.create_options = raw_create_options,
.bdrv_aio_readv = raw_aio_readv,
@@ -1680,7 +1680,7 @@ static BlockDriver bdrv_host_floppy = {
.bdrv_eject = floppy_eject,
};
-static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cdrom_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVRawState *s = bs->opaque;
@@ -1758,12 +1758,12 @@ static BlockDriver bdrv_host_cdrom = {
.protocol_name = "host_cdrom",
.instance_size = sizeof(BDRVRawState),
.bdrv_probe_device = cdrom_probe_device,
- .bdrv_file_open = cdrom_open,
+ .bdrv_co_file_open = cdrom_co_open,
.bdrv_close = raw_close,
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_create = hdev_create,
+ .bdrv_co_create = hdev_co_create,
.create_options = raw_create_options,
.bdrv_aio_readv = raw_aio_readv,
@@ -1787,7 +1787,7 @@ static BlockDriver bdrv_host_cdrom = {
#endif /* __linux__ */
#if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
-static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cdrom_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVRawState *s = bs->opaque;
int ret;
@@ -1879,12 +1879,12 @@ static BlockDriver bdrv_host_cdrom = {
.protocol_name = "host_cdrom",
.instance_size = sizeof(BDRVRawState),
.bdrv_probe_device = cdrom_probe_device,
- .bdrv_file_open = cdrom_open,
+ .bdrv_co_file_open = cdrom_co_open,
.bdrv_close = raw_close,
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_create = hdev_create,
+ .bdrv_co_create = hdev_co_create,
.create_options = raw_create_options,
.bdrv_aio_readv = raw_aio_readv,
diff --git a/block/raw.c b/block/raw.c
index ce10422..83fc9f8 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -3,7 +3,7 @@
#include "block/block_int.h"
#include "qemu/module.h"
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn raw_co_open(BlockDriverState *bs, QDict *options, int flags)
{
bs->sg = bs->file->sg;
return 0;
@@ -95,7 +95,7 @@ static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
}
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn raw_co_create(const char *filename, QEMUOptionParameter *options)
{
return bdrv_create_file(filename, options);
}
@@ -120,7 +120,7 @@ static BlockDriver bdrv_raw = {
/* It's really 0, but we need to make g_malloc() happy */
.instance_size = 1,
- .bdrv_open = raw_open,
+ .bdrv_co_open = raw_co_open,
.bdrv_close = raw_close,
.bdrv_reopen_prepare = raw_reopen_prepare,
@@ -142,7 +142,7 @@ static BlockDriver bdrv_raw = {
.bdrv_ioctl = raw_ioctl,
.bdrv_aio_ioctl = raw_aio_ioctl,
- .bdrv_create = raw_create,
+ .bdrv_co_create = raw_co_create,
.create_options = raw_create_options,
.bdrv_has_zero_init = raw_has_zero_init,
};
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b397b5b..66c7c22 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1259,7 +1259,7 @@ static QemuOptsList runtime_opts = {
},
};
-static int sd_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn sd_co_open(BlockDriverState *bs, QDict *options, int flags)
{
int ret, fd;
uint32_t vid = 0;
@@ -1454,7 +1454,7 @@ out:
return ret;
}
-static int sd_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn sd_co_create(const char *filename, QEMUOptionParameter *options)
{
int ret = 0;
uint32_t vid = 0, base_vid = 0;
@@ -2186,7 +2186,7 @@ out:
return found;
}
-static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
+static int coroutine_fn do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
int64_t pos, int size, int load)
{
bool create;
@@ -2344,9 +2344,9 @@ static BlockDriver bdrv_sheepdog = {
.format_name = "sheepdog",
.protocol_name = "sheepdog",
.instance_size = sizeof(BDRVSheepdogState),
- .bdrv_file_open = sd_open,
- .bdrv_close = sd_close,
- .bdrv_create = sd_create,
+ .bdrv_co_file_open = sd_co_open,
+ .bdrv_close = sd_close,
+ .bdrv_co_create = sd_co_create,
.bdrv_getlength = sd_getlength,
.bdrv_truncate = sd_truncate,
@@ -2371,9 +2371,9 @@ static BlockDriver bdrv_sheepdog_tcp = {
.format_name = "sheepdog",
.protocol_name = "sheepdog+tcp",
.instance_size = sizeof(BDRVSheepdogState),
- .bdrv_file_open = sd_open,
- .bdrv_close = sd_close,
- .bdrv_create = sd_create,
+ .bdrv_co_file_open = sd_co_open,
+ .bdrv_close = sd_close,
+ .bdrv_co_create = sd_co_create,
.bdrv_getlength = sd_getlength,
.bdrv_truncate = sd_truncate,
@@ -2398,9 +2398,9 @@ static BlockDriver bdrv_sheepdog_unix = {
.format_name = "sheepdog",
.protocol_name = "sheepdog+unix",
.instance_size = sizeof(BDRVSheepdogState),
- .bdrv_file_open = sd_open,
- .bdrv_close = sd_close,
- .bdrv_create = sd_create,
+ .bdrv_co_file_open = sd_co_open,
+ .bdrv_close = sd_close,
+ .bdrv_co_create = sd_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_getlength = sd_getlength,
.bdrv_truncate = sd_truncate,
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..541d83d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,6 +25,8 @@
#include "block/snapshot.h"
#include "block/block_int.h"
+#define NOT_DONE 0x7fffffff
+
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name)
{
@@ -81,6 +83,34 @@ int bdrv_snapshot_create(BlockDriverState *bs,
return -ENOTSUP;
}
+struct SnapOp {
+ BlockDriverState *bs;
+ int ret;
+};
+
+static void coroutine_fn bdrv_snapshot_open_entry(void *opaque)
+{
+ struct SnapOp *so = opaque;
+ so->ret = so->bs->drv->bdrv_co_open(so->bs, NULL, so->bs->open_flags);
+}
+
+static int bdrv_snapshot_open(BlockDriverState *bs)
+{
+ Coroutine *co;
+ struct SnapOp so = {
+ .bs = bs,
+ .ret = NOT_DONE,
+ };
+
+ co = qemu_coroutine_create(bdrv_snapshot_open_entry);
+ qemu_coroutine_enter(co, &so);
+ while (so.ret == NOT_DONE) {
+ qemu_aio_wait();
+ }
+
+ return so.ret;
+}
+
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id)
{
@@ -97,7 +127,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
if (bs->file) {
drv->bdrv_close(bs);
ret = bdrv_snapshot_goto(bs->file, snapshot_id);
- open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+ open_ret = bdrv_snapshot_open(bs);
if (open_ret < 0) {
bdrv_delete(bs->file);
bs->drv = NULL;
diff --git a/block/ssh.c b/block/ssh.c
index d7e7bf8..66ac4c1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -608,7 +608,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
return ret;
}
-static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
+static int coroutine_fn ssh_co_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
{
BDRVSSHState *s = bs->opaque;
int ret;
@@ -650,7 +650,7 @@ static QEMUOptionParameter ssh_create_options[] = {
{ NULL }
};
-static int ssh_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn ssh_co_create(const char *filename, QEMUOptionParameter *options)
{
int r, ret;
Error *local_err = NULL;
@@ -748,7 +748,7 @@ static int return_true(void *opaque)
return 1;
}
-static coroutine_fn void set_fd_handler(BDRVSSHState *s)
+static void set_fd_handler(BDRVSSHState *s)
{
int r;
IOHandler *rd_handler = NULL, *wr_handler = NULL;
@@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
}
-static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
+static void clear_fd_handler(BDRVSSHState *s)
{
DPRINTF("s->sock=%d", s->sock);
qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
@@ -888,7 +888,7 @@ static coroutine_fn int ssh_co_readv(BlockDriverState *bs,
return ret;
}
-static int ssh_write(BDRVSSHState *s,
+static int coroutine_fn ssh_write(BDRVSSHState *s,
int64_t offset, size_t size,
QEMUIOVector *qiov)
{
@@ -1049,8 +1049,8 @@ static BlockDriver bdrv_ssh = {
.protocol_name = "ssh",
.instance_size = sizeof(BDRVSSHState),
.bdrv_parse_filename = ssh_parse_filename,
- .bdrv_file_open = ssh_file_open,
- .bdrv_create = ssh_create,
+ .bdrv_co_file_open = ssh_co_file_open,
+ .bdrv_co_create = ssh_co_create,
.bdrv_close = ssh_close,
.bdrv_has_zero_init = ssh_has_zero_init,
.bdrv_co_readv = ssh_co_readv,
diff --git a/block/vdi.c b/block/vdi.c
index 8a91525..577a638 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -364,7 +364,7 @@ static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
return result;
}
-static int vdi_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vdi_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVVdiState *s = bs->opaque;
VdiHeader header;
@@ -633,7 +633,7 @@ static int vdi_co_write(BlockDriverState *bs,
return ret;
}
-static int vdi_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vdi_co_create(const char *filename, QEMUOptionParameter *options)
{
int fd;
int result = 0;
@@ -775,17 +775,17 @@ static BlockDriver bdrv_vdi = {
.format_name = "vdi",
.instance_size = sizeof(BDRVVdiState),
.bdrv_probe = vdi_probe,
- .bdrv_open = vdi_open,
+ .bdrv_co_open = vdi_co_open,
.bdrv_close = vdi_close,
.bdrv_reopen_prepare = vdi_reopen_prepare,
- .bdrv_create = vdi_create,
+ .bdrv_co_create = vdi_co_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_is_allocated = vdi_co_is_allocated,
.bdrv_make_empty = vdi_make_empty,
- .bdrv_read = vdi_co_read,
+ .bdrv_co_read = vdi_co_read,
#if defined(CONFIG_VDI_WRITE)
- .bdrv_write = vdi_co_write,
+ .bdrv_co_write = vdi_co_write,
#endif
.bdrv_get_info = vdi_get_info,
diff --git a/block/vhdx.c b/block/vhdx.c
index e9704b1..af38098 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -715,7 +715,7 @@ exit:
}
-static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vhdx_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVVHDXState *s = bs->opaque;
int ret = 0;
@@ -957,7 +957,7 @@ static BlockDriver bdrv_vhdx = {
.format_name = "vhdx",
.instance_size = sizeof(BDRVVHDXState),
.bdrv_probe = vhdx_probe,
- .bdrv_open = vhdx_open,
+ .bdrv_co_open = vhdx_co_open,
.bdrv_close = vhdx_close,
.bdrv_reopen_prepare = vhdx_reopen_prepare,
.bdrv_co_readv = vhdx_co_readv,
diff --git a/block/vmdk.c b/block/vmdk.c
index a28fb5e..ae1dee4 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -767,7 +767,7 @@ exit:
return ret;
}
-static int vmdk_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vmdk_co_open(BlockDriverState *bs, QDict *options, int flags)
{
int ret;
BDRVVmdkState *s = bs->opaque;
@@ -1487,7 +1487,7 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
return VMDK_OK;
}
-static int vmdk_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vmdk_co_create(const char *filename, QEMUOptionParameter *options)
{
int fd, idx = 0;
char desc[BUF_SIZE];
@@ -1765,13 +1765,13 @@ static BlockDriver bdrv_vmdk = {
.format_name = "vmdk",
.instance_size = sizeof(BDRVVmdkState),
.bdrv_probe = vmdk_probe,
- .bdrv_open = vmdk_open,
+ .bdrv_co_open = vmdk_co_open,
.bdrv_reopen_prepare = vmdk_reopen_prepare,
- .bdrv_read = vmdk_co_read,
- .bdrv_write = vmdk_co_write,
+ .bdrv_co_read = vmdk_co_read,
+ .bdrv_co_write = vmdk_co_write,
.bdrv_co_write_zeroes = vmdk_co_write_zeroes,
.bdrv_close = vmdk_close,
- .bdrv_create = vmdk_create,
+ .bdrv_co_create = vmdk_co_create,
.bdrv_co_flush_to_disk = vmdk_co_flush,
.bdrv_co_is_allocated = vmdk_co_is_allocated,
.bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
diff --git a/block/vpc.c b/block/vpc.c
index fe4f311..6eb293a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -155,7 +155,7 @@ static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int vpc_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vpc_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVVPCState *s = bs->opaque;
int i;
@@ -683,7 +683,7 @@ static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
return ret;
}
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vpc_co_create(const char *filename, QEMUOptionParameter *options)
{
uint8_t buf[1024];
struct vhd_footer *footer = (struct vhd_footer *) buf;
@@ -831,13 +831,13 @@ static BlockDriver bdrv_vpc = {
.instance_size = sizeof(BDRVVPCState),
.bdrv_probe = vpc_probe,
- .bdrv_open = vpc_open,
+ .bdrv_co_open = vpc_co_open,
.bdrv_close = vpc_close,
.bdrv_reopen_prepare = vpc_reopen_prepare,
- .bdrv_create = vpc_create,
+ .bdrv_co_create = vpc_co_create,
- .bdrv_read = vpc_co_read,
- .bdrv_write = vpc_co_write,
+ .bdrv_co_read = vpc_co_read,
+ .bdrv_co_write = vpc_co_write,
.create_options = vpc_create_options,
.bdrv_has_zero_init = vpc_has_zero_init,
diff --git a/block/vvfat.c b/block/vvfat.c
index 87b0279..4771a5d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1065,7 +1065,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
qdict_put(options, "rw", qbool_from_int(rw));
}
-static int vvfat_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vvfat_co_open(BlockDriverState *bs, QDict *options, int flags)
{
BDRVVVFATState *s = bs->opaque;
int cyls, heads, secs;
@@ -2886,7 +2886,7 @@ static int coroutine_fn vvfat_co_is_allocated(BlockDriverState *bs,
return 1;
}
-static int write_target_commit(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn write_target_commit(BlockDriverState *bs, int64_t sector_num,
const uint8_t* buffer, int nb_sectors) {
BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
return try_commit(s);
@@ -2900,7 +2900,7 @@ static void write_target_close(BlockDriverState *bs) {
static BlockDriver vvfat_write_target = {
.format_name = "vvfat_write_target",
- .bdrv_write = write_target_commit,
+ .bdrv_co_write = write_target_commit,
.bdrv_close = write_target_close,
};
@@ -2975,12 +2975,12 @@ static BlockDriver bdrv_vvfat = {
.instance_size = sizeof(BDRVVVFATState),
.bdrv_parse_filename = vvfat_parse_filename,
- .bdrv_file_open = vvfat_open,
+ .bdrv_co_file_open = vvfat_co_open,
.bdrv_close = vvfat_close,
.bdrv_rebind = vvfat_rebind,
- .bdrv_read = vvfat_co_read,
- .bdrv_write = vvfat_co_write,
+ .bdrv_co_read = vvfat_co_read,
+ .bdrv_co_write = vvfat_co_write,
.bdrv_co_is_allocated = vvfat_co_is_allocated,
};
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..bd92260 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -96,15 +96,15 @@ struct BlockDriver {
void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
- int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
- int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags);
- int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
+ int coroutine_fn (*bdrv_co_open)(BlockDriverState *bs, QDict *options, int flags);
+ int coroutine_fn (*bdrv_co_file_open)(BlockDriverState *bs, QDict *options, int flags);
+ int coroutine_fn (*bdrv_co_read)(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors);
- int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
+ int coroutine_fn (*bdrv_co_write)(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors);
void (*bdrv_close)(BlockDriverState *bs);
void (*bdrv_rebind)(BlockDriverState *bs);
- int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
+ int coroutine_fn (*bdrv_co_create)(const char *filename, QEMUOptionParameter *options);
int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
int (*bdrv_make_empty)(BlockDriverState *bs);
/* aio */
diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 563dcde..75bc6f8 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -189,14 +189,14 @@ void qemu_co_rwlock_init(CoRwlock *lock);
* of a parallel writer, control is transferred to the caller of the current
* coroutine.
*/
-void qemu_co_rwlock_rdlock(CoRwlock *lock);
+void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock);
/**
* Write Locks the mutex. If the lock cannot be taken immediately because
* of a parallel reader, control is transferred to the caller of the current
* coroutine.
*/
-void qemu_co_rwlock_wrlock(CoRwlock *lock);
+void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock);
/**
* Unlocks the read/write lock and schedules the next coroutine that was
diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
index f133d65..d0ab27d 100644
--- a/include/block/coroutine_int.h
+++ b/include/block/coroutine_int.h
@@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
void qemu_coroutine_delete(Coroutine *co);
CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
CoroutineAction action);
-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
+void qemu_co_queue_run_restart(Coroutine *co);
#endif
diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
index d9fea49..200bfd2 100644
--- a/qemu-coroutine-lock.c
+++ b/qemu-coroutine-lock.c
@@ -145,7 +145,7 @@ void qemu_co_rwlock_init(CoRwlock *lock)
qemu_co_queue_init(&lock->queue);
}
-void qemu_co_rwlock_rdlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock)
{
while (lock->writer) {
qemu_co_queue_wait(&lock->queue);
@@ -169,7 +169,7 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
}
}
-void qemu_co_rwlock_wrlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock)
{
while (lock->writer || lock->reader) {
qemu_co_queue_wait(&lock->queue);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (2 preceding siblings ...)
2013-08-05 18:44 ` [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations Charlie Shepherd
@ 2013-08-05 18:44 ` Charlie Shepherd
2013-08-05 20:01 ` Gabriel Kerneis
2013-08-06 9:36 ` Kevin Wolf
2013-08-05 18:44 ` [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations Charlie Shepherd
` (3 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, gabriel, Charlie Shepherd, stefanha
This patch follows on from the previous one and converts some block layer functions to be
explicitly annotated with coroutine_fn instead of yielding depending upon calling context.
---
block.c | 235 ++++++++++++++++++++++++++------------------------
block/mirror.c | 4 +-
include/block/block.h | 37 ++++----
3 files changed, 148 insertions(+), 128 deletions(-)
diff --git a/block.c b/block.c
index aaa122c..e7011f9 100644
--- a/block.c
+++ b/block.c
@@ -164,7 +164,7 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs)
|| io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
}
-static void bdrv_io_limits_intercept(BlockDriverState *bs,
+static void coroutine_fn bdrv_io_limits_intercept(BlockDriverState *bs,
bool is_write, int nb_sectors)
{
int64_t wait_time = -1;
@@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
typedef struct CreateCo {
BlockDriver *drv;
- char *filename;
+ const char *filename;
QEMUOptionParameter *options;
int ret;
} CreateCo;
@@ -372,48 +372,48 @@ typedef struct CreateCo {
static void coroutine_fn bdrv_create_co_entry(void *opaque)
{
CreateCo *cco = opaque;
- assert(cco->drv);
-
- cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
+ cco->ret = bdrv_create(cco->drv, cco->filename, cco->options);
}
-int bdrv_create(BlockDriver *drv, const char* filename,
+int coroutine_fn bdrv_create(BlockDriver *drv, const char* filename,
QEMUOptionParameter *options)
{
int ret;
+ char *dup_fn;
+
+ assert(drv);
+ if (!drv->bdrv_co_create) {
+ return -ENOTSUP;
+ }
+ dup_fn = g_strdup(filename);
+ ret = drv->bdrv_co_create(dup_fn, options);
+ g_free(dup_fn);
+ return ret;
+}
+
+
+int bdrv_create_sync(BlockDriver *drv, const char* filename,
+ QEMUOptionParameter *options)
+{
Coroutine *co;
CreateCo cco = {
.drv = drv,
- .filename = g_strdup(filename),
+ .filename = filename,
.options = options,
.ret = NOT_DONE,
};
- if (!drv->bdrv_co_create) {
- ret = -ENOTSUP;
- goto out;
- }
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_create_co_entry(&cco);
- } else {
- co = qemu_coroutine_create(bdrv_create_co_entry);
- qemu_coroutine_enter(co, &cco);
- while (cco.ret == NOT_DONE) {
- qemu_aio_wait();
- }
+ co = qemu_coroutine_create(bdrv_create_co_entry);
+ qemu_coroutine_enter(co, &cco);
+ while (cco.ret == NOT_DONE) {
+ qemu_aio_wait();
}
- ret = cco.ret;
-
-out:
- g_free(cco.filename);
- return ret;
+ return cco.ret;
}
-int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
+int coroutine_fn bdrv_create_file(const char* filename, QEMUOptionParameter *options)
{
BlockDriver *drv;
@@ -522,7 +522,7 @@ BlockDriver *bdrv_find_protocol(const char *filename)
return NULL;
}
-static int find_image_format(BlockDriverState *bs, const char *filename,
+static int coroutine_fn find_image_format(BlockDriverState *bs, const char *filename,
BlockDriver **pdrv)
{
int score, score_max;
@@ -676,8 +676,8 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
*
* Removes all processed options from *options.
*/
-static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
- QDict *options, int flags, BlockDriver *drv)
+static int coroutine_fn bdrv_open_common(BlockDriverState *bs,
+ BlockDriverState *file, QDict *options, int flags, BlockDriver *drv)
{
int ret, open_flags;
const char *filename;
@@ -778,7 +778,7 @@ free_and_fail:
* after the call (even on failure), so if the caller intends to reuse the
* dictionary, it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDict *options, int flags)
{
BlockDriverState *bs;
@@ -881,7 +881,7 @@ fail:
* function (even on failure), so if the caller intends to reuse the dictionary,
* it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
+int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
{
char backing_filename[PATH_MAX];
int back_flags, ret;
@@ -955,7 +955,7 @@ static void extract_subqdict(QDict *src, QDict **dst, const char *start)
* after the call (even on failure), so if the caller intends to reuse the
* dictionary, it needs to use QINCREF() before calling bdrv_open.
*/
-int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
+int coroutine_fn bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
int flags, BlockDriver *drv)
{
int ret;
@@ -1279,7 +1279,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
}
- ret = bdrv_flush(reopen_state->bs);
+ ret = bdrv_flush_sync(reopen_state->bs);
if (ret) {
error_set(errp, ERROR_CLASS_GENERIC_ERROR, "Error (%s) flushing drive",
strerror(-ret));
@@ -1358,7 +1358,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
void bdrv_close(BlockDriverState *bs)
{
- bdrv_flush(bs);
+ bdrv_flush_sync(bs);
if (bs->job) {
block_job_cancel_sync(bs->job);
}
@@ -1754,7 +1754,7 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
#define COMMIT_BUF_SECTORS 2048
/* commit COW file into the raw image */
-int bdrv_commit(BlockDriverState *bs)
+int coroutine_fn bdrv_commit(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
int64_t sector, total_sectors;
@@ -1805,7 +1805,7 @@ int bdrv_commit(BlockDriverState *bs)
if (drv->bdrv_make_empty) {
ret = drv->bdrv_make_empty(bs);
- bdrv_flush(bs);
+ bdrv_flush_sync(bs);
}
/*
@@ -1813,7 +1813,7 @@ int bdrv_commit(BlockDriverState *bs)
* stable on disk.
*/
if (bs->backing_hd)
- bdrv_flush(bs->backing_hd);
+ bdrv_flush_sync(bs->backing_hd);
ro_cleanup:
g_free(buf);
@@ -1826,7 +1826,7 @@ ro_cleanup:
return ret;
}
-int bdrv_commit_all(void)
+int coroutine_fn bdrv_commit_all(void)
{
BlockDriverState *bs;
@@ -2156,35 +2156,25 @@ typedef struct RwCo {
int ret;
} RwCo;
-static void coroutine_fn bdrv_rw_co_entry(void *opaque)
+static int bdrv_sync_rwco(void coroutine_fn (*co_fn)(void *), RwCo *rwco)
{
- RwCo *rwco = opaque;
-
- if (!rwco->is_write) {
- rwco->ret = bdrv_co_do_readv(rwco->bs, rwco->sector_num,
- rwco->nb_sectors, rwco->qiov, 0);
- } else {
- rwco->ret = bdrv_co_do_writev(rwco->bs, rwco->sector_num,
- rwco->nb_sectors, rwco->qiov, 0);
+ Coroutine *co;
+ co = qemu_coroutine_create(co_fn);
+ qemu_coroutine_enter(co, rwco);
+ while (rwco->ret == NOT_DONE) {
+ qemu_aio_wait();
}
+ return rwco->ret;
}
/*
* Process a vectored synchronous request using coroutines
*/
-static int bdrv_rwv_co(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn bdrv_rwv_co(BlockDriverState *bs, int64_t sector_num,
QEMUIOVector *qiov, bool is_write)
{
- Coroutine *co;
- RwCo rwco = {
- .bs = bs,
- .sector_num = sector_num,
- .nb_sectors = qiov->size >> BDRV_SECTOR_BITS,
- .qiov = qiov,
- .is_write = is_write,
- .ret = NOT_DONE,
- };
assert((qiov->size & (BDRV_SECTOR_SIZE - 1)) == 0);
+ int nb_sectors = qiov->size >> BDRV_SECTOR_BITS;
/**
* In sync call context, when the vcpu is blocked, this throttling timer
@@ -2197,23 +2187,40 @@ static int bdrv_rwv_co(BlockDriverState *bs, int64_t sector_num,
bdrv_io_limits_disable(bs);
}
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_rw_co_entry(&rwco);
+ if (!is_write) {
+ return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
} else {
- co = qemu_coroutine_create(bdrv_rw_co_entry);
- qemu_coroutine_enter(co, &rwco);
- while (rwco.ret == NOT_DONE) {
- qemu_aio_wait();
- }
+ return bdrv_co_do_writev(bs, sector_num, nb_sectors, qiov, 0);
}
- return rwco.ret;
+}
+
+static void coroutine_fn bdrv_rw_co_entry(void *opaque)
+{
+ RwCo *rwco = opaque;
+ rwco->ret = bdrv_rwv_co_co(rwco->bs, rwco->sector_num, rwco->qiov, rwco->is_write);
+}
+
+/*
+ * Process a vectored synchronous request synchronously
+ */
+static int bdrv_rwv_sync(BlockDriverState *bs, int64_t sector_num,
+ QEMUIOVector *qiov, bool is_write)
+{
+ RwCo rwco = {
+ .bs = bs,
+ .sector_num = sector_num,
+ .qiov = qiov,
+ .is_write = is_write,
+ .ret = NOT_DONE,
+ };
+
+ return bdrv_sync_rwco(bdrv_rw_co_entry, &rwco);
}
/*
* Process a synchronous request using coroutines
*/
-static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
+static int coroutine_fn bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
int nb_sectors, bool is_write)
{
QEMUIOVector qiov;
@@ -2226,15 +2233,37 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
return bdrv_rwv_co(bs, sector_num, &qiov, is_write);
}
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BlockDriverState *bs, int64_t sector_num,
+/*
+ * Process a synchronous request using coroutines
+ */
+static int bdrv_rw_sync(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
+ int nb_sectors, bool is_write)
+{
+ QEMUIOVector qiov;
+ struct iovec iov = {
+ .iov_base = (void *)buf,
+ .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
+ };
+
+ qemu_iovec_init_external(&qiov, &iov, 1);
+ return bdrv_rwv_sync(bs, sector_num, &qiov, is_write);
+}
+
+int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false);
}
+/* return < 0 if error. See bdrv_write() for the return codes */
+int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ return bdrv_rw_sync(bs, sector_num, buf, nb_sectors, false);
+}
+
/* Just like bdrv_read(), but with I/O throttling temporarily disabled */
-int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
bool enabled;
@@ -2253,18 +2282,24 @@ int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
-EINVAL Invalid sector number or nb_sectors
-EACCES Trying to write a read-only device
*/
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num,
+ const uint8_t *buf, int nb_sectors)
+{
+ return bdrv_rw_co_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true);
+}
+
+int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true);
}
-int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov)
+int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov)
{
return bdrv_rwv_co(bs, sector_num, qiov, true);
}
-int bdrv_pread(BlockDriverState *bs, int64_t offset,
+int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset,
void *buf, int count1)
{
uint8_t tmp_buf[BDRV_SECTOR_SIZE];
@@ -2309,7 +2344,7 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
return count1;
}
-int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
+int coroutine_fn bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
{
uint8_t tmp_buf[BDRV_SECTOR_SIZE];
int len, nb_sectors, count;
@@ -2366,7 +2401,7 @@ int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
+int coroutine_fn bdrv_pwrite(BlockDriverState *bs, int64_t offset,
const void *buf, int count1)
{
QEMUIOVector qiov;
@@ -2385,7 +2420,7 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
*
* Returns 0 on success, -errno in error cases.
*/
-int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
+int coroutine_fn bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
const void *buf, int count)
{
int ret;
@@ -2397,7 +2432,7 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
/* No flush needed for cache modes that already do it */
if (bs->enable_write_cache) {
- bdrv_flush(bs);
+ bdrv_flush_sync(bs);
}
return 0;
@@ -2907,7 +2942,7 @@ void bdrv_flush_all(void)
BlockDriverState *bs;
QTAILQ_FOREACH(bs, &bdrv_states, list) {
- bdrv_flush(bs);
+ bdrv_flush_sync(bs);
}
}
@@ -3740,7 +3775,7 @@ static void bdrv_aio_bh_cb(void *opaque)
qemu_aio_release(acb);
}
-static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
+static coroutine_fn BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num,
QEMUIOVector *qiov,
int nb_sectors,
@@ -3769,14 +3804,14 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
return &acb->common;
}
-static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
+static coroutine_fn BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
BlockDriverCompletionFunc *cb, void *opaque)
{
return bdrv_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
}
-static BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
+static coroutine_fn BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
BlockDriverCompletionFunc *cb, void *opaque)
{
@@ -4014,7 +4049,7 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque)
rwco->ret = bdrv_co_flush(rwco->bs);
}
-int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
+int coroutine_fn bdrv_flush(BlockDriverState *bs)
{
int ret;
@@ -4102,7 +4137,7 @@ void bdrv_clear_incoming_migration_all(void)
}
}
-int bdrv_flush(BlockDriverState *bs)
+int bdrv_flush_sync(BlockDriverState *bs)
{
Coroutine *co;
RwCo rwco = {
@@ -4110,18 +4145,7 @@ int bdrv_flush(BlockDriverState *bs)
.ret = NOT_DONE,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_flush_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_flush_co_entry);
- qemu_coroutine_enter(co, &rwco);
- while (rwco.ret == NOT_DONE) {
- qemu_aio_wait();
- }
- }
-
- return rwco.ret;
+ return bdrv_sync_rwco(bdrv_flush_co_entry, &rwco);
}
static void coroutine_fn bdrv_discard_co_entry(void *opaque)
@@ -4131,7 +4155,7 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
}
-int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_discard(BlockDriverState *bs, int64_t sector_num,
int nb_sectors)
{
if (!bs->drv) {
@@ -4172,7 +4196,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
}
}
-int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
+int bdrv_discard_sync(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
{
Coroutine *co;
RwCo rwco = {
@@ -4182,18 +4206,7 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
.ret = NOT_DONE,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_discard_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_discard_co_entry);
- qemu_coroutine_enter(co, &rwco);
- while (rwco.ret == NOT_DONE) {
- qemu_aio_wait();
- }
- }
-
- return rwco.ret;
+ return bdrv_sync_rwco(bdrv_discard_co_entry, &rwco);
}
/**************************************************************/
@@ -4432,7 +4445,7 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
}
-void bdrv_img_create(const char *filename, const char *fmt,
+void coroutine_fn bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
char *options, uint64_t img_size, int flags,
Error **errp, bool quiet)
diff --git a/block/mirror.c b/block/mirror.c
index bed4a7e..3d5da7e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -282,7 +282,7 @@ static void mirror_free_init(MirrorBlockJob *s)
}
}
-static void mirror_drain(MirrorBlockJob *s)
+static void coroutine_fn mirror_drain(MirrorBlockJob *s)
{
while (s->in_flight > 0) {
qemu_coroutine_yield();
@@ -390,7 +390,7 @@ static void coroutine_fn mirror_run(void *opaque)
should_complete = false;
if (s->in_flight == 0 && cnt == 0) {
trace_mirror_before_flush(s);
- ret = bdrv_flush(s->target);
+ ret = bdrv_co_flush(s->target);
if (ret < 0) {
if (mirror_error_action(s, false, -ret) == BDRV_ACTION_REPORT) {
goto immediate_exit;
diff --git a/include/block/block.h b/include/block/block.h
index dd8eca1..57d8ba2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -125,9 +125,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
void bdrv_delete(BlockDriverState *bs);
int bdrv_parse_cache_flags(const char *mode, int *flags);
int bdrv_parse_discard_flags(const char *mode, int *flags);
-int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDict *options, int flags);
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
+int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
int flags, BlockDriver *drv);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
@@ -150,18 +150,24 @@ void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
bool bdrv_dev_has_removable_media(BlockDriverState *bs);
bool bdrv_dev_is_tray_open(BlockDriverState *bs);
bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
-int bdrv_read(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors);
-int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
+int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors);
+int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors);
-int bdrv_write(BlockDriverState *bs, int64_t sector_num,
+int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num,
+ const uint8_t *buf, int nb_sectors);
+int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors);
-int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
-int bdrv_pread(BlockDriverState *bs, int64_t offset,
+int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
+int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset,
+ void *buf, int count);
+int bdrv_pread_sync(BlockDriverState *bs, int64_t offset,
void *buf, int count);
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
+int coroutine_fn bdrv_pwrite(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
-int bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
+int coroutine_fn bdrv_pwritev(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov);
int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -191,8 +197,8 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset);
int64_t bdrv_getlength(BlockDriverState *bs);
int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
-int bdrv_commit(BlockDriverState *bs);
-int bdrv_commit_all(void);
+int coroutine_fn bdrv_commit(BlockDriverState *bs);
+int coroutine_fn bdrv_commit_all(void);
int bdrv_change_backing_file(BlockDriverState *bs,
const char *backing_file, const char *backing_fmt);
void bdrv_register(BlockDriver *bdrv);
@@ -264,13 +270,14 @@ void bdrv_invalidate_cache_all(void);
void bdrv_clear_incoming_migration_all(void);
/* Ensure contents are flushed to disk. */
-int bdrv_flush(BlockDriverState *bs);
-int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
+int coroutine_fn bdrv_flush(BlockDriverState *bs);
+int bdrv_flush_sync(BlockDriverState *bs);
void bdrv_flush_all(void);
void bdrv_close_all(void);
void bdrv_drain_all(void);
-int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
+int coroutine_fn bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
+int bdrv_discard_sync(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
int bdrv_has_zero_init_1(BlockDriverState *bs);
int bdrv_has_zero_init(BlockDriverState *bs);
@@ -333,7 +340,7 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
int64_t pos, int size);
-void bdrv_img_create(const char *filename, const char *fmt,
+void coroutine_fn bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
char *options, uint64_t img_size, int flags,
Error **errp, bool quiet);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (3 preceding siblings ...)
2013-08-05 18:44 ` [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions Charlie Shepherd
@ 2013-08-05 18:44 ` Charlie Shepherd
2013-08-05 20:15 ` Gabriel Kerneis
2013-08-05 19:25 ` [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (2 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 18:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, gabriel, Charlie Shepherd, stefanha
This patch updates the callers of block layer functions converted to explicit
coroutine_fn annotation in the previous patch.
---
block/bochs.c | 4 ++--
block/cloop.c | 4 ++--
block/cow.c | 8 +++----
block/dmg.c | 8 +++----
block/qcow.c | 6 +++---
block/qcow2.c | 18 ++++++++--------
block/qcow2.h | 2 +-
block/vdi.c | 4 ++--
block/vmdk.c | 34 +++++++++++++++---------------
block/vpc.c | 4 ++--
block/vvfat.c | 20 +++++++++---------
blockjob.c | 2 +-
include/block/blockjob.h | 2 +-
nbd.c | 4 ++--
qemu-img.c | 54 ++++++++++++++++++++++++++++++++++++++----------
qemu-io-cmds.c | 12 +++++------
16 files changed, 109 insertions(+), 77 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index c827bd4..a64cfa3 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -165,7 +165,7 @@ fail:
return ret;
}
-static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
+static int64_t coroutine_fn seek_to_sector(BlockDriverState *bs, int64_t sector_num)
{
BDRVBochsState *s = bs->opaque;
int64_t offset = sector_num * 512;
@@ -196,7 +196,7 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
return bitmap_offset + (512 * (s->bitmap_blocks + extent_offset));
}
-static int bochs_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn bochs_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
int ret;
diff --git a/block/cloop.c b/block/cloop.c
index ef5555f..b9dee5f 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -114,7 +114,7 @@ fail:
return ret;
}
-static inline int cloop_read_block(BlockDriverState *bs, int block_num)
+static inline int coroutine_fn cloop_read_block(BlockDriverState *bs, int block_num)
{
BDRVCloopState *s = bs->opaque;
@@ -146,7 +146,7 @@ static inline int cloop_read_block(BlockDriverState *bs, int block_num)
return 0;
}
-static int cloop_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn cloop_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
BDRVCloopState *s = bs->opaque;
diff --git a/block/cow.c b/block/cow.c
index c68c5ae..e63b154 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -106,7 +106,7 @@ static int coroutine_fn cow_co_open(BlockDriverState *bs, QDict *options, int fl
* XXX(hch): right now these functions are extremely inefficient.
* We should just read the whole bitmap we'll need in one go instead.
*/
-static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
+static inline int coroutine_fn cow_set_bit(BlockDriverState *bs, int64_t bitnum)
{
uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
uint8_t bitmap;
@@ -126,7 +126,7 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
return 0;
}
-static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
+static inline int coroutine_fn is_bit_set(BlockDriverState *bs, int64_t bitnum)
{
uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
uint8_t bitmap;
@@ -166,7 +166,7 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
return changed;
}
-static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
int nb_sectors)
{
int error = 0;
@@ -225,7 +225,7 @@ static coroutine_fn int cow_co_read(BlockDriverState *bs, int64_t sector_num,
return ret;
}
-static int cow_write(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn cow_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
BDRVCowState *s = bs->opaque;
diff --git a/block/dmg.c b/block/dmg.c
index 346aa7d..3933f45 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -64,7 +64,7 @@ static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
+static int coroutine_fn read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
{
uint64_t buffer;
int ret;
@@ -78,7 +78,7 @@ static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
return 0;
}
-static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
+static int coroutine_fn read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
{
uint32_t buffer;
int ret;
@@ -276,7 +276,7 @@ static inline uint32_t search_chunk(BDRVDMGState* s,int sector_num)
return s->n_chunks; /* error */
}
-static inline int dmg_read_chunk(BlockDriverState *bs, int sector_num)
+static inline int coroutine_fn dmg_read_chunk(BlockDriverState *bs, int sector_num)
{
BDRVDMGState *s = bs->opaque;
@@ -332,7 +332,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, int sector_num)
return 0;
}
-static int dmg_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn dmg_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
BDRVDMGState *s = bs->opaque;
diff --git a/block/qcow.c b/block/qcow.c
index 04f59f2..09e631c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -78,7 +78,7 @@ typedef struct BDRVQcowState {
Error *migration_blocker;
} BDRVQcowState;
-static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
+static int coroutine_fn decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
{
@@ -268,7 +268,7 @@ static void encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
*
* return 0 if not allocated.
*/
-static uint64_t get_cluster_offset(BlockDriverState *bs,
+static uint64_t coroutine_fn get_cluster_offset(BlockDriverState *bs,
uint64_t offset, int allocate,
int compressed_size,
int n_start, int n_end)
@@ -440,7 +440,7 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
return 0;
}
-static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
+static int coroutine_fn decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
{
BDRVQcowState *s = bs->opaque;
int ret, csize;
diff --git a/block/qcow2.c b/block/qcow2.c
index 2ed0bb6..9b359a4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -82,7 +82,7 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
* unknown magic is skipped (future extension this version knows nothing about)
* return 0 upon success, non-0 otherwise
*/
-static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
+static int coroutine_fn qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
uint64_t end_offset, void **p_feature_table)
{
BDRVQcowState *s = bs->opaque;
@@ -227,7 +227,7 @@ static void report_unsupported_feature(BlockDriverState *bs,
* updated successfully. Therefore it is not required to check the return
* value of this function.
*/
-int qcow2_mark_dirty(BlockDriverState *bs)
+int coroutine_fn qcow2_mark_dirty(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
uint64_t val;
@@ -260,7 +260,7 @@ int qcow2_mark_dirty(BlockDriverState *bs)
* function when there are no pending requests, it does not guard against
* concurrent requests dirtying the image.
*/
-static int qcow2_mark_clean(BlockDriverState *bs)
+static int coroutine_fn qcow2_mark_clean(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
@@ -276,7 +276,7 @@ static int qcow2_mark_clean(BlockDriverState *bs)
return 0;
}
-static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
+static int coroutine_fn qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
BdrvCheckMode fix)
{
int ret = qcow2_check_refcounts(bs, result, fix);
@@ -1001,7 +1001,7 @@ static void qcow2_close(BlockDriverState *bs)
qcow2_free_snapshots(bs);
}
-static void qcow2_invalidate_cache(BlockDriverState *bs)
+static void coroutine_fn qcow2_invalidate_cache(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
int flags = s->flags;
@@ -1592,7 +1592,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
/* XXX: put compressed sectors first, then all the cluster aligned
tables to avoid losing bytes in alignment */
-static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
BDRVQcowState *s = bs->opaque;
@@ -1738,7 +1738,7 @@ static void dump_refcounts(BlockDriverState *bs)
}
#endif
-static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+static int coroutine_fn qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
int64_t pos)
{
BDRVQcowState *s = bs->opaque;
@@ -1747,13 +1747,13 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
bs->growable = 1;
- ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
+ ret = bdrv_pwritev_sync(bs, qcow2_vm_state_offset(s) + pos, qiov);
bs->growable = growable;
return ret;
}
-static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
+static int coroutine_fn qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
int64_t pos, int size)
{
BDRVQcowState *s = bs->opaque;
diff --git a/block/qcow2.h b/block/qcow2.h
index 0b482ff..e195db0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -360,7 +360,7 @@ static inline uint64_t l2meta_cow_end(QCowL2Meta *m)
int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
int64_t sector_num, int nb_sectors);
-int qcow2_mark_dirty(BlockDriverState *bs);
+int coroutine_fn qcow2_mark_dirty(BlockDriverState *bs);
int qcow2_update_header(BlockDriverState *bs);
/* qcow2-refcount.c functions */
diff --git a/block/vdi.c b/block/vdi.c
index 577a638..6f1a52c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -487,7 +487,7 @@ static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
return VDI_IS_ALLOCATED(bmap_entry);
}
-static int vdi_co_read(BlockDriverState *bs,
+static int coroutine_fn vdi_co_read(BlockDriverState *bs,
int64_t sector_num, uint8_t *buf, int nb_sectors)
{
BDRVVdiState *s = bs->opaque;
@@ -532,7 +532,7 @@ static int vdi_co_read(BlockDriverState *bs,
return ret;
}
-static int vdi_co_write(BlockDriverState *bs,
+static int coroutine_fn vdi_co_write(BlockDriverState *bs,
int64_t sector_num, const uint8_t *buf, int nb_sectors)
{
BDRVVdiState *s = bs->opaque;
diff --git a/block/vmdk.c b/block/vmdk.c
index ae1dee4..178c21a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -232,7 +232,7 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
s->extents = g_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
}
-static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
+static uint32_t coroutine_fn vmdk_read_cid(BlockDriverState *bs, int parent)
{
char desc[DESC_SIZE];
uint32_t cid = 0xffffffff;
@@ -264,7 +264,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
return cid;
}
-static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
+static int coroutine_fn vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
{
char desc[DESC_SIZE], tmp_desc[DESC_SIZE];
char *p_name, *tmp_str;
@@ -298,7 +298,7 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
return 0;
}
-static int vmdk_is_cid_valid(BlockDriverState *bs)
+static int coroutine_fn vmdk_is_cid_valid(BlockDriverState *bs)
{
#ifdef CHECK_CID
BDRVVmdkState *s = bs->opaque;
@@ -351,7 +351,7 @@ exit:
return ret;
}
-static int vmdk_parent_open(BlockDriverState *bs)
+static int coroutine_fn vmdk_parent_open(BlockDriverState *bs)
{
char *p_name;
char desc[DESC_SIZE + 1];
@@ -419,7 +419,7 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
return extent;
}
-static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
+static int coroutine_fn vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
{
int ret;
int l1_size, i;
@@ -462,7 +462,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
return ret;
}
-static int vmdk_open_vmdk3(BlockDriverState *bs,
+static int coroutine_fn vmdk_open_vmdk3(BlockDriverState *bs,
BlockDriverState *file,
int flags)
{
@@ -492,7 +492,7 @@ static int vmdk_open_vmdk3(BlockDriverState *bs,
static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
int64_t desc_offset);
-static int vmdk_open_vmdk4(BlockDriverState *bs,
+static int coroutine_fn vmdk_open_vmdk4(BlockDriverState *bs,
BlockDriverState *file,
int flags)
{
@@ -628,7 +628,7 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
}
/* Open an extent file and append to bs array */
-static int vmdk_open_sparse(BlockDriverState *bs,
+static int coroutine_fn vmdk_open_sparse(BlockDriverState *bs,
BlockDriverState *file,
int flags)
{
@@ -652,7 +652,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
}
}
-static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
+static int coroutine_fn vmdk_parse_extents(const char *desc, BlockDriverState *bs,
const char *desc_file_path)
{
int ret;
@@ -727,7 +727,7 @@ next_line:
return 0;
}
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
+static int coroutine_fn vmdk_open_desc_file(BlockDriverState *bs, int flags,
int64_t desc_offset)
{
int ret;
@@ -801,7 +801,7 @@ fail:
return ret;
}
-static int get_whole_cluster(BlockDriverState *bs,
+static int coroutine_fn get_whole_cluster(BlockDriverState *bs,
VmdkExtent *extent,
uint64_t cluster_offset,
uint64_t offset,
@@ -868,7 +868,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
return VMDK_OK;
}
-static int get_cluster_offset(BlockDriverState *bs,
+static int coroutine_fn get_cluster_offset(BlockDriverState *bs,
VmdkExtent *extent,
VmdkMetaData *m_data,
uint64_t offset,
@@ -1026,7 +1026,7 @@ static int coroutine_fn vmdk_co_is_allocated(BlockDriverState *bs,
return ret;
}
-static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
+static int coroutine_fn vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
int64_t offset_in_cluster, const uint8_t *buf,
int nb_sectors, int64_t sector_num)
{
@@ -1067,7 +1067,7 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
return ret;
}
-static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
+static int coroutine_fn vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
int64_t offset_in_cluster, uint8_t *buf,
int nb_sectors)
{
@@ -1133,7 +1133,7 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
return ret;
}
-static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vmdk_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
BDRVVmdkState *s = bs->opaque;
@@ -1205,7 +1205,7 @@ static coroutine_fn int vmdk_co_read(BlockDriverState *bs, int64_t sector_num,
*
* Returns: error code with 0 for success.
*/
-static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vmdk_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors,
bool zeroed, bool zero_dry_run)
{
@@ -1692,7 +1692,7 @@ static coroutine_fn int vmdk_co_flush(BlockDriverState *bs)
int ret = 0;
for (i = 0; i < s->num_extents; i++) {
- err = bdrv_co_flush(s->extents[i].file);
+ err = bdrv_flush(s->extents[i].file);
if (err < 0) {
ret = err;
}
diff --git a/block/vpc.c b/block/vpc.c
index 6eb293a..0c94003 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -438,7 +438,7 @@ fail:
return -1;
}
-static int vpc_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vpc_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
BDRVVPCState *s = bs->opaque;
@@ -487,7 +487,7 @@ static coroutine_fn int vpc_co_read(BlockDriverState *bs, int64_t sector_num,
return ret;
}
-static int vpc_write(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vpc_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
BDRVVPCState *s = bs->opaque;
diff --git a/block/vvfat.c b/block/vvfat.c
index 4771a5d..91f8bec 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1368,7 +1368,7 @@ static void print_mapping(const mapping_t* mapping)
}
#endif
-static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vvfat_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
BDRVVVFATState *s = bs->opaque;
@@ -1677,7 +1677,7 @@ typedef enum {
* Further, the files/directories handled by this function are
* assumed to be *not* deleted (and *only* those).
*/
-static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
+static uint32_t coroutine_fn get_cluster_count_for_direntry(BDRVVVFATState* s,
direntry_t* direntry, const char* path)
{
/*
@@ -1824,7 +1824,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
* It returns 0 upon inconsistency or error, and the number of clusters
* used by the directory, its subdirectories and their files.
*/
-static int check_directory_consistency(BDRVVVFATState *s,
+static int coroutine_fn check_directory_consistency(BDRVVVFATState *s,
int cluster_num, const char* path)
{
int ret = 0;
@@ -1948,7 +1948,7 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
}
/* returns 1 on success */
-static int is_consistent(BDRVVVFATState* s)
+static int coroutine_fn is_consistent(BDRVVVFATState* s)
{
int i, check;
int used_clusters_count = 0;
@@ -2224,7 +2224,7 @@ static int commit_mappings(BDRVVVFATState* s,
return 0;
}
-static int commit_direntries(BDRVVVFATState* s,
+static int coroutine_fn commit_direntries(BDRVVVFATState* s,
int dir_index, int parent_mapping_index)
{
direntry_t* direntry = array_get(&(s->directory), dir_index);
@@ -2304,7 +2304,7 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
/* commit one file (adjust contents, adjust mapping),
return first_mapping_index */
-static int commit_one_file(BDRVVVFATState* s,
+static int coroutine_fn commit_one_file(BDRVVVFATState* s,
int dir_index, uint32_t offset)
{
direntry_t* direntry = array_get(&(s->directory), dir_index);
@@ -2559,7 +2559,7 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
/*
* TODO: make sure that the short name is not matching *another* file
*/
-static int handle_commits(BDRVVVFATState* s)
+static int coroutine_fn handle_commits(BDRVVVFATState* s)
{
int i, fail = 0;
@@ -2705,7 +2705,7 @@ static int handle_deletes(BDRVVVFATState* s)
* - recurse direntries from root (using bs->bdrv_read)
* - delete files corresponding to mappings marked as deleted
*/
-static int do_commit(BDRVVVFATState* s)
+static int coroutine_fn do_commit(BDRVVVFATState* s)
{
int ret = 0;
@@ -2757,7 +2757,7 @@ DLOG(checkpoint());
return 0;
}
-static int try_commit(BDRVVVFATState* s)
+static int coroutine_fn try_commit(BDRVVVFATState* s)
{
vvfat_close_current_file(s);
DLOG(checkpoint());
@@ -2766,7 +2766,7 @@ DLOG(checkpoint());
return do_commit(s);
}
-static int vvfat_write(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn vvfat_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
BDRVVVFATState *s = bs->opaque;
diff --git a/blockjob.c b/blockjob.c
index ca80df1..92bb98d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -187,7 +187,7 @@ int block_job_cancel_sync(BlockJob *job)
return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret;
}
-void block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns)
+void coroutine_fn block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns)
{
assert(job->busy);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index c290d07..ac06b85 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -141,7 +141,7 @@ void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
* Put the job to sleep (assuming that it wasn't canceled) for @ns
* nanoseconds. Canceling the job will interrupt the wait immediately.
*/
-void block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns);
+void coroutine_fn block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns);
/**
* block_job_completed:
diff --git a/nbd.c b/nbd.c
index 2606403..dd835a3 100644
--- a/nbd.c
+++ b/nbd.c
@@ -971,7 +971,7 @@ static int nbd_can_read(void *opaque);
static void nbd_read(void *opaque);
static void nbd_restart_write(void *opaque);
-static ssize_t nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
+static ssize_t coroutine_fn nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
int len)
{
NBDClient *client = req->client;
@@ -1055,7 +1055,7 @@ out:
return rc;
}
-static void nbd_trip(void *opaque)
+static void coroutine_fn nbd_trip(void *opaque)
{
NBDClient *client = opaque;
NBDExport *exp = client->exp;
diff --git a/qemu-img.c b/qemu-img.c
index f8c97d3..eba5d2a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -324,7 +324,7 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
return 0;
}
-static int img_create(int argc, char **argv)
+static int coroutine_fn img_create(int argc, char **argv)
{
int c;
uint64_t img_size = -1;
@@ -519,7 +519,7 @@ static int collect_image_check(BlockDriverState *bs,
* 2 - Check completed, image is corrupted
* 3 - Check completed, image has leaked clusters, but is good otherwise
*/
-static int img_check(int argc, char **argv)
+static int coroutine_fn img_check(int argc, char **argv)
{
int c, ret;
OutputFormat output_format = OFORMAT_HUMAN;
@@ -654,7 +654,7 @@ fail:
return ret;
}
-static int img_commit(int argc, char **argv)
+static int coroutine_fn img_commit(int argc, char **argv)
{
int c, ret, flags;
const char *filename, *fmt, *cache;
@@ -851,7 +851,7 @@ static int64_t sectors_to_process(int64_t total, int64_t from)
* @param buffer: Allocated buffer for storing read data
* @param quiet: Flag for quiet mode
*/
-static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num,
+static int coroutine_fn check_empty_sectors(BlockDriverState *bs, int64_t sect_num,
int sect_count, const char *filename,
uint8_t *buffer, bool quiet)
{
@@ -879,7 +879,7 @@ static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num,
* 1 - Images differ
* >1 - Error occurred
*/
-static int img_compare(int argc, char **argv)
+static int coroutine_fn img_compare(int argc, char **argv)
{
const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
BlockDriverState *bs1, *bs2;
@@ -1111,7 +1111,7 @@ out3:
return ret;
}
-static int img_convert(int argc, char **argv)
+static int coroutine_fn img_convert(int argc, char **argv)
{
int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
int progress = 0, flags;
@@ -1701,7 +1701,7 @@ err:
return NULL;
}
-static int img_info(int argc, char **argv)
+static int coroutine_fn img_info(int argc, char **argv)
{
int c;
OutputFormat output_format = OFORMAT_HUMAN;
@@ -1782,7 +1782,7 @@ static int img_info(int argc, char **argv)
#define SNAPSHOT_APPLY 3
#define SNAPSHOT_DELETE 4
-static int img_snapshot(int argc, char **argv)
+static int coroutine_fn img_snapshot(int argc, char **argv)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn;
@@ -1899,7 +1899,7 @@ static int img_snapshot(int argc, char **argv)
return 0;
}
-static int img_rebase(int argc, char **argv)
+static int coroutine_fn img_rebase(int argc, char **argv)
{
BlockDriverState *bs, *bs_old_backing = NULL, *bs_new_backing = NULL;
BlockDriver *old_backing_drv, *new_backing_drv;
@@ -2181,7 +2181,7 @@ out:
return 0;
}
-static int img_resize(int argc, char **argv)
+static int coroutine_fn img_resize(int argc, char **argv)
{
int c, ret, relative;
const char *filename, *fmt, *size;
@@ -2314,6 +2314,38 @@ static const img_cmd_t img_cmds[] = {
{ NULL, NULL, },
};
+struct HandlerCo {
+ int coroutine_fn (*handler)(int, char **);
+ int argc;
+ char **argv;
+ int ret;
+};
+
+
+static void coroutine_fn handler_entry(void *opaque)
+{
+ struct HandlerCo *hco = opaque;
+ hco->ret = hco->handler(hco->argc, hco->argv);
+ hco->handler = NULL;
+}
+
+static int run_handler(int coroutine_fn (*handler)(int, char **), int argc, char **argv)
+{
+ struct HandlerCo hco = {
+ .handler = handler,
+ .argc = argc,
+ .argv = argv,
+ };
+
+ Coroutine *co = qemu_coroutine_create(handler_entry);
+ qemu_coroutine_enter(co, &hco);
+ while (hco.handler) {
+ qemu_aio_wait();
+ }
+
+ return hco.ret;
+}
+
int main(int argc, char **argv)
{
const img_cmd_t *cmd;
@@ -2331,7 +2363,7 @@ int main(int argc, char **argv)
/* find the command */
for(cmd = img_cmds; cmd->name != NULL; cmd++) {
if (!strcmp(cmdname, cmd->name)) {
- return cmd->handler(argc, argv);
+ return run_handler(cmd->handler, argc, argv);
}
}
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index ffbcf31..d15c800 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -385,7 +385,7 @@ static int do_read(BlockDriverState *bs, char *buf, int64_t offset, int count,
{
int ret;
- ret = bdrv_read(bs, offset >> 9, (uint8_t *)buf, count >> 9);
+ ret = bdrv_read_sync(bs, offset >> 9, (uint8_t *)buf, count >> 9);
if (ret < 0) {
return ret;
}
@@ -398,7 +398,7 @@ static int do_write(BlockDriverState *bs, char *buf, int64_t offset, int count,
{
int ret;
- ret = bdrv_write(bs, offset >> 9, (uint8_t *)buf, count >> 9);
+ ret = bdrv_write_sync(bs, offset >> 9, (uint8_t *)buf, count >> 9);
if (ret < 0) {
return ret;
}
@@ -409,7 +409,7 @@ static int do_write(BlockDriverState *bs, char *buf, int64_t offset, int count,
static int do_pread(BlockDriverState *bs, char *buf, int64_t offset, int count,
int *total)
{
- *total = bdrv_pread(bs, offset, (uint8_t *)buf, count);
+ *total = bdrv_pread_sync(bs, offset, (uint8_t *)buf, count);
if (*total < 0) {
return *total;
}
@@ -419,7 +419,7 @@ static int do_pread(BlockDriverState *bs, char *buf, int64_t offset, int count,
static int do_pwrite(BlockDriverState *bs, char *buf, int64_t offset, int count,
int *total)
{
- *total = bdrv_pwrite(bs, offset, (uint8_t *)buf, count);
+ *total = bdrv_pwrite_sync(bs, offset, (uint8_t *)buf, count);
if (*total < 0) {
return *total;
}
@@ -1608,7 +1608,7 @@ static const cmdinfo_t aio_flush_cmd = {
static int flush_f(BlockDriverState *bs, int argc, char **argv)
{
- bdrv_flush(bs);
+ bdrv_flush_sync(bs);
return 0;
}
@@ -1777,7 +1777,7 @@ static int discard_f(BlockDriverState *bs, int argc, char **argv)
}
gettimeofday(&t1, NULL);
- ret = bdrv_discard(bs, offset >> BDRV_SECTOR_BITS,
+ ret = bdrv_discard_sync(bs, offset >> BDRV_SECTOR_BITS,
count >> BDRV_SECTOR_BITS);
gettimeofday(&t2, NULL);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 18:44 ` [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations Charlie Shepherd
@ 2013-08-05 19:23 ` Gabriel Kerneis
2013-08-05 19:33 ` Charlie Shepherd
2013-08-06 9:24 ` Kevin Wolf
1 sibling, 1 reply; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-05 19:23 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, qemu-devel, stefanha
Hi Charlie,
Many thanks for this patch series.
On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
> they are executed in a coroutine context or not.
The commit message is incomplete. You also convert brdv_read and brdv_write.
Moreover:
> diff --git a/block/ssh.c b/block/ssh.c
> index d7e7bf8..66ac4c1 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -748,7 +748,7 @@ static int return_true(void *opaque)
> return 1;
> }
>
> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> +static void set_fd_handler(BDRVSSHState *s)
> {
> int r;
> IOHandler *rd_handler = NULL, *wr_handler = NULL;
> @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
> }
>
> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
> +static void clear_fd_handler(BDRVSSHState *s)
> {
> DPRINTF("s->sock=%d", s->sock);
> qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> index f133d65..d0ab27d 100644
> --- a/include/block/coroutine_int.h
> +++ b/include/block/coroutine_int.h
> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
> void qemu_coroutine_delete(Coroutine *co);
> CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
> CoroutineAction action);
> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> +void qemu_co_queue_run_restart(Coroutine *co);
>
> #endif
Adding coroutine_fn where it is necessary seems straightforward to me: just
follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
got it right and did not over-annotate). On the other hand, you
should probably explain in the commit message why you *remove* those three
coroutine_fn annotations.
Best,
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (4 preceding siblings ...)
2013-08-05 18:44 ` [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations Charlie Shepherd
@ 2013-08-05 19:25 ` Charlie Shepherd
2013-08-06 7:06 ` Gabriel Kerneis
2013-08-06 9:37 ` Kevin Wolf
7 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 19:25 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, gabriel, qemu-devel, stefanha
On 05/08/2013 19:44, Charlie Shepherd wrote:
> In order to compile the tree with CPC:
> $ git clone git://github.com/kerneis/cpc.git
> $ cd cpc
> $ make
> $ ./configure
> $ make
> $ cd ..
> $ export CPC=$(pwd)/cpc/bin/cpc
> $ cd qemu
> $ mkdir -p bin/cpc
> $ cd bin/cpc
> $ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --warnall "
> $ make
Sorry, I should have corrected this to:
$ git clone git://github.com/kerneis/cpc.git -b cps-inference
$ cd cpc
$ make
$ ./configure
$ make
$ cd ..
$ export CPC=$(pwd)/cpc/bin/cpc
$ cd qemu
$ mkdir -p bin/cpc
$ cd bin/cpc
$ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --dont-cpc --warnall "
$ make
This will use the cps-inference branch, which will provide more information about the annotation graph, and using the --dont-cpc option will disable the CPS translation.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 19:23 ` Gabriel Kerneis
@ 2013-08-05 19:33 ` Charlie Shepherd
2013-08-05 20:05 ` Gabriel Kerneis
2013-08-07 19:30 ` Stefan Hajnoczi
0 siblings, 2 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-05 19:33 UTC (permalink / raw)
To: Gabriel Kerneis; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On 05/08/2013 20:23, Gabriel Kerneis wrote:
> On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
>> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
>> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
>> they are executed in a coroutine context or not.
> The commit message is incomplete. You also convert brdv_read and brdv_write.
Thanks, I do need to mention this.
> Moreover:
>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index d7e7bf8..66ac4c1 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -748,7 +748,7 @@ static int return_true(void *opaque)
>> return 1;
>> }
>>
>> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>> +static void set_fd_handler(BDRVSSHState *s)
>> {
>> int r;
>> IOHandler *rd_handler = NULL, *wr_handler = NULL;
>> @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>> qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
>> }
>>
>> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
>> +static void clear_fd_handler(BDRVSSHState *s)
>> {
>> DPRINTF("s->sock=%d", s->sock);
>> qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
>> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
>> index f133d65..d0ab27d 100644
>> --- a/include/block/coroutine_int.h
>> +++ b/include/block/coroutine_int.h
>> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>> void qemu_coroutine_delete(Coroutine *co);
>> CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>> CoroutineAction action);
>> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
>> +void qemu_co_queue_run_restart(Coroutine *co);
>>
>> #endif
> Adding coroutine_fn where it is necessary seems straightforward to me: just
> follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
> got it right and did not over-annotate). On the other hand, you
> should probably explain in the commit message why you *remove* those three
> coroutine_fn annotations.
Yes that does merit some explanation.
Building the tree with cps inference warned that these functions were
annotated spuriously. I initially thought this was because they called a
coroutine function that hadn't been annotated, but it seems the
*_handler functions called qemu_aio_set_fd_handler which, from my
investigation, it seems does not need annotating. Therefore they were
indeed spuriously annotated and so I removed the annotation.
qemu_co_queue_run_restart is a bit different. It is only called from
coroutine_swap in qemu-coroutine.c, and it enters coroutines that were
waiting but have now moved to the runnable state by the actions of the
most recent coroutine (I believe). I think we discussed this on IRC on
Thursday? It only calls qemu_coroutine_enter, and cannot yield, so again
I removed the annotation. I'll add these explanations to the commit message.
Thanks,
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions
2013-08-05 18:44 ` [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions Charlie Shepherd
@ 2013-08-05 20:01 ` Gabriel Kerneis
2013-08-06 9:36 ` Kevin Wolf
1 sibling, 0 replies; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-05 20:01 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On Mon, Aug 05, 2013 at 08:44:06PM +0200, Charlie Shepherd wrote:
> This patch follows on from the previous one and converts some block layer functions to be
> explicitly annotated with coroutine_fn instead of yielding depending upon calling context.
And just like the previous one, it also removes one annotation, which you
might want to mention in the commit message:
> -int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
> +int coroutine_fn bdrv_flush(BlockDriverState *bs)
By the way:
> diff --git a/block.c b/block.c
> index aaa122c..e7011f9 100644
> --- a/block.c
> +++ b/block.c
> @@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>
> typedef struct CreateCo {
> BlockDriver *drv;
> - char *filename;
> + const char *filename;
> QEMUOptionParameter *options;
> int ret;
> } CreateCo;
This looks like an unrelated change which might deserve its own patch. I'm not
sure what the QEMU policiy is about that kind of minor fix, but maybe send it to
qemu-trivial?
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 19:33 ` Charlie Shepherd
@ 2013-08-05 20:05 ` Gabriel Kerneis
2013-08-06 9:04 ` Kevin Wolf
2013-08-07 19:30 ` Stefan Hajnoczi
1 sibling, 1 reply; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-05 20:05 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> Yes that does merit some explanation.
Thanks for the details.
> qemu_co_queue_run_restart is a bit different. It is only called from
> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> were waiting but have now moved to the runnable state by the actions
> of the most recent coroutine (I believe). I think we discussed this
> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> yield, so again I removed the annotation. I'll add these
> explanations to the commit message.
Or maybe split those in different commits altogether? I find it often easier to
review many small commits than a big one, each focused on an "atomic" change.
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations
2013-08-05 18:44 ` [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations Charlie Shepherd
@ 2013-08-05 20:15 ` Gabriel Kerneis
2013-08-08 1:19 ` Charlie Shepherd
0 siblings, 1 reply; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-05 20:15 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On Mon, Aug 05, 2013 at 08:44:07PM +0200, Charlie Shepherd wrote:
> This patch updates the callers of block layer functions converted to explicit
> coroutine_fn annotation in the previous patch.
It looks like this patch is made of three parts:
- updating the annotations, following the rule "caller of coroutine_fn must be
coroutine_fn",
- introducing a run_handler function in qemu-img.c,
- replacing bdrv_* by bdrv_*_sync in qemu-io-cmds.c.
The first one is purely mechanical and boring. The second one is moderately
inventive (note that I did not review the code in detail). The third one is
simple but definitely deserves some explanation. Again, I think this could be
split in three different patches, with an expanded commit message.
Best,
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (5 preceding siblings ...)
2013-08-05 19:25 ` [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
@ 2013-08-06 7:06 ` Gabriel Kerneis
2013-08-06 9:37 ` Kevin Wolf
7 siblings, 0 replies; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-06 7:06 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On Mon, Aug 05, 2013 at 08:44:02PM +0200, Charlie Shepherd wrote:
> This patch series is a follow up to a previous RFC about converting functions
> that dynamically yield execution depending on whether they are in executing in
> a coroutine context or not to be explicitly statically annotated.
Another general remark on your patches for the next iteration: they need to
include a Signed-off-by line.
http://wiki.qemu.org/Contribute/SubmitAPatch
git config --global format.signoff true will make sure such a line is included
automatically when you use git format-patch (but you can also sign-off individual
commits locally with git commit -s).
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn
2013-08-05 18:44 ` [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn Charlie Shepherd
@ 2013-08-06 8:39 ` Stefan Hajnoczi
2013-08-08 1:20 ` Charlie Shepherd
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-08-06 8:39 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, Charlie Shepherd, gabriel, qemu-devel
On Mon, Aug 05, 2013 at 08:44:03PM +0200, Charlie Shepherd wrote:
> From: Charlie Shepherd <cs648@cam.ac.uk>
>
> Coroutine functions that can yield directly or indirectly should be annotated
> with a coroutine_fn annotation. Add an explanation to that effect in
> include/block/coroutine.h.
> ---
> include/block/coroutine.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/block/coroutine.h b/include/block/coroutine.h
> index 377805a..3b94b6d 100644
> --- a/include/block/coroutine.h
> +++ b/include/block/coroutine.h
> @@ -37,6 +37,9 @@
> * static checker support for catching such errors. This annotation might make
> * it possible and in the meantime it serves as documentation.
> *
> + * A function must be marked with coroutine_fn if it can yield execution, either
> + * directly or indirectly.
> + *
This is correct except for the case of dynamic functions that do:
if (qemu_in_coroutine()) {
} else {
Coroutine *co = qemu_coroutine_new(...);
...
}
Here the function is coroutine_fn only if the caller is in coroutine
context.
I think your comment update should include a note about this. When
you've split all dynamic functions into coroutine/non-coroutine versions
then the note can be removed.
Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 20:05 ` Gabriel Kerneis
@ 2013-08-06 9:04 ` Kevin Wolf
0 siblings, 0 replies; 35+ messages in thread
From: Kevin Wolf @ 2013-08-06 9:04 UTC (permalink / raw)
To: Gabriel Kerneis; +Cc: stefanha, Charlie Shepherd, qemu-devel, pbonzini
Am 05.08.2013 um 22:05 hat Gabriel Kerneis geschrieben:
> On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> > Yes that does merit some explanation.
>
> Thanks for the details.
>
> > qemu_co_queue_run_restart is a bit different. It is only called from
> > coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> > were waiting but have now moved to the runnable state by the actions
> > of the most recent coroutine (I believe). I think we discussed this
> > on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> > yield, so again I removed the annotation. I'll add these
> > explanations to the commit message.
>
> Or maybe split those in different commits altogether? I find it often easier to
> review many small commits than a big one, each focused on an "atomic" change.
Yes, please. One logical change per commit, and no changes that aren't
mentioned in the commit message.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 18:44 ` [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations Charlie Shepherd
2013-08-05 19:23 ` Gabriel Kerneis
@ 2013-08-06 9:24 ` Kevin Wolf
2013-08-08 1:14 ` Charlie Shepherd
1 sibling, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2013-08-06 9:24 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: pbonzini, gabriel, qemu-devel, stefanha
Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
> they are executed in a coroutine context or not.
I wonder whether this patch should be split into one for each converted
BlockDriver function. It would probably make review easier.
For those function where you actually just correct the coroutine_fn
annotation, but whose behaviour doesn't change, a single patch for all
might be enough.
> block.c | 16 +++++++--------
> block/blkdebug.c | 10 ++++-----
> block/blkverify.c | 4 ++--
> block/bochs.c | 8 ++++----
> block/cloop.c | 6 +++---
> block/cow.c | 12 +++++------
> block/curl.c | 12 +++++------
> block/dmg.c | 6 +++---
> block/nbd.c | 28 ++++++++++++-------------
> block/parallels.c | 6 +++---
> block/qcow.c | 8 ++++----
> block/qcow2-cluster.c | 8 ++++----
> block/qcow2.c | 48 +++++++++++++++++++++++++++++++++++++------
> block/qcow2.h | 4 ++--
> block/qed.c | 8 ++++----
> block/raw-posix.c | 34 +++++++++++++++---------------
> block/raw.c | 8 ++++----
> block/sheepdog.c | 24 +++++++++++-----------
> block/snapshot.c | 32 ++++++++++++++++++++++++++++-
> block/ssh.c | 14 ++++++-------
> block/vdi.c | 12 +++++------
> block/vhdx.c | 4 ++--
> block/vmdk.c | 12 +++++------
> block/vpc.c | 12 +++++------
> block/vvfat.c | 12 +++++------
> include/block/block_int.h | 10 ++++-----
> include/block/coroutine.h | 4 ++--
> include/block/coroutine_int.h | 2 +-
> qemu-coroutine-lock.c | 4 ++--
> 30 files changed, 218 insertions(+), 152 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6c493ad..aaa122c 100644
> --- a/block.c
> +++ b/block.c
> @@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
> CreateCo *cco = opaque;
> assert(cco->drv);
>
> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
> + cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
> }
>
> int bdrv_create(BlockDriver *drv, const char* filename,
> @@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> .ret = NOT_DONE,
> };
>
> - if (!drv->bdrv_create) {
> + if (!drv->bdrv_co_create) {
> ret = -ENOTSUP;
> goto out;
> }
> @@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> /* 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) {
> + if (file != NULL && drv->bdrv_co_file_open) {
> bdrv_swap(file, bs);
> return 0;
> }
> @@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
>
> /* Open the image, either directly or using a protocol */
> - if (drv->bdrv_file_open) {
> + if (drv->bdrv_co_file_open) {
> assert(file == NULL);
> assert(drv->bdrv_parse_filename || filename != NULL);
> - ret = drv->bdrv_file_open(bs, options, open_flags);
> + ret = drv->bdrv_co_file_open(bs, options, open_flags);
bdrv_open_common() isn't coroutine_fn, though?
Ah well, I see that you change it in a later patch. Please make sure
that the code is in a consistent state after each single commit.
For this series, I think this suggests that you indeed split by converted
function, but put everything related to this function in one patch. For
example, the bdrv_open_common() conversion would be in the same patch as
this hunk.
> } else {
> if (file == NULL) {
> qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
> @@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> }
> assert(file != NULL);
> bs->file = file;
> - ret = drv->bdrv_open(bs, options, open_flags);
> + ret = drv->bdrv_co_open(bs, options, open_flags);
> }
>
> if (ret < 0) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0eceefe..2ed0bb6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -58,6 +58,10 @@ typedef struct {
> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>
> +
> +#define NOT_DONE 0x7fffffff
> +
> +
> static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> {
> const QCowHeader *cow_header = (const void *)buf;
> @@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = {
> },
> };
>
> -static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> +static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options, int flags)
> {
> BDRVQcowState *s = bs->opaque;
> int len, i, ret = 0;
> @@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> return ret;
> }
>
> +struct QOpenCo {
> + BlockDriverState *bs;
> + QDict *options;
> + int flags;
> + int ret;
> +};
> +
> +static void coroutine_fn qcow2_co_open_entry(void *opaque)
> +{
> + struct QOpenCo *qo = opaque;
> +
> + qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags);
> +}
> +
> +static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> +{
> + Coroutine *co;
> + struct QOpenCo qo = {
> + .bs = bs,
> + .options = options,
> + .flags = flags,
> + .ret = NOT_DONE,
> + };
> +
> + co = qemu_coroutine_create(qcow2_co_open_entry);
> + qemu_coroutine_enter(co, &qo);
> + while (qo.ret == NOT_DONE) {
> + qemu_aio_wait();
> + }
> + return qo.ret;
> +}
I think it would be better to convert qcow2_invalidate_cache() to
coroutine_fn (which you apparently do in patch 4) so that it can
directly call qcow2_co_open, and to keep this coroutine wrapper in
the block.c function.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions
2013-08-05 18:44 ` [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions Charlie Shepherd
2013-08-05 20:01 ` Gabriel Kerneis
@ 2013-08-06 9:36 ` Kevin Wolf
2013-08-08 1:17 ` Charlie Shepherd
1 sibling, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2013-08-06 9:36 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: pbonzini, gabriel, qemu-devel, stefanha
Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> This patch follows on from the previous one and converts some block layer functions to be
> explicitly annotated with coroutine_fn instead of yielding depending upon calling context.
> ---
> block.c | 235 ++++++++++++++++++++++++++------------------------
> block/mirror.c | 4 +-
> include/block/block.h | 37 ++++----
> 3 files changed, 148 insertions(+), 128 deletions(-)
>
> diff --git a/block.c b/block.c
> index aaa122c..e7011f9 100644
> --- a/block.c
> +++ b/block.c
> @@ -164,7 +164,7 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs)
> || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
> }
>
> -static void bdrv_io_limits_intercept(BlockDriverState *bs,
> +static void coroutine_fn bdrv_io_limits_intercept(BlockDriverState *bs,
> bool is_write, int nb_sectors)
> {
> int64_t wait_time = -1;
> @@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>
> typedef struct CreateCo {
> BlockDriver *drv;
> - char *filename;
> + const char *filename;
> QEMUOptionParameter *options;
> int ret;
> } CreateCo;
Like Gabriel said, this should be a separate patch. Keeping it in this
series is probably easiest.
> @@ -372,48 +372,48 @@ typedef struct CreateCo {
> static void coroutine_fn bdrv_create_co_entry(void *opaque)
> {
> CreateCo *cco = opaque;
> - assert(cco->drv);
> -
> - cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
> + cco->ret = bdrv_create(cco->drv, cco->filename, cco->options);
> }
>
> -int bdrv_create(BlockDriver *drv, const char* filename,
> +int coroutine_fn bdrv_create(BlockDriver *drv, const char* filename,
> QEMUOptionParameter *options)
> {
> int ret;
> + char *dup_fn;
> +
> + assert(drv);
> + if (!drv->bdrv_co_create) {
> + return -ENOTSUP;
> + }
>
> + dup_fn = g_strdup(filename);
> + ret = drv->bdrv_co_create(dup_fn, options);
> + g_free(dup_fn);
> + return ret;
> +}
> +
> +
> +int bdrv_create_sync(BlockDriver *drv, const char* filename,
> + QEMUOptionParameter *options)
bdrv_foo_sync is an unfortunate naming convention, because
bdrv_pwrite_sync already exists and means something totally different:
It's a write directly followed by a disk flush.
> diff --git a/include/block/block.h b/include/block/block.h
> index dd8eca1..57d8ba2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -125,9 +125,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> void bdrv_delete(BlockDriverState *bs);
> int bdrv_parse_cache_flags(const char *mode, int *flags);
> int bdrv_parse_discard_flags(const char *mode, int *flags);
> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> +int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDict *options, int flags);
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
> +int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
> int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> int flags, BlockDriver *drv);
> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> @@ -150,18 +150,24 @@ void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
> bool bdrv_dev_has_removable_media(BlockDriverState *bs);
> bool bdrv_dev_is_tray_open(BlockDriverState *bs);
> bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
> -int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> +int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num,
> uint8_t *buf, int nb_sectors);
> -int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
> +int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num,
> + uint8_t *buf, int nb_sectors);
> +int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
> uint8_t *buf, int nb_sectors);
> -int bdrv_write(BlockDriverState *bs, int64_t sector_num,
> +int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num,
> + const uint8_t *buf, int nb_sectors);
> +int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num,
> const uint8_t *buf, int nb_sectors);
> -int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
> -int bdrv_pread(BlockDriverState *bs, int64_t offset,
> +int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
> +int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset,
> + void *buf, int count);
> +int bdrv_pread_sync(BlockDriverState *bs, int64_t offset,
> void *buf, int count);
I haven't checked everything, but bdrv_pread_sync is an example of a
declaration without any user and without implementation.
Proper patch splitting will make review of such things easier, so I'll
defer thorough review until then.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
` (6 preceding siblings ...)
2013-08-06 7:06 ` Gabriel Kerneis
@ 2013-08-06 9:37 ` Kevin Wolf
2013-08-08 1:22 ` Charlie Shepherd
7 siblings, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2013-08-06 9:37 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: pbonzini, gabriel, qemu-devel, stefanha
Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> This patch series is a follow up to a previous RFC about converting functions
> that dynamically yield execution depending on whether they are in executing in
> a coroutine context or not to be explicitly statically annotated. This change
> is necessary for the GSoC CPC project, but was also agreed in an IRC
> conversation on #qemu to be benefical overall if it can be upstream before the
> end of the project. This is an update to see if the approach I'm taking to
> implementing this conversion is correct.
>
> In order to statically check the tree to ensure the annotations are correct,
> I've been using CPC to compile the QEMU tree. This does a source to source
> translation to convert coroutine code to continuation-passing style (the
> purpose of the GSoC project), but as a side benefit statically checks
> annotations (any functions annotated with coroutine_fn are transformed into
> CPS, so have a different "calling style" to the standard C convention).
>
> In order to compile the tree with CPC:
> $ git clone git://github.com/kerneis/cpc.git
> $ cd cpc
> $ make
> $ ./configure
> $ make
> $ cd ..
> $ export CPC=$(pwd)/cpc/bin/cpc
> $ cd qemu
> $ mkdir -p bin/cpc
> $ cd bin/cpc
> $ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --warnall "
> $ make
Against which tree is this? It didn't apply on top of qemu-git master,
nor on my block branch.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-05 18:44 ` [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield Charlie Shepherd
@ 2013-08-07 19:18 ` Stefan Hajnoczi
2013-08-07 22:13 ` Gabriel Kerneis
2013-08-08 1:25 ` Charlie Shepherd
0 siblings, 2 replies; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-08-07 19:18 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, Charlie Shepherd, gabriel, qemu-devel
On Mon, Aug 05, 2013 at 08:44:04PM +0200, Charlie Shepherd wrote:
> From: Charlie Shepherd <cs648@cam.ac.uk>
>
> While it only really makes sense to call qemu_coroutine_self() in a coroutine
> context, it cannot actually yield execution itself, so remove the coroutine_fn
> annotation.
> ---
> include/block/coroutine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
By removing coroutine_fn the rules have changed:
It's now legal to call qemu_coroutine_self() outside a coroutine.
Previously only callers that knew the internals of the coroutine
implementation did that.
coroutine_fn gives coroutine backend implementors more flexibility in
how they choose to implement qemu_coroutine_self(). From an API
perspective I prefer to keep qemu_coroutine_self() marked as a
coroutine_fn.
I guess the practical problem is that CPC will get
upset that it's being called by the coroutine implementation from
non-coroutine contexts. This can be solved:
1. Keep the public qemu_coroutine_self() marked coroutine_fn.
2. Have it call an internal coroutine_self() function that is not
coroutine_fn.
This way the API stays strict and the internal implementation doesn't
violate the coroutine_fn calling rule.
Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-05 19:33 ` Charlie Shepherd
2013-08-05 20:05 ` Gabriel Kerneis
@ 2013-08-07 19:30 ` Stefan Hajnoczi
2013-08-08 1:31 ` Charlie Shepherd
2013-08-08 6:27 ` Gabriel Kerneis
1 sibling, 2 replies; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-08-07 19:30 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, pbonzini, Gabriel Kerneis, qemu-devel
On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> On 05/08/2013 20:23, Gabriel Kerneis wrote:
> >On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> >>diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> >>index f133d65..d0ab27d 100644
> >>--- a/include/block/coroutine_int.h
> >>+++ b/include/block/coroutine_int.h
> >>@@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
> >> void qemu_coroutine_delete(Coroutine *co);
> >> CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
> >> CoroutineAction action);
> >>-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> >>+void qemu_co_queue_run_restart(Coroutine *co);
> >> #endif
> >Adding coroutine_fn where it is necessary seems straightforward to me: just
> >follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
> >got it right and did not over-annotate). On the other hand, you
> >should probably explain in the commit message why you *remove* those three
> >coroutine_fn annotations.
>
> Yes that does merit some explanation.
>
> Building the tree with cps inference warned that these functions
> were annotated spuriously. I initially thought this was because they
> called a coroutine function that hadn't been annotated, but it seems
> the *_handler functions called qemu_aio_set_fd_handler which, from
> my investigation, it seems does not need annotating. Therefore they
> were indeed spuriously annotated and so I removed the annotation.
>
> qemu_co_queue_run_restart is a bit different. It is only called from
> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> were waiting but have now moved to the runnable state by the actions
> of the most recent coroutine (I believe). I think we discussed this
> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> yield, so again I removed the annotation. I'll add these
> explanations to the commit message.
I have mixed feelings about removing coroutine_fn annotations from a
function when it does not yield or call other coroutine_fn functions.
These functions were probably written as part of a coroutine code path.
The coroutine_fn annotation tells me I'm in coroutine context.
By removing this information those modifying the code now need to
convert it back to coroutine_fn after auditing callers before they can
use coroutine context.
The thing is, these leaf functions are typically only called from
coroutine context anyway. I think they should be left marked
coroutine_fn.
I'd compare this to a comment that says "lock foo is held across this
function" but the function doesn't use anything that lock foo protects.
Removing the comment isn't really helpful, you are throwing away
information that can be useful when modifying the function.
Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-07 19:18 ` Stefan Hajnoczi
@ 2013-08-07 22:13 ` Gabriel Kerneis
2013-08-08 1:29 ` Charlie Shepherd
2013-08-08 1:25 ` Charlie Shepherd
1 sibling, 1 reply; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-07 22:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, Charlie Shepherd, Charlie Shepherd, qemu-devel, pbonzini
On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
> I guess the practical problem is that CPC will get
> upset that it's being called by the coroutine implementation from
> non-coroutine contexts.
But is it really the case? If Charlie added an annotation, it probably means
that in practice it was only called from coroutine context anyway.
The only downside from CPC's point of view is that there is a cost to making a
coroutine_fn, and it's a pity to pay it when it's useless (ie. when the function
never yields anyway).
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-06 9:24 ` Kevin Wolf
@ 2013-08-08 1:14 ` Charlie Shepherd
0 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:14 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, gabriel, qemu-devel, stefanha
On 06/08/2013 10:24, Kevin Wolf wrote:
> Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
>> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
>> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
>> they are executed in a coroutine context or not.
> I wonder whether this patch should be split into one for each converted
> BlockDriver function. It would probably make review easier.
>
> For those function where you actually just correct the coroutine_fn
> annotation, but whose behaviour doesn't change, a single patch for all
> might be enough.
>
>> block.c | 16 +++++++--------
>> block/blkdebug.c | 10 ++++-----
>> block/blkverify.c | 4 ++--
>> block/bochs.c | 8 ++++----
>> block/cloop.c | 6 +++---
>> block/cow.c | 12 +++++------
>> block/curl.c | 12 +++++------
>> block/dmg.c | 6 +++---
>> block/nbd.c | 28 ++++++++++++-------------
>> block/parallels.c | 6 +++---
>> block/qcow.c | 8 ++++----
>> block/qcow2-cluster.c | 8 ++++----
>> block/qcow2.c | 48 +++++++++++++++++++++++++++++++++++++------
>> block/qcow2.h | 4 ++--
>> block/qed.c | 8 ++++----
>> block/raw-posix.c | 34 +++++++++++++++---------------
>> block/raw.c | 8 ++++----
>> block/sheepdog.c | 24 +++++++++++-----------
>> block/snapshot.c | 32 ++++++++++++++++++++++++++++-
>> block/ssh.c | 14 ++++++-------
>> block/vdi.c | 12 +++++------
>> block/vhdx.c | 4 ++--
>> block/vmdk.c | 12 +++++------
>> block/vpc.c | 12 +++++------
>> block/vvfat.c | 12 +++++------
>> include/block/block_int.h | 10 ++++-----
>> include/block/coroutine.h | 4 ++--
>> include/block/coroutine_int.h | 2 +-
>> qemu-coroutine-lock.c | 4 ++--
>> 30 files changed, 218 insertions(+), 152 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 6c493ad..aaa122c 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> CreateCo *cco = opaque;
>> assert(cco->drv);
>>
>> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
>> + cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
>> }
>>
>> int bdrv_create(BlockDriver *drv, const char* filename,
>> @@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>> .ret = NOT_DONE,
>> };
>>
>> - if (!drv->bdrv_create) {
>> + if (!drv->bdrv_co_create) {
>> ret = -ENOTSUP;
>> goto out;
>> }
>> @@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>> /* 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) {
>> + if (file != NULL && drv->bdrv_co_file_open) {
>> bdrv_swap(file, bs);
>> return 0;
>> }
>> @@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>> bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
>>
>> /* Open the image, either directly or using a protocol */
>> - if (drv->bdrv_file_open) {
>> + if (drv->bdrv_co_file_open) {
>> assert(file == NULL);
>> assert(drv->bdrv_parse_filename || filename != NULL);
>> - ret = drv->bdrv_file_open(bs, options, open_flags);
>> + ret = drv->bdrv_co_file_open(bs, options, open_flags);
> bdrv_open_common() isn't coroutine_fn, though?
>
> Ah well, I see that you change it in a later patch. Please make sure
> that the code is in a consistent state after each single commit.
>
> For this series, I think this suggests that you indeed split by converted
> function, but put everything related to this function in one patch. For
> example, the bdrv_open_common() conversion would be in the same patch as
> this hunk.
Yeah, it's in this kind of scenario that I struggled to split up the
patch series properly. I'll try and implement your approach in the next
version.
>> } else {
>> if (file == NULL) {
>> qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
>> @@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>> }
>> assert(file != NULL);
>> bs->file = file;
>> - ret = drv->bdrv_open(bs, options, open_flags);
>> + ret = drv->bdrv_co_open(bs, options, open_flags);
>> }
>>
>> if (ret < 0) {
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 0eceefe..2ed0bb6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -58,6 +58,10 @@ typedef struct {
>> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>
>> +
>> +#define NOT_DONE 0x7fffffff
>> +
>> +
>> static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>> {
>> const QCowHeader *cow_header = (const void *)buf;
>> @@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = {
>> },
>> };
>>
>> -static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
>> +static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options, int flags)
>> {
>> BDRVQcowState *s = bs->opaque;
>> int len, i, ret = 0;
>> @@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
>> return ret;
>> }
>>
>> +struct QOpenCo {
>> + BlockDriverState *bs;
>> + QDict *options;
>> + int flags;
>> + int ret;
>> +};
>> +
>> +static void coroutine_fn qcow2_co_open_entry(void *opaque)
>> +{
>> + struct QOpenCo *qo = opaque;
>> +
>> + qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags);
>> +}
>> +
>> +static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
>> +{
>> + Coroutine *co;
>> + struct QOpenCo qo = {
>> + .bs = bs,
>> + .options = options,
>> + .flags = flags,
>> + .ret = NOT_DONE,
>> + };
>> +
>> + co = qemu_coroutine_create(qcow2_co_open_entry);
>> + qemu_coroutine_enter(co, &qo);
>> + while (qo.ret == NOT_DONE) {
>> + qemu_aio_wait();
>> + }
>> + return qo.ret;
>> +}
> I think it would be better to convert qcow2_invalidate_cache() to
> coroutine_fn (which you apparently do in patch 4) so that it can
> directly call qcow2_co_open, and to keep this coroutine wrapper in
> the block.c function.
Ok I'll make this change.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions
2013-08-06 9:36 ` Kevin Wolf
@ 2013-08-08 1:17 ` Charlie Shepherd
0 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, gabriel, qemu-devel, stefanha
On 06/08/2013 10:36, Kevin Wolf wrote:
> Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
>> This patch follows on from the previous one and converts some block layer functions to be
>> explicitly annotated with coroutine_fn instead of yielding depending upon calling context.
>> ---
>> block.c | 235 ++++++++++++++++++++++++++------------------------
>> block/mirror.c | 4 +-
>> include/block/block.h | 37 ++++----
>> 3 files changed, 148 insertions(+), 128 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index aaa122c..e7011f9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -164,7 +164,7 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs)
>> || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
>> }
>>
>> -static void bdrv_io_limits_intercept(BlockDriverState *bs,
>> +static void coroutine_fn bdrv_io_limits_intercept(BlockDriverState *bs,
>> bool is_write, int nb_sectors)
>> {
>> int64_t wait_time = -1;
>> @@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>>
>> typedef struct CreateCo {
>> BlockDriver *drv;
>> - char *filename;
>> + const char *filename;
>> QEMUOptionParameter *options;
>> int ret;
>> } CreateCo;
> Like Gabriel said, this should be a separate patch. Keeping it in this
> series is probably easiest.
>
>> @@ -372,48 +372,48 @@ typedef struct CreateCo {
>> static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> {
>> CreateCo *cco = opaque;
>> - assert(cco->drv);
>> -
>> - cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
>> + cco->ret = bdrv_create(cco->drv, cco->filename, cco->options);
>> }
>>
>> -int bdrv_create(BlockDriver *drv, const char* filename,
>> +int coroutine_fn bdrv_create(BlockDriver *drv, const char* filename,
>> QEMUOptionParameter *options)
>> {
>> int ret;
>> + char *dup_fn;
>> +
>> + assert(drv);
>> + if (!drv->bdrv_co_create) {
>> + return -ENOTSUP;
>> + }
>>
>> + dup_fn = g_strdup(filename);
>> + ret = drv->bdrv_co_create(dup_fn, options);
>> + g_free(dup_fn);
>> + return ret;
>> +}
>> +
>> +
>> +int bdrv_create_sync(BlockDriver *drv, const char* filename,
>> + QEMUOptionParameter *options)
> bdrv_foo_sync is an unfortunate naming convention, because
> bdrv_pwrite_sync already exists and means something totally different:
> It's a write directly followed by a disk flush.
Yes it's not a great naming convention. I could rename bdrv_pwrite_sync
to something like bdrv_pwrite_and_sync? Or is that too horrible?
An alternative would be bdrv_foo and bdrv_sync_foo? But bdrv_sync_pwrite
and bdrv_pwrite_sync would be quite confusing in a hurry.
>> diff --git a/include/block/block.h b/include/block/block.h
>> index dd8eca1..57d8ba2 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -125,9 +125,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
>> void bdrv_delete(BlockDriverState *bs);
>> int bdrv_parse_cache_flags(const char *mode, int *flags);
>> int bdrv_parse_discard_flags(const char *mode, int *flags);
>> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> +int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> QDict *options, int flags);
>> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
>> +int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
>> int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> int flags, BlockDriver *drv);
>> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>> @@ -150,18 +150,24 @@ void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
>> bool bdrv_dev_has_removable_media(BlockDriverState *bs);
>> bool bdrv_dev_is_tray_open(BlockDriverState *bs);
>> bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
>> -int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>> +int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num,
>> uint8_t *buf, int nb_sectors);
>> -int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
>> +int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num,
>> + uint8_t *buf, int nb_sectors);
>> +int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
>> uint8_t *buf, int nb_sectors);
>> -int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>> +int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num,
>> + const uint8_t *buf, int nb_sectors);
>> +int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num,
>> const uint8_t *buf, int nb_sectors);
>> -int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
>> -int bdrv_pread(BlockDriverState *bs, int64_t offset,
>> +int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
>> +int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset,
>> + void *buf, int count);
>> +int bdrv_pread_sync(BlockDriverState *bs, int64_t offset,
>> void *buf, int count);
> I haven't checked everything, but bdrv_pread_sync is an example of a
> declaration without any user and without implementation.
>
> Proper patch splitting will make review of such things easier, so I'll
> defer thorough review until then.
Yes my bad, hopefully splitting them as per your other email will catch
these issues.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations
2013-08-05 20:15 ` Gabriel Kerneis
@ 2013-08-08 1:19 ` Charlie Shepherd
0 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:19 UTC (permalink / raw)
To: Gabriel Kerneis; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On 05/08/2013 21:15, Gabriel Kerneis wrote:
> On Mon, Aug 05, 2013 at 08:44:07PM +0200, Charlie Shepherd wrote:
>> This patch updates the callers of block layer functions converted to explicit
>> coroutine_fn annotation in the previous patch.
> It looks like this patch is made of three parts:
> - updating the annotations, following the rule "caller of coroutine_fn must be
> coroutine_fn",
> - introducing a run_handler function in qemu-img.c,
> - replacing bdrv_* by bdrv_*_sync in qemu-io-cmds.c.
>
> The first one is purely mechanical and boring. The second one is moderately
> inventive (note that I did not review the code in detail). The third one is
> simple but definitely deserves some explanation. Again, I think this could be
> split in three different patches, with an expanded commit message.
Ok thanks this seems like a logical way to split them up.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn
2013-08-06 8:39 ` Stefan Hajnoczi
@ 2013-08-08 1:20 ` Charlie Shepherd
0 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:20 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, gabriel, qemu-devel
On 06/08/2013 09:39, Stefan Hajnoczi wrote:
> On Mon, Aug 05, 2013 at 08:44:03PM +0200, Charlie Shepherd wrote:
>> From: Charlie Shepherd <cs648@cam.ac.uk>
>>
>> Coroutine functions that can yield directly or indirectly should be annotated
>> with a coroutine_fn annotation. Add an explanation to that effect in
>> include/block/coroutine.h.
>> ---
>> include/block/coroutine.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/block/coroutine.h b/include/block/coroutine.h
>> index 377805a..3b94b6d 100644
>> --- a/include/block/coroutine.h
>> +++ b/include/block/coroutine.h
>> @@ -37,6 +37,9 @@
>> * static checker support for catching such errors. This annotation might make
>> * it possible and in the meantime it serves as documentation.
>> *
>> + * A function must be marked with coroutine_fn if it can yield execution, either
>> + * directly or indirectly.
>> + *
> This is correct except for the case of dynamic functions that do:
>
> if (qemu_in_coroutine()) {
> } else {
> Coroutine *co = qemu_coroutine_new(...);
> ...
> }
>
> Here the function is coroutine_fn only if the caller is in coroutine
> context.
>
> I think your comment update should include a note about this. When
> you've split all dynamic functions into coroutine/non-coroutine versions
> then the note can be removed.
Hmm ok, I can add a note to that effect.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-06 9:37 ` Kevin Wolf
@ 2013-08-08 1:22 ` Charlie Shepherd
2013-08-08 7:15 ` Kevin Wolf
0 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, gabriel, qemu-devel, stefanha
On 06/08/2013 10:37, Kevin Wolf wrote:
> Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
>> This patch series is a follow up to a previous RFC about converting functions
>> that dynamically yield execution depending on whether they are in executing in
>> a coroutine context or not to be explicitly statically annotated. This change
>> is necessary for the GSoC CPC project, but was also agreed in an IRC
>> conversation on #qemu to be benefical overall if it can be upstream before the
>> end of the project. This is an update to see if the approach I'm taking to
>> implementing this conversion is correct.
>>
>> In order to statically check the tree to ensure the annotations are correct,
>> I've been using CPC to compile the QEMU tree. This does a source to source
>> translation to convert coroutine code to continuation-passing style (the
>> purpose of the GSoC project), but as a side benefit statically checks
>> annotations (any functions annotated with coroutine_fn are transformed into
>> CPS, so have a different "calling style" to the standard C convention).
>>
>> In order to compile the tree with CPC:
>> $ git clone git://github.com/kerneis/cpc.git
>> $ cd cpc
>> $ make
>> $ ./configure
>> $ make
>> $ cd ..
>> $ export CPC=$(pwd)/cpc/bin/cpc
>> $ cd qemu
>> $ mkdir -p bin/cpc
>> $ cd bin/cpc
>> $ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --warnall "
>> $ make
> Against which tree is this? It didn't apply on top of qemu-git master,
> nor on my block branch.
Sorry, just to clarify, this isn't based on QEMU but is a repository
containing the CPC tool, which can then be used to statically check
QEMU; you'll need to clone it into its own repo. In this example it
should be a sibling directory to whichever QEMU checkout you're testing
it on.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-07 19:18 ` Stefan Hajnoczi
2013-08-07 22:13 ` Gabriel Kerneis
@ 2013-08-08 1:25 ` Charlie Shepherd
1 sibling, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:25 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, Charlie Shepherd, gabriel, qemu-devel
On 07/08/2013 20:18, Stefan Hajnoczi wrote:
> On Mon, Aug 05, 2013 at 08:44:04PM +0200, Charlie Shepherd wrote:
>> From: Charlie Shepherd <cs648@cam.ac.uk>
>>
>> While it only really makes sense to call qemu_coroutine_self() in a coroutine
>> context, it cannot actually yield execution itself, so remove the coroutine_fn
>> annotation.
>> ---
>> include/block/coroutine.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> By removing coroutine_fn the rules have changed:
>
> It's now legal to call qemu_coroutine_self() outside a coroutine.
> Previously only callers that knew the internals of the coroutine
> implementation did that.
Yes, I agree that it's probably not helpful to loosen the restrictions
on when its possible to call qemu_coroutine_self().
> coroutine_fn gives coroutine backend implementors more flexibility in
> how they choose to implement qemu_coroutine_self(). From an API
> perspective I prefer to keep qemu_coroutine_self() marked as a
> coroutine_fn.
>
> I guess the practical problem is that CPC will get
> upset that it's being called by the coroutine implementation from
> non-coroutine contexts. This can be solved:
>
> 1. Keep the public qemu_coroutine_self() marked coroutine_fn.
>
> 2. Have it call an internal coroutine_self() function that is not
> coroutine_fn.
>
> This way the API stays strict and the internal implementation doesn't
> violate the coroutine_fn calling rule.
This is a good solution, I'll add a patch implementing it to the series.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-07 22:13 ` Gabriel Kerneis
@ 2013-08-08 1:29 ` Charlie Shepherd
2013-08-08 6:16 ` Gabriel Kerneis
0 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:29 UTC (permalink / raw)
To: Gabriel Kerneis
Cc: kwolf, Stefan Hajnoczi, Charlie Shepherd, qemu-devel, pbonzini
On 07/08/2013 23:13, Gabriel Kerneis wrote:
> On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
>> I guess the practical problem is that CPC will get
>> upset that it's being called by the coroutine implementation from
>> non-coroutine contexts.
> But is it really the case? If Charlie added an annotation, it probably means
> that in practice it was only called from coroutine context anyway.
It was also called from coroutine implementation in functions that
weren't annotated coroutine_fn (qemu_coroutine_switch() and friends).
> The only downside from CPC's point of view is that there is a cost to making a
> coroutine_fn, and it's a pity to pay it when it's useless (ie. when the function
> never yields anyway).
Yes although in this case it won't be too high as the function will
simply be
Coroutine *internal_qemu_coroutine_self(void);
coroutine_fn Coroutine *qemu_coroutine_self(void)
{
return internal_qemu_coroutine_self();
}
and the original qemu_coroutine_self couldn't be inlined, so I don't
think it is too prohibitive. Besides, I doubt qemu_coroutine_self() is
called in many fast-paths.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-07 19:30 ` Stefan Hajnoczi
@ 2013-08-08 1:31 ` Charlie Shepherd
2013-08-08 6:27 ` Gabriel Kerneis
1 sibling, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 1:31 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, Gabriel Kerneis, qemu-devel
On 07/08/2013 20:30, Stefan Hajnoczi wrote:
> On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
>> On 05/08/2013 20:23, Gabriel Kerneis wrote:
>>> On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
>>>> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
>>>> index f133d65..d0ab27d 100644
>>>> --- a/include/block/coroutine_int.h
>>>> +++ b/include/block/coroutine_int.h
>>>> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>>>> void qemu_coroutine_delete(Coroutine *co);
>>>> CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>>>> CoroutineAction action);
>>>> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
>>>> +void qemu_co_queue_run_restart(Coroutine *co);
>>>> #endif
>>> Adding coroutine_fn where it is necessary seems straightforward to me: just
>>> follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
>>> got it right and did not over-annotate). On the other hand, you
>>> should probably explain in the commit message why you *remove* those three
>>> coroutine_fn annotations.
>> Yes that does merit some explanation.
>>
>> Building the tree with cps inference warned that these functions
>> were annotated spuriously. I initially thought this was because they
>> called a coroutine function that hadn't been annotated, but it seems
>> the *_handler functions called qemu_aio_set_fd_handler which, from
>> my investigation, it seems does not need annotating. Therefore they
>> were indeed spuriously annotated and so I removed the annotation.
>>
>> qemu_co_queue_run_restart is a bit different. It is only called from
>> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
>> were waiting but have now moved to the runnable state by the actions
>> of the most recent coroutine (I believe). I think we discussed this
>> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
>> yield, so again I removed the annotation. I'll add these
>> explanations to the commit message.
> I have mixed feelings about removing coroutine_fn annotations from a
> function when it does not yield or call other coroutine_fn functions.
>
> These functions were probably written as part of a coroutine code path.
> The coroutine_fn annotation tells me I'm in coroutine context.
>
> By removing this information those modifying the code now need to
> convert it back to coroutine_fn after auditing callers before they can
> use coroutine context.
>
> The thing is, these leaf functions are typically only called from
> coroutine context anyway. I think they should be left marked
> coroutine_fn.
>
> I'd compare this to a comment that says "lock foo is held across this
> function" but the function doesn't use anything that lock foo protects.
> Removing the comment isn't really helpful, you are throwing away
> information that can be useful when modifying the function.
I see, so in that case coroutine_fn is more a contract saying "this
function can only be, and is only, called from a coroutine context"?
That feels fair enough, but I'll amend the comment in coroutine.h to
that effect.
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-08 1:29 ` Charlie Shepherd
@ 2013-08-08 6:16 ` Gabriel Kerneis
2013-08-08 9:10 ` Charlie Shepherd
0 siblings, 1 reply; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-08 6:16 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, Stefan Hajnoczi, qemu-devel, pbonzini
On Thu, Aug 08, 2013 at 02:29:39AM +0100, Charlie Shepherd wrote:
> On 07/08/2013 23:13, Gabriel Kerneis wrote:
> >On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
> >>I guess the practical problem is that CPC will get
> >>upset that it's being called by the coroutine implementation from
> >>non-coroutine contexts.
> >But is it really the case? If Charlie added an annotation, it probably means
> >that in practice it was only called from coroutine context anyway.
>
> It was also called from coroutine implementation in functions that
> weren't annotated coroutine_fn (qemu_coroutine_switch() and
> friends).
In that case, the interface/implementation split suggested by Stefan makes
sense.
Note that qemu_coroutine_self() in principle really needs to be annotated
coroutine_fn since it accesses (and returns) its execution context. The fact
that it is implemented with thread-local variables in Qemu is an implementation
detail, almost a hack (however smart); the "natural" CPC way would be to just
return the coroutine associated with the current continuation. So keeping the
annotation definitely makes sense.
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
2013-08-07 19:30 ` Stefan Hajnoczi
2013-08-08 1:31 ` Charlie Shepherd
@ 2013-08-08 6:27 ` Gabriel Kerneis
1 sibling, 0 replies; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-08 6:27 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, Charlie Shepherd, qemu-devel, pbonzini
On Wed, Aug 07, 2013 at 09:30:25PM +0200, Stefan Hajnoczi wrote:
> I have mixed feelings about removing coroutine_fn annotations from a
> function when it does not yield or call other coroutine_fn functions.
>
> These functions were probably written as part of a coroutine code path.
> The coroutine_fn annotation tells me I'm in coroutine context.
No, it tells you it is forbidden to call this function from outside coroutine
context. Which is false: if the function never yields, it is definitely correct
to call from somewhere else (unless there is some other invariant in qemu about
coroutine context?).
> By removing this information those modifying the code now need to
> convert it back to coroutine_fn after auditing callers before they can
> use coroutine context.
I don't understand this. You mean someone who, later, would decide to make a
version of the function that yields? In that case, wouldn't it make sense to
introduce an alternative coroutine_fn counter-part? (I see, however, how line of
reasoning might have led to the "dynamic functions" maze).
> I'd compare this to a comment that says "lock foo is held across this
> function" but the function doesn't use anything that lock foo protects.
> Removing the comment isn't really helpful, you are throwing away
> information that can be useful when modifying the function.
Except that the function might also makes sense outside of coroutine context,
and you are forcing people to allocate a spurious coroutine if they want to
use it.
Note that I'm arguing for the sake of defining precisely what Qemu developpers
expect when they read or write "coroutine_fn". As long as we can agree on that
point, and get it documented in coroutine.h, I'm fine. From a performance
point-of-view, it matters only for the CPC backend and there aren't that many
spuriously-annotated functions anyway. We could even benchmark the overhead at
the end of the GSoC, but I don't expect it to be significant (if there were
dozens of them, it would be a different story).
Best,
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-08 1:22 ` Charlie Shepherd
@ 2013-08-08 7:15 ` Kevin Wolf
2013-08-08 9:36 ` Charlie Shepherd
0 siblings, 1 reply; 35+ messages in thread
From: Kevin Wolf @ 2013-08-08 7:15 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: pbonzini, gabriel, qemu-devel, stefanha
Am 08.08.2013 um 03:22 hat Charlie Shepherd geschrieben:
> On 06/08/2013 10:37, Kevin Wolf wrote:
> >Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
> >>This patch series is a follow up to a previous RFC about converting functions
> >>that dynamically yield execution depending on whether they are in executing in
> >>a coroutine context or not to be explicitly statically annotated. This change
> >>is necessary for the GSoC CPC project, but was also agreed in an IRC
> >>conversation on #qemu to be benefical overall if it can be upstream before the
> >>end of the project. This is an update to see if the approach I'm taking to
> >>implementing this conversion is correct.
> >>
> >>In order to statically check the tree to ensure the annotations are correct,
> >>I've been using CPC to compile the QEMU tree. This does a source to source
> >>translation to convert coroutine code to continuation-passing style (the
> >>purpose of the GSoC project), but as a side benefit statically checks
> >>annotations (any functions annotated with coroutine_fn are transformed into
> >>CPS, so have a different "calling style" to the standard C convention).
> >>
> >>In order to compile the tree with CPC:
> >> $ git clone git://github.com/kerneis/cpc.git
> >> $ cd cpc
> >> $ make
> >> $ ./configure
> >> $ make
> >> $ cd ..
> >> $ export CPC=$(pwd)/cpc/bin/cpc
> >> $ cd qemu
> >> $ mkdir -p bin/cpc
> >> $ cd bin/cpc
> >> $ ../../configure --enable-debug --disable-werror --target-list=x86_64-softmmu --cc="$CPC" --extra-cflags="--noMakeStaticGlobal --useLogicalOperators --useCaseRange --save-temps -U__SSE2__ -w -Dcoroutine_fn='__attribute__((__cps__))' --docpsInference --warnall "
> >> $ make
> >Against which tree is this? It didn't apply on top of qemu-git master,
> >nor on my block branch.
>
> Sorry, just to clarify, this isn't based on QEMU but is a repository
> containing the CPC tool, which can then be used to statically check
> QEMU; you'll need to clone it into its own repo. In this example it
> should be a sibling directory to whichever QEMU checkout you're
> testing it on.
Sorry, the quoted cover letter text was probably more confusing than
helpful. I meant what qemu tree your patch series is against.
Kevin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-08 6:16 ` Gabriel Kerneis
@ 2013-08-08 9:10 ` Charlie Shepherd
2013-08-08 9:12 ` Gabriel Kerneis
0 siblings, 1 reply; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 9:10 UTC (permalink / raw)
To: Gabriel Kerneis; +Cc: kwolf, Stefan Hajnoczi, qemu-devel, pbonzini
On 08/08/2013 07:16, Gabriel Kerneis wrote:
> On Thu, Aug 08, 2013 at 02:29:39AM +0100, Charlie Shepherd wrote:
>> On 07/08/2013 23:13, Gabriel Kerneis wrote:
>>> On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
>>>> I guess the practical problem is that CPC will get
>>>> upset that it's being called by the coroutine implementation from
>>>> non-coroutine contexts.
>>> But is it really the case? If Charlie added an annotation, it probably means
>>> that in practice it was only called from coroutine context anyway.
>> It was also called from coroutine implementation in functions that
>> weren't annotated coroutine_fn (qemu_coroutine_switch() and
>> friends).
> In that case, the interface/implementation split suggested by Stefan makes
> sense.
>
> Note that qemu_coroutine_self() in principle really needs to be annotated
> coroutine_fn since it accesses (and returns) its execution context. The fact
> that it is implemented with thread-local variables in Qemu is an implementation
> detail, almost a hack (however smart); the "natural" CPC way would be to just
> return the coroutine associated with the current continuation. So keeping the
> annotation definitely makes sense.
While that would be the natural way, we are going to need the thread
local variables regardless due to the use of the "leader" coroutine I
believe (Stefan is this correct?). Also implementing the split in the
way Stefan suggests would mean it's not possible to return the CPC
continuation without a hack like the last hack I did for
qemu_coroutine_yield(), as internal_qemu_coroutine_self() (I guess this
function needs a better name too) would be marked as non coroutine
across all coroutine implementions (ie in coroutine_int.h).
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield
2013-08-08 9:10 ` Charlie Shepherd
@ 2013-08-08 9:12 ` Gabriel Kerneis
0 siblings, 0 replies; 35+ messages in thread
From: Gabriel Kerneis @ 2013-08-08 9:12 UTC (permalink / raw)
To: Charlie Shepherd; +Cc: kwolf, Stefan Hajnoczi, qemu-devel, pbonzini
On Thu, Aug 08, 2013 at 10:10:18AM +0100, Charlie Shepherd wrote:
> >Note that qemu_coroutine_self() in principle really needs to be annotated
> >coroutine_fn since it accesses (and returns) its execution context. The fact
> >that it is implemented with thread-local variables in Qemu is an implementation
> >detail, almost a hack (however smart); the "natural" CPC way would be to just
> >return the coroutine associated with the current continuation. So keeping the
> >annotation definitely makes sense.
>
> While that would be the natural way, we are going to need the thread
> local variables regardless due to the use of the "leader" coroutine
> I believe (Stefan is this correct?).
Sure. I was simply pointing out that from a "philosophical" point-of-view,
it makes more sense to keep it coroutine_fn.
--
Gabriel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions
2013-08-08 7:15 ` Kevin Wolf
@ 2013-08-08 9:36 ` Charlie Shepherd
0 siblings, 0 replies; 35+ messages in thread
From: Charlie Shepherd @ 2013-08-08 9:36 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, gabriel, qemu-devel, stefanha
On 08/08/2013 08:15, Kevin Wolf wrote:
> Am 08.08.2013 um 03:22 hat Charlie Shepherd geschrieben:
>> On 06/08/2013 10:37, Kevin Wolf wrote:
>>>
>>> Against which tree is this? It didn't apply on top of qemu-git master,
>>> nor on my block branch.
>> Sorry, just to clarify, this isn't based on QEMU but is a repository
>> containing the CPC tool, which can then be used to statically check
>> QEMU; you'll need to clone it into its own repo. In this example it
>> should be a sibling directory to whichever QEMU checkout you're
>> testing it on.
> Sorry, the quoted cover letter text was probably more confusing than
> helpful. I meant what qemu tree your patch series is against.
Ah, that would make more sense. It's based on 1acd5a3, which is just
where master was when I started the patch series a few weeks ago. It
probably makes sense to base it on a more sensible start point, is there
one you'd prefer? Maybe v1.6.0-rc0?
Charlie
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2013-08-08 9:36 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-05 18:44 [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 1/5] Add an explanation of when a function should be marked coroutine_fn Charlie Shepherd
2013-08-06 8:39 ` Stefan Hajnoczi
2013-08-08 1:20 ` Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield Charlie Shepherd
2013-08-07 19:18 ` Stefan Hajnoczi
2013-08-07 22:13 ` Gabriel Kerneis
2013-08-08 1:29 ` Charlie Shepherd
2013-08-08 6:16 ` Gabriel Kerneis
2013-08-08 9:10 ` Charlie Shepherd
2013-08-08 9:12 ` Gabriel Kerneis
2013-08-08 1:25 ` Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations Charlie Shepherd
2013-08-05 19:23 ` Gabriel Kerneis
2013-08-05 19:33 ` Charlie Shepherd
2013-08-05 20:05 ` Gabriel Kerneis
2013-08-06 9:04 ` Kevin Wolf
2013-08-07 19:30 ` Stefan Hajnoczi
2013-08-08 1:31 ` Charlie Shepherd
2013-08-08 6:27 ` Gabriel Kerneis
2013-08-06 9:24 ` Kevin Wolf
2013-08-08 1:14 ` Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions Charlie Shepherd
2013-08-05 20:01 ` Gabriel Kerneis
2013-08-06 9:36 ` Kevin Wolf
2013-08-08 1:17 ` Charlie Shepherd
2013-08-05 18:44 ` [Qemu-devel] [PATCH 5/5] Convert block layer callers' annotations Charlie Shepherd
2013-08-05 20:15 ` Gabriel Kerneis
2013-08-08 1:19 ` Charlie Shepherd
2013-08-05 19:25 ` [Qemu-devel] RFC: [PATCH 0/5] Explicitly annotating coroutine_fn functions Charlie Shepherd
2013-08-06 7:06 ` Gabriel Kerneis
2013-08-06 9:37 ` Kevin Wolf
2013-08-08 1:22 ` Charlie Shepherd
2013-08-08 7:15 ` Kevin Wolf
2013-08-08 9:36 ` Charlie Shepherd
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.