Re: [Qemu-devel] [PATCH for-4.0 v9 06/16] qemu_thread: Make qemu_thread_create() handle errors properly

[fei]

> 在 2019年1月8日，01:18，Markus Armbruster <address@hidden> 写道：
> 
> 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
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)
> [...]