Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Stefan Weil <sw@weilnetz.de>]

Am 22.06.2015 um 23:54 schrieb Zavadovsky Yan:
> Calling SuspendThread() is not enough to suspend Win32 thread.
> We need to call GetThreadContext() after SuspendThread()
> to make sure that OS have really suspended target thread.
> But GetThreadContext() needs for THREAD_GET_CONTEXT
> access right on thread object.
>
> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
> and change 'while(GetThreadContext() == SUCCESS)' to
> 'while(GetThreadContext() == FAILED)'.
> So this 'while' loop will stop only after successful grabbing
> of thread context(i.e. when thread is really suspended).
> Not after the one failed GetThreadContext() call.
>
> Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
> ---
>   cpus.c                   | 2 +-
>   util/qemu-thread-win32.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index b85fb5f..83d5eb5 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>            * suspended until we can get the context.
>            */
>           tcgContext.ContextFlags = CONTEXT_CONTROL;
> -        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
> +        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>               continue;
>           }
>   
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 406b52f..823eca1 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>   
>       EnterCriticalSection(&data->cs);
>       if (!data->exited) {
> -        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
> -                            thread->tid);
> +        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME | THREAD_GET_CONTEXT,
> +                            FALSE, thread->tid);
>       } else {
>           handle = NULL;
>       }


I added the contributers of the original code to the cc list.

The modifications look reasonable - if GetThreadContext is needed at all.
We should add an URL to reliable documentation which supports that
claim.

Is it a good idea to run a busy waiting loop? Or would a Sleep(0) in
the loop be better (it allows other threads to run, maybe it helps
them to suspend, too).

Regards
Stefan