> 在 2019年1月8日，01:18，Markus Armbruster <arm...@redhat.com> 写道：
>
> 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
Ok, thanks for the suggestion. :)
>
> 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?
For those have TODO, I polish them in the next following patches, and for those
do not have TODO I just let them use &error_abort.
>
> [...]
>> 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.
> */
Thanks so much for amending! :)
> Personally, I'd return negative errno instead of false, and dispense
> with setting errno.
Emm, I think I have replied this in last version, but due to several reasons I
did not wait for your feedback and sent the v9. Sorry for that.. And I like to
paste my two considerations here:
“- Actually only one caller needs the errno, that is the above
qemu_signalfd_compat().
- For the returning value, I remember there's once a email thread talking about
it: returning a bool (and let the passed errp hold the error message) is to
keep the consistency with glib.
”
So IMO I am wondering whether it is really needed to change the bool
(true/false) to int (0/-errno), just for that sole function:
qemu_signalfd_compat() which needs the errno. Besides if we return -errno, for
each caller we need add a local variable like “ret= qemu_thread_create()” to
store the -errno.
Have a nice day, thanks
Fei
>
>> +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)
> [...]
