[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_ini

From: Fei Li
Subject: Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Date: Wed, 5 Sep 2018 12:17:24 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

Thanks for the review! :)

On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
Currently, when qemu_signal_init() fails it only returns a non-zero
value but without propagating any Error. But its callers need a
non-null err when runs error_report_err(err), or else 0->msg occurs.

To avoid such segmentation fault, add a new Error parameter to make
the call trace to propagate the err to the final caller.

Signed-off-by: Fei Li <address@hidden>
  include/qemu/osdep.h |  2 +-
  util/compatfd.c      | 17 ++++++++++++-----
  util/main-loop.c     | 11 +++++++----
  3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index a91068df0e..09ed85fcb8 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
                               additional fields in the future) */
-int qemu_signalfd(const sigset_t *mask);
+int qemu_signalfd(const sigset_t *mask, Error **errp);
  void sigaction_invoke(struct sigaction *action,
                        struct qemu_signalfd_siginfo *info);
diff --git a/util/compatfd.c b/util/compatfd.c
index 980bd33e52..65501de622 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> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
-static int qemu_signalfd_compat(const sigset_t *mask)
+static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
      struct sigfd_compat_info *info;
      QemuThread thread;
@@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
info = malloc(sizeof(*info));
      if (info == NULL) {
+        error_setg(errp, "Failed to malloc in %s", __func__);
I'd avoid using __func__ in error messages. Instead

    "Failed to allocate signalfd memory"
Ok, thanks.

          errno = ENOMEM;
          return -1;
if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create a pipe in %s", __func__);
     "Failed to create signalfd pipe"

          return -1;
@@ -94,17 +97,21 @@ static int qemu_signalfd_compat(const sigset_t *mask)
      return fds[0];
-int qemu_signalfd(const sigset_t *mask)
+int qemu_signalfd(const sigset_t *mask, Error **errp)
-#if defined(CONFIG_SIGNALFD)
      int ret;
+    Error *local_err = NULL;
+#if defined(CONFIG_SIGNALFD)
      ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
      if (ret != -1) {
          return ret;
-    return qemu_signalfd_compat(mask);
+    ret = qemu_signalfd_compat(mask, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
Using a local_err is not required - you can just pass errp stright
to qemu_signalfd_compat() and then check

    if (ret < 0)
For the use of a local error object & error_propagate call, I'd like to explain here. :) In our code, the initial caller passes two kinds of Error to the call trace, one is
something like &error_abort and &error_fatal, the other is NULL.

For the former, the exit() occurs in the functions where error_handle_fatal() is called (e.g. called by error_propagate/error_setg/...). The patch3: qemu_init_vcpu is the case, that means the system will exit in the final callee: qemu_thread_create(), instead of the initial caller pc_new_cpu(). In such case, I think propagating seems more reasonable.

For the latter, suppose we pass errp straightly. As NULL is passed, the error occurs in the final callee will automatically "propagating" to the initial caller, and let it handle this error. The patch1: qemu_signal_init is this case, let the caller of qemu_init_main_loop handle.
In such case, the error_propagate is indeed needless.

How do you think passing errp straightly for the latter case, and use a local error object & error_propagate for the former case? This is a distinct treatment, but would shorten the code.

Have a nice day, thanks again
+    return ret;
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..20f6ad3849 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -71,10 +71,11 @@ static void sigfd_handler(void *opaque)
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
      int sigfd;
      sigset_t set;
+    Error *local_err = NULL;
       * SIG_IPI must be blocked in the main thread and must not be caught
@@ -94,9 +95,10 @@ static int qemu_signal_init(void)
      pthread_sigmask(SIG_BLOCK, &set, NULL);
sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, &local_err);
      if (sigfd == -1) {
          fprintf(stderr, "failed to create signalfd\n");
+        error_propagate(errp, local_err);
          return -errno;
Again, no need for local_err - just pass errp stright in

@@ -109,7 +111,7 @@ static int qemu_signal_init(void) #else /* _WIN32 */ -static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
      return 0;
@@ -148,8 +150,9 @@ int qemu_init_main_loop(Error **errp)
init_clocks(qemu_timer_notify_cb); - ret = qemu_signal_init();
+    ret = qemu_signal_init(&local_error);
      if (ret) {
+        error_propagate(errp, local_error);
          return ret;
Again, no need for local_error.


reply via email to

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