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

[Markus Armbruster]

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)
[...]