Fei Li <f...@suse.com> 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 <arm...@redhat.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Signed-off-by: Fei Li <f...@suse.com>
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) [...]