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

[Achilles Benetopoulos <abenetopoulos@gmail.com>]

Thank you for the detailed review. The indentation errors mentioned
were pure carelesness on my part, sorry about that.

On 3/22/17 2:20 PM, Eric Blake wrote:
>> @@ -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(). >

I don't know about it being easier, but it does seem like it would
make the code at the call site cleaner, when the function calling
qemu_thread_create was passed an error variable itself. However, in
all but four cases there is no preexisting error variable, so the code
would stay pretty much the same.

>> +++ 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.

Well, I wrote it this way because of a recommendation in the error.h
header, but yes, if we were to change the return type of
qemu_thread_create, then this would make sense.

>> +++ 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.

Noted for future submissions.

>> +++ 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.

I was on the fence about this one. I wrote it this way thinking that
we would want to know the 'first' time the call to qemu_thread_create
failed, but proceed to try and create subsequent threads, which might
also fail. Seeing it now, it might be more reasonable to catch the
first error, report it then return?

>> +++ 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. >

OK, I will change the code in the tests.

Achilles