[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.0 v9 06/16] qemu_thread: Make qemu_thread_
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-4.0 v9 06/16] qemu_thread: Make qemu_thread_create() handle errors properly |
Date: |
Mon, 07 Jan 2019 18:18:35 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Fei Li <address@hidden> writes:
> qemu_thread_create() abort()s on error. Not nice. Give it a return
> value and an Error ** argument, so it can return success/failure.
>
> Considering qemu_thread_create() is quite widely used in qemu, split
> this into two steps: this patch passes the &error_abort to
> qemu_thread_create() everywhere, and the next 9 patches will improve
> on &error_abort for callers who need.
>
> Cc: Markus Armbruster <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Fei Li <address@hidden>
The commit message's title promises more than the patch delivers.
Suggest:
qemu_thread: Make qemu_thread_create() take Error ** argument
The rest of the commit message is fine.
> ---
> cpus.c | 23 +++++++++++++++--------
> dump.c | 3 ++-
> hw/misc/edu.c | 4 +++-
> hw/ppc/spapr_hcall.c | 4 +++-
> hw/rdma/rdma_backend.c | 3 ++-
> hw/usb/ccid-card-emulated.c | 5 +++--
> include/qemu/thread.h | 4 ++--
> io/task.c | 3 ++-
> iothread.c | 3 ++-
> migration/migration.c | 11 ++++++++---
> migration/postcopy-ram.c | 4 +++-
> migration/ram.c | 12 ++++++++----
> migration/savevm.c | 3 ++-
> tests/atomic_add-bench.c | 3 ++-
> tests/iothread.c | 2 +-
> tests/qht-bench.c | 3 ++-
> tests/rcutorture.c | 3 ++-
> tests/test-aio.c | 2 +-
> tests/test-rcu-list.c | 3 ++-
> ui/vnc-jobs.c | 6 ++++--
> util/compatfd.c | 6 ++++--
> util/oslib-posix.c | 3 ++-
> util/qemu-thread-posix.c | 27 ++++++++++++++++++++-------
> util/qemu-thread-win32.c | 16 ++++++++++++----
> util/rcu.c | 3 ++-
> util/thread-pool.c | 4 +++-
> 26 files changed, 112 insertions(+), 51 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 0ddeeefc14..25df03326b 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
> cpu->cpu_index);
>
> + /* TODO: let the callers handle the error instead of abort()
> here */
> qemu_thread_create(cpu->thread, thread_name,
> qemu_tcg_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE);
> + cpu, QEMU_THREAD_JOINABLE, &error_abort);
>
> } else {
> /* share a single thread for all cpus with TCG */
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
> + /* TODO: let the callers handle the error instead of abort()
> here */
> qemu_thread_create(cpu->thread, thread_name,
> qemu_tcg_rr_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE);
> + cpu, QEMU_THREAD_JOINABLE, &error_abort);
>
> single_tcg_halt_cond = cpu->halt_cond;
> single_tcg_cpu_thread = cpu->thread;
You add this TODO comment to 24 out of 37 calls. Can you give your
reasons for adding it to some calls, but not to others?
[...]
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index 55d83a907c..12291f4ccd 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
> void qemu_event_wait(QemuEvent *ev);
> void qemu_event_destroy(QemuEvent *ev);
>
> -void qemu_thread_create(QemuThread *thread, const char *name,
> +bool qemu_thread_create(QemuThread *thread, const char *name,
> void *(*start_routine)(void *),
> - void *arg, int mode);
> + void *arg, int mode, Error **errp);
> void *qemu_thread_join(QemuThread *thread);
> void qemu_thread_get_self(QemuThread *thread);
> bool qemu_thread_is_self(QemuThread *thread);
[...]
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 865e476df5..39834b0551 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -15,6 +15,7 @@
> #include "qemu/atomic.h"
> #include "qemu/notify.h"
> #include "qemu-thread-common.h"
> +#include "qapi/error.h"
>
> static bool name_threads;
>
> @@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
> return r;
> }
>
> -void qemu_thread_create(QemuThread *thread, const char *name,
> - void *(*start_routine)(void*),
> - void *arg, int mode)
> +/*
> + * Return a boolean: true/false to indicate whether it succeeds.
> + * If fails, propagate the error to Error **errp and set the errno.
> + */
Let's write something that can pass as a function contract:
/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/
Personally, I'd return negative errno instead of false, and dispense
with setting errno.
> +bool qemu_thread_create(QemuThread *thread, const char *name,
> + void *(*start_routine)(void *),
> + void *arg, int mode, Error **errp)
> {
> sigset_t set, oldset;
> int err;
> @@ -511,7 +516,9 @@ void qemu_thread_create(QemuThread *thread, const char
> *name,
>
> err = pthread_attr_init(&attr);
> if (err) {
> - error_exit(err, __func__);
> + errno = err;
> + error_setg_errno(errp, errno, "pthread_attr_init failed");
> + return false;
> }
>
> if (mode == QEMU_THREAD_DETACHED) {
> @@ -529,13 +536,19 @@ void qemu_thread_create(QemuThread *thread, const char
> *name,
>
> err = pthread_create(&thread->thread, &attr,
> qemu_thread_start, qemu_thread_args);
> -
> - if (err)
> - error_exit(err, __func__);
> + if (err) {
> + errno = err;
> + error_setg_errno(errp, errno, "pthread_create failed");
> + pthread_attr_destroy(&attr);
> + g_free(qemu_thread_args->name);
> + g_free(qemu_thread_args);
> + return false;
> + }
>
> pthread_sigmask(SIG_SETMASK, &oldset, NULL);
>
> pthread_attr_destroy(&attr);
> + return true;
> }
>
> void qemu_thread_get_self(QemuThread *thread)
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 4a363ca675..57b1143e97 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -20,6 +20,7 @@
> #include "qemu/thread.h"
> #include "qemu/notify.h"
> #include "qemu-thread-common.h"
> +#include "qapi/error.h"
> #include <process.h>
>
> static bool name_threads;
> @@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
> return ret;
> }
>
> -void qemu_thread_create(QemuThread *thread, const char *name,
> - void *(*start_routine)(void *),
> - void *arg, int mode)
> +bool qemu_thread_create(QemuThread *thread, const char *name,
> + void *(*start_routine)(void *),
> + void *arg, int mode, Error **errp)
> {
> HANDLE hThread;
> struct QemuThreadData *data;
> @@ -409,10 +410,17 @@ void qemu_thread_create(QemuThread *thread, const char
> *name,
> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
> data, 0, &thread->tid);
> if (!hThread) {
> - error_exit(GetLastError(), __func__);
> + if (data->mode != QEMU_THREAD_DETACHED) {
> + DeleteCriticalSection(&data->cs);
> + }
> + error_setg_errno(errp, errno,
> + "failed to create win32_start_routine");
> + g_free(data);
> + return false;
> }
> CloseHandle(hThread);
> thread->data = data;
> + return true;
> }
>
> void qemu_thread_get_self(QemuThread *thread)
[...]
- Re: [Qemu-devel] [PATCH for-4.0 v9 06/16] qemu_thread: Make qemu_thread_create() handle errors properly,
Markus Armbruster <=