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
