|
| From: | Fei Li |
| Subject: | Re: [Qemu-devel] [PATCH for-4.0 v8 6/7] qemu_thread_create: propagate the error to callers to handle |
| Date: | Fri, 21 Dec 2018 17:36:57 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 12/19/2018 08:14 PM, Fei Li wrote:
On 12/19/2018 06:10 PM, Markus Armbruster wrote:Fei Li <address@hidden> 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 <address@hidden> 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 areturn 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 <address@hidden> Cc: Daniel P. Berrangé <address@hidden> Cc: Dr. David Alan Gilbert <address@hidden> Signed-off-by: Fei Li <address@hidden> ---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 failuremodes set errno. You add a failure mode that doesn't set errno. That'sa 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. :)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 theAlso add a function comment. I suspect returning negated errno would lead to a shorter function comment.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. :)Yes, a definite wrong code.. :( Actually, pthread_attr_init() returns a nonzero errordiff --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.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
| [Prev in Thread] | Current Thread | [Next in Thread] |