Re: [Qemu-devel] [PATCH v3] qemu/thread: Add support for error reporting in qemu_thread_create

[Eric Blake]

On 03/21/2017 04:00 PM, Achilles Benetopoulos wrote:
> Failure during thread creation in qemu_thread_create does not force
> the program to exit anymore, since that isn't always the desired
> behaviour. The caller of qemu_thread_create is responsible for the
> error handling.
> 
> Signed-off-by: Achilles Benetopoulos <address@hidden>
> ---
>  cpus.c                      | 43 +++++++++++++++++++++++++++++++++++--------

When sending a new version, it's nice to summarize between the ---
separator and the diffstat what is different from the earlier version.


> +++ b/cpus.c
> @@ -1599,6 +1599,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>      char thread_name[VCPU_THREAD_NAME_SIZE];
>      static QemuCond *single_tcg_halt_cond;
>      static QemuThread *single_tcg_cpu_thread;
> +    Error *local_err = NULL;

We have a mix of 'err' and 'local_err'; if the shorter name makes it
easier to avoid 80-column lines, then that name might be worth using.

>  
>      if (qemu_tcg_mttcg_enabled() || !single_tcg_cpu_thread) {
>          cpu->thread = g_malloc0(sizeof(QemuThread));
> @@ -1612,14 +1613,25 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>                   cpu->cpu_index);
>  
>              qemu_thread_create(cpu->thread, thread_name, 
> qemu_tcg_cpu_thread_fn,
> -                               cpu, QEMU_THREAD_JOINABLE);
> +                    cpu, QEMU_THREAD_JOINABLE, &local_err);

The old indentation was correct, your change made it look wrong because
it is missing enough space.

> +
> +            if (local_err) {
> +                error_report_err(local_err);
> +                exit(1);
> +            }
>  
>          } else {
>              /* share a single thread for all cpus with TCG */
>              snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
> +
>              qemu_thread_create(cpu->thread, thread_name,

Why the blank line addition?

> -                               qemu_tcg_rr_cpu_thread_fn,
> -                               cpu, QEMU_THREAD_JOINABLE);
> +                    qemu_tcg_rr_cpu_thread_fn, cpu, QEMU_THREAD_JOINABLE,
> +                    &local_err);

Again, why are you changing correct indentation?  The
qemu_tc_rr_cpu_thread_fn line should not be touched.

> +
> +            if (local_err) {
> +                error_report_err(local_err);
> +                exit(1);
> +            }
>  
>              single_tcg_halt_cond = cpu->halt_cond;
>              single_tcg_cpu_thread = cpu->thread;
> @@ -1640,6 +1652,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>  static void qemu_hax_start_vcpu(CPUState *cpu)
>  {
>      char thread_name[VCPU_THREAD_NAME_SIZE];
> +    Error *local_err = NULL;
>  
>      cpu->thread = g_malloc0(sizeof(QemuThread));
>      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> @@ -1648,7 +1661,11 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
>      snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
>               cpu->cpu_index);
>      qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
> -                       cpu, QEMU_THREAD_JOINABLE);
> +                           cpu, QEMU_THREAD_JOINABLE, &local_err);

Indentation is wrong, now in the opposite direction (too much space).


> @@ -342,13 +343,19 @@ static void pci_edu_realize(PCIDevice *pdev, Error 
> **errp)
>  {
>      EduState *edu = DO_UPCAST(EduState, pdev, pdev);
>      uint8_t *pci_conf = pdev->config;
> +    Error *local_err = NULL;
>  
>      timer_init_ms(&edu->dma_timer, QEMU_CLOCK_VIRTUAL, edu_dma_timer, edu);
>  
>      qemu_mutex_init(&edu->thr_mutex);
>      qemu_cond_init(&edu->thr_cond);
>      qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
> -                       edu, QEMU_THREAD_JOINABLE);
> +                       edu, QEMU_THREAD_JOINABLE, &local_err);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Looking at code like this, I wonder if it would be easier to make
qemu_thread_create() return a value, rather than being void.  Then you
could write:

if (qemu_thread_create(..., errp) < 0) {
    return;
}

instead of having to futz around with local_err and error_propagate().


> +++ b/hw/usb/ccid-card-emulated.c
> @@ -34,6 +34,7 @@
>  
>  #include "qemu/thread.h"
>  #include "sysemu/char.h"
> +#include "qapi/error.h"
>  #include "ccid.h"
>  
>  #define DPRINTF(card, lvl, fmt, ...) \
> @@ -485,6 +486,7 @@ static int emulated_initfn(CCIDCardState *base)
>      EmulatedState *card = EMULATED_CCID_CARD(base);
>      VCardEmulError ret;
>      const EnumTable *ptable;
> +    Error *err = NULL, *local_err = NULL;

Huh? Why do you need two local error objects? One is generally sufficient.

>  
>      QSIMPLEQ_INIT(&card->event_list);
>      QSIMPLEQ_INIT(&card->guest_apdu_list);
> @@ -541,9 +543,17 @@ static int emulated_initfn(CCIDCardState *base)
>          return -1;
>      }
>      qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> -                       card, QEMU_THREAD_JOINABLE);
> +                       card, QEMU_THREAD_JOINABLE, &err);
> +
>      qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", 
> handle_apdu_thread,
> -                       card, QEMU_THREAD_JOINABLE);
> +                       card, QEMU_THREAD_JOINABLE, &local_err);
> +    error_propagate(&err, local_err);
> +
> +    if (err) {
> +        error_report_err(err);
> +        return -1;
> +    }


If you used the return value, you could write:

if (qemu_thread_create(..., &err) < 0 ||
    qemu_thread_create(..., &err) < 0) {
    error_report_err(err);
    return -1;
}

without needing the second object.

> +++ b/include/qemu/thread.h
> @@ -55,7 +55,7 @@ void qemu_event_destroy(QemuEvent *ev);
>  
>  void qemu_thread_create(QemuThread *thread, const char *name,
>                          void *(*start_routine)(void *),
> -                        void *arg, int mode);
> +                        void *arg, int mode, Error **errp);

Hmm, we still haven't made it official recommended practice, but it's a
good idea to use 'git config diff.orderFile /path/to/file' in order to
hoist .h changes to the front of a diff (it makes diffs easier to review
when you see the interface change prior to the clients of the
interface).  I wish I had a better URL to point to on the topic, but
didn't want to spend time finding it in the mailing list archives at
this time.

> +++ b/iothread.c
> @@ -132,7 +132,14 @@ static void iothread_complete(UserCreatable *obj, Error 
> **errp)
>      name = object_get_canonical_path_component(OBJECT(obj));
>      thread_name = g_strdup_printf("IO %s", name);
>      qemu_thread_create(&iothread->thread, thread_name, iothread_run,
> -                       iothread, QEMU_THREAD_JOINABLE);
> +                           iothread, QEMU_THREAD_JOINABLE, &local_error);

Indentation problems continue to be a theme. I'll quit pointing them
out, but expect that you'll fix all of theme.


> +++ b/migration/ram.c
> @@ -357,6 +357,7 @@ void migrate_compress_threads_join(void)
>  void migrate_compress_threads_create(void)
>  {
>      int i, thread_count;
> +    Error *err = NULL, *local_err = NULL;
>  
>      if (!migrate_use_compression()) {
>          return;
> @@ -378,7 +379,16 @@ void migrate_compress_threads_create(void)
>          qemu_cond_init(&comp_param[i].cond);
>          qemu_thread_create(compress_threads + i, "compress",
>                             do_data_compress, comp_param + i,
> -                           QEMU_THREAD_JOINABLE);
> +                           QEMU_THREAD_JOINABLE, &local_err);
> +
> +        if (local_err) {
> +            error_propagate(&err, local_err);
> +            local_err = NULL;
> +        }
> +    }
> +
> +    if (err) {
> +        error_report_err(err);
>      }

Another place that looks weird with two error variables.

> +++ b/tests/iothread.c
> @@ -69,11 +69,18 @@ void iothread_join(IOThread *iothread)
>  IOThread *iothread_new(void)
>  {
>      IOThread *iothread = g_new0(IOThread, 1);
> +    Error *local_err = NULL;
>  
>      qemu_mutex_init(&iothread->init_done_lock);
>      qemu_cond_init(&iothread->init_done_cond);
>      qemu_thread_create(&iothread->thread, NULL, iothread_run,
> -                       iothread, QEMU_THREAD_JOINABLE);
> +                       iothread, QEMU_THREAD_JOINABLE, &local_err);

In a test, you can just assert success, by using:

qemu_thread_create(..., &error_abort);

> +
> +    if (local_err) {
> +        error_report_err(local_err);
> +        /*what makes sense here as a return value?*/
> +        return NULL;

Doing that will get rid of this fishy comment.

> +++ b/tests/rcutorture.c
> @@ -64,6 +64,7 @@
>  #include "qemu/atomic.h"
>  #include "qemu/rcu.h"
>  #include "qemu/thread.h"
> +#include "qapi/error.h"
>  
>  long long n_reads = 0LL;
>  long n_updates = 0L;
> @@ -85,12 +86,20 @@ static int n_threads;
>  
>  static void create_thread(void *(*func)(void *))
>  {
> +    Error *local_err = NULL;
> +
>      if (n_threads >= NR_THREADS) {
>          fprintf(stderr, "Thread limit of %d exceeded!\n", NR_THREADS);
>          exit(-1);
>      }
>      qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads],
> -                       QEMU_THREAD_JOINABLE);
> +                       QEMU_THREAD_JOINABLE, &local_err);
> +
> +    if (local_err) {
> +        error_report_err(local_err);
> +        exit(1);

Again, in a test, if you're just going to exit anyway, then it's easier
to pass &error_abort to the original call, than it is to post-process
and report the error.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature