[PATCH] coroutine: Make qemu_coroutine_self() return NULL if not in coroutine

[PATCH] coroutine: Make qemu_coroutine_self() return NULL if not in coroutine

[Alberto Faria]

qemu_coroutine_self() is used in several places outside of coroutine
context (mostly in qcow2 tracing calls).

Ensure qemu_coroutine_self() works properly when not called from
coroutine context, returning NULL in that case, and remove its
coroutine_fn annotation.

Signed-off-by: Alberto Faria <afaria@redhat.com>
---
 include/qemu/coroutine.h     |  5 +++--
 util/coroutine-sigaltstack.c |  4 ++--
 util/coroutine-ucontext.c    | 18 +++++++++---------
 util/coroutine-win32.c       |  9 +--------
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index e55b36f49a..0b9c8b8dac 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -95,9 +95,10 @@ void coroutine_fn qemu_coroutine_yield(void);
 AioContext *qemu_coroutine_get_aio_context(Coroutine *co);
 
 /**
- * Get the currently executing coroutine
+ * Get the currently executing coroutine, or NULL if not called from coroutine
+ * context
  */
-Coroutine *coroutine_fn qemu_coroutine_self(void);
+Coroutine *qemu_coroutine_self(void);
 
 /**
  * Return whether or not currently inside a coroutine
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index e2690c5f41..d9d90187a8 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -289,9 +289,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
 
 Coroutine *qemu_coroutine_self(void)
 {
-    CoroutineThreadState *s = coroutine_get_thread_state();
+    CoroutineThreadState *s = pthread_getspecific(thread_state_key);
 
-    return s->current;
+    return s && s->current->caller ? s->current : NULL;
 }
 
 bool qemu_in_coroutine(void)
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index ddc98fb4f8..d1dfe0dae5 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -320,18 +320,18 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
 Coroutine *qemu_coroutine_self(void)
 {
     Coroutine *self = get_current();
-    CoroutineUContext *leaderp = get_ptr_leader();
 
-    if (!self) {
-        self = &leaderp->base;
-        set_current(self);
-    }
+    if (self && self->caller) {
 #ifdef CONFIG_TSAN
-    if (!leaderp->tsan_co_fiber) {
-        leaderp->tsan_co_fiber = __tsan_get_current_fiber();
-    }
+        CoroutineUContext *leaderp = get_ptr_leader();
+        if (!leaderp->tsan_co_fiber) {
+            leaderp->tsan_co_fiber = __tsan_get_current_fiber();
+        }
 #endif
-    return self;
+        return self;
+    } else {
+        return NULL;
+    }
 }
 
 bool qemu_in_coroutine(void)
diff --git a/util/coroutine-win32.c b/util/coroutine-win32.c
index 7db2e8f8c8..97a593da7a 100644
--- a/util/coroutine-win32.c
+++ b/util/coroutine-win32.c
@@ -91,14 +91,7 @@ Coroutine *qemu_coroutine_self(void)
 {
     Coroutine *current = get_current();
 
-    if (!current) {
-        CoroutineWin32 *leader = get_ptr_leader();
-
-        current = &leader->base;
-        set_current(current);
-        leader->fiber = ConvertThreadToFiber(NULL);
-    }
-    return current;
+    return current && current->caller ? current : NULL;
 }
 
 bool qemu_in_coroutine(void)
-- 
2.37.3

Re: [PATCH] coroutine: Make qemu_coroutine_self() return NULL if not in coroutine

[Kevin Wolf]

Am 05.10.2022 um 16:20 hat Alberto Faria geschrieben:
> qemu_coroutine_self() is used in several places outside of coroutine
> context (mostly in qcow2 tracing calls).
> 
> Ensure qemu_coroutine_self() works properly when not called from
> coroutine context, returning NULL in that case, and remove its
> coroutine_fn annotation.
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>

The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I
think it already works outside of coroutine context, and consistently in
all three backends, by returning &leader.

Changing that to NULL makes me kind of nervous because the callers might
actually access the leader Coroutine object, and after this change they
would crash. (And even if they didn't crash, they wouldn't be able to
distinguish the leader coroutines of different threads any more.)

Do we have an actual reason to make this chance? That is, do we have any
case that was broken before?

Kevin

Re: [PATCH] coroutine: Make qemu_coroutine_self() return NULL if not in coroutine

[Alberto Campinho Faria]

On Wed, Oct 5, 2022 at 5:34 PM Kevin Wolf <kwolf@redhat.com> wrote:
> The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I
> think it already works outside of coroutine context, and consistently in
> all three backends, by returning &leader.
>
> Changing that to NULL makes me kind of nervous because the callers might
> actually access the leader Coroutine object, and after this change they
> would crash. (And even if they didn't crash, they wouldn't be able to
> distinguish the leader coroutines of different threads any more.)
>
> Do we have an actual reason to make this chance? That is, do we have any
> case that was broken before?

No, I just wasn't sure if the current implementations would return a
meaningful value when called from outside coroutine context, but it
seems they do. I'll send a patch only dropping the coroutine_fn
annotation.

Thanks,
Alberto

Re: [PATCH] coroutine: Make qemu_coroutine_self() return NULL if not in coroutine

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Wed, Oct 05, 2022 at 06:34:09PM +0200, Kevin Wolf wrote:
> Am 05.10.2022 um 16:20 hat Alberto Faria geschrieben:
> > qemu_coroutine_self() is used in several places outside of coroutine
> > context (mostly in qcow2 tracing calls).
> > 
> > Ensure qemu_coroutine_self() works properly when not called from
> > coroutine context, returning NULL in that case, and remove its
> > coroutine_fn annotation.
> > 
> > Signed-off-by: Alberto Faria <afaria@redhat.com>
> 
> The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I
> think it already works outside of coroutine context, and consistently in
> all three backends, by returning &leader.

Yes, I remember implementing it specifically so that NULL is never
returned.

The coroutine_fn annotation should be removed.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]