qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-4.0 v8 6/7] qemu_thread_create: propagate th


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]