Re: [Qemu-devel] [PATCH for-4.0 v8 6/7] qemu_thread_create: propagate the error to callers to handle

[Fei Li <fli@suse.com>]

On 12/19/2018 08:14 PM, Fei Li wrote:
>
> On 12/19/2018 06:10 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> On 12/13/2018 03:26 PM, Markus Armbruster wrote:
>>>> There's a question for David Gibson inline.  Please search for /ppc/.
>>>>
>>>> Fei Li <fli@suse.com> writes:
>>>>
>>>>> Make qemu_thread_create() return a Boolean to indicate if it succeeds
>>>>> rather than failing with an error. And add an Error parameter to hold
>>>>> the error message and let the callers handle it.
>>>> The "rather than failing with an error" is misleading. Before the
>>>> patch, we report to stderr and abort().  What about:
>>>>
>>>>       qemu-thread: Make qemu_thread_create() handle errors properly
>>>>
>>>>       qemu_thread_create() abort()s on error.  Not nice. Give it a
>>>>       return value and an Error ** argument, so it can return 
>>>> success /
>>>>       failure.
>>> A nice commit-amend! Thanks!
>>>> Still missing from the commit message then: how you update the 
>>>> callers.
>>> Yes, agree. I think the-how should also be noted here, like
>>> - propagating the err to callers whose call trace already have the
>>> Error paramater;
>>> - just add an &error_abort for qemu_thread_create() and make it a
>>> "TODO: xxx";
>>>> Let's see below.
>>>>
>>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>>> Cc: Daniel P. Berrangé <berrange@redhat.com>
>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>>> ---
>>>>>    cpus.c                      | 45 
>>>>> ++++++++++++++++++++++++-------------
>>>>>    dump.c                      |  6 +++--
>>>>>    hw/misc/edu.c               |  6 +++--
>>>>>    hw/ppc/spapr_hcall.c        | 10 +++++++--
>>>>>    hw/rdma/rdma_backend.c      |  4 +++-
>>>>>    hw/usb/ccid-card-emulated.c | 16 ++++++++++----
>>>>>    include/qemu/thread.h       |  4 ++--
>>>>>    io/task.c                   |  3 ++-
>>>>>    iothread.c                  | 16 +++++++++-----
>>>>>    migration/migration.c       | 54 
>>>>> +++++++++++++++++++++++++++++----------------
>>>>>    migration/postcopy-ram.c    | 14 ++++++++++--
>>>>>    migration/ram.c             | 40 ++++++++++++++++++++++++---------
>>>>>    migration/savevm.c          | 11 ++++++---
>>>>>    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               | 17 +++++++++-----
>>>>>    ui/vnc-jobs.h               |  2 +-
>>>>>    ui/vnc.c                    |  4 +++-
>>>>>    util/compatfd.c             | 12 ++++++++--
>>>>>    util/oslib-posix.c          | 17 ++++++++++----
>>>>>    util/qemu-thread-posix.c    | 24 +++++++++++++-------
>>>>>    util/qemu-thread-win32.c    | 16 ++++++++++----
>>>>>    util/rcu.c                  |  3 ++-
>>>>>    util/thread-pool.c          |  4 +++-
>>>>>    28 files changed, 243 insertions(+), 101 deletions(-)
>>>>>
...snip, and only leave the three uncertain small topics...
>>>
>>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>>> index 658dfa88a3..6e0cccf066 100644
>>>>> --- a/migration/ram.c
>>>>> +++ b/migration/ram.c
>>>>> @@ -473,6 +473,7 @@ static void compress_threads_save_cleanup(void)
>>>>>    static int compress_threads_save_setup(void)
>>>>>    {
>>>>>        int i, thread_count;
>>>>> +    Error *local_err = NULL;
>>>>>          if (!migrate_use_compression()) {
>>>>>            return 0;
>>>>> @@ -502,9 +503,12 @@ static int compress_threads_save_setup(void)
>>>>>            comp_param[i].quit = false;
>>>>>            qemu_mutex_init(&comp_param[i].mutex);
>>>>>            qemu_cond_init(&comp_param[i].cond);
>>>>> -        qemu_thread_create(compress_threads + i, "compress",
>>>>> -                           do_data_compress, comp_param + i,
>>>>> -                           QEMU_THREAD_JOINABLE);
>>>>> +        if (!qemu_thread_create(compress_threads + i, "compress",
>>>>> +                                do_data_compress, comp_param + i,
>>>>> +                                QEMU_THREAD_JOINABLE, &local_err)) {
>>>>> +            error_reportf_err(local_err, "failed to create 
>>>>> do_data_compress: ");
>>>>> +            goto exit;
>>>>> +        }
>>>>>        }
>>>>>        return 0;
>>>> Reviewing the migration changes is getting tiresome...
>>> Yes, indeed, the migration involves a lot! Thanks so much for helping
>>> to review!
>>>>    Is reporting the
>>>> error appropriate here, and why?
>>> I think the qemu monitor should display the obvious and exact failing
>>> reason for administrators, esp considering that qemu_thread_create()
>>> itself does not print any message thus we have no idea which direct
>>> function fails if gdb is not enabled.
>>> IOW, I think David's answer to that ppc's error_reportf_err() also
>>> apply here:
>>>
>>> "The error returns are for the guest, the reported errors are for the
>>> guest administrator or management layers."
>> There could well be an issue with the "management layers" part. Should
>> this error be sent to the management layer via QMP somehow? Migration
>> maintainers should be able to assist with this question.
Kindly ping migration maintainers. :)
>
>>>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>>>> index 980bd33e52..886aa249f9 100644
>>>>> --- a/util/compatfd.c
>>>>> +++ b/util/compatfd.c
>>>>> @@ -16,6 +16,7 @@
>>>>>    #include "qemu/osdep.h"
>>>>>    #include "qemu-common.h"
>>>>>    #include "qemu/thread.h"
>>>>> +#include "qapi/error.h"
>>>>>      #include <sys/syscall.h>
>>>>>    @@ -70,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t
>>>>> *mask)
>>>>>        struct sigfd_compat_info *info;
>>>>>        QemuThread thread;
>>>>>        int fds[2];
>>>>> +    Error *local_err = NULL;
>>>>>          info = malloc(sizeof(*info));
>>>>>        if (info == NULL) {
>>>>> @@ -88,8 +90,14 @@ static int qemu_signalfd_compat(const sigset_t 
>>>>> *mask)
>>>>>        memcpy(&info->mask, mask, sizeof(*mask));
>>>>>        info->fd = fds[1];
>>>>>    -    qemu_thread_create(&thread, "signalfd_compat",
>>>>> sigwait_compat, info,
>>>>> -                       QEMU_THREAD_DETACHED);
>>>>> +    if (!qemu_thread_create(&thread, "signalfd_compat", 
>>>>> sigwait_compat,
>>>>> +                            info, QEMU_THREAD_DETACHED, 
>>>>> &local_err)) {
>>>>> +        error_reportf_err(local_err, "failed to create 
>>>>> sigwait_compat: ");
>>>>> +        close(fds[0]);
>>>>> +        close(fds[1]);
>>>>> +        free(info);
>>>>> +        return -1;
>>>>> +    }
>>>>>          return fds[0];
>>>>>    }
>>>> This function is implements signalfd() when the kernel doesn't provide
>>>> it.
>>>>
>>>> signalfd() sets errno on failure.  The replacement's existing failure
>>>> modes set errno.  You add a failure mode that doesn't set errno.  
>>>> That's
>>>> a bug.  To fix it, you can either make qemu_thread_create() set errno,
>>>> or you can make it return a value you can use to set errno. The common
>>>> way to do the latter is returning a *negated* errno value.
>>> Oops, I forgot setting the errno for Linux implementation! My fault..
>>> I will set errno inside qemu_thread_create() as follows:
>>>       err = pthread_attr_init(&attr);
>>>       if (err) {
>>> -        error_setg_errno(errp, -err, "pthread_attr_init failed: %s",
>>> -                         strerror(err));
>>> +        errno = err;
>>> +        error_setg_errno(errp, errno, "pthread_attr_init failed");
>>>           return false;
>>>       }
>> Make sure to set errno on all failures, not just this one.
> Actually, this code update is changed for qemu_thread_create() itself,
> I think if the errno is set in this function, no callers' errno need 
> to be set.
> Please correct me if I understand wrong. :)
>> Also add a function comment.  I suspect returning negated errno would
>> lead to a shorter function comment.
> 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. IMO, returning a bool or returning the -errno 
> is equal to
> me if we do not use the return value again in the callers, it just 
> involves the
> judgement. But if we want to reuse the return value, like:
>   ret = qemu_thread_create(xx, xx, &local_err);
> I do not think it is much needed. What do you think?
One place needs to be confirmed. :)
>>   Yet another reason to write
>> function comments!  Making myself document the mess I made has made me
>> clean it up before I submit it many times :)
> Ok, thanks for the experience. Will add the comment. :)
>>
>>>>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>>>>> index 865e476df5..81b40a1ece 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,9 @@ 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)
>>>>> +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 +512,9 @@ void qemu_thread_create(QemuThread *thread, 
>>>>> const char *name,
>>>>>          err = pthread_attr_init(&attr);
>>>>>        if (err) {
>>>>> -        error_exit(err, __func__);
>>>>> +        error_setg_errno(errp, -err, "pthread_attr_init failed: %s",
>>>>> +                         strerror(err));
>> -err is actually wrong: pthread_attr_init() returns a *positive* errno
>> code on failure.
> Yes, a definite wrong code.. :( Actually, pthread_attr_init() returns 
> a nonzero error
> number, thus I do the below update by assigning the return err to errno.
>
>      err = pthread_attr_init(&attr);
>      if (err) {
> -        error_exit(err, __func__);
> +        errno = err;
> +        error_setg_errno(errp, errno, "pthread_attr_init failed");
> +        return false;
>      }
>
Another place needs to be confirmed. :)
>
Have a nice day, thanks
Fei