qemu_thread_create: propagate errors to callers to check

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 include/qemu/osdep.h | 2 +-
 util/compatfd.c      | 9 ++++++---
 util/main-loop.c     | 9 ++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -1

index 4f8559e550..f1f56763a0 100644
@@ -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);
 #endif

util/compatfd.c +5 -2

index 980bd33e52..d3ed890405 100644
@@ -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 allocate signalfd memory");
         errno = ENOMEM;
         return -1;
     }
 
     if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
         free(info);
         return -1;
     }

@@ -94,7 +97,7 @@ 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;

util/main-loop.c +1 -1

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
     }
 }
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
 {
     int sigfd;
     sigset_t set;

util/main-loop.c +2 -2

@@ -109,7 +108,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,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
 
     init_clocks(qemu_timer_notify_cb);
 
-    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
     if (ret) {
         return ret;
     }

Quoted 3 lines · Fei Li

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.

The bug is in qemu_init_main_loop():

     ret = qemu_signal_init();
     if (ret) {
         return ret;
     }

Fails without setting an error, unlike the other failures. Its callers crash then.

Quoted 69 lines

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  include/qemu/osdep.h | 2 +-
  util/compatfd.c      | 9 ++++++---
  util/main-loop.c     | 9 ++++-----
  3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -1

index 4f8559e550..f1f56763a0 100644
@@ -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);
  #endif

util/compatfd.c +5 -2

index 980bd33e52..d3ed890405 100644
@@ -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 allocate signalfd memory");
          errno = ENOMEM;
          return -1;
      }
 
      if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
          free(info);
          return -1;
      }

@@ -94,7 +97,7 @@ 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;

util/compatfd.c int qemu_signalfd(const sigset_t *mask) · line 113

Quoted 6 lines

util/compatfd.c +1 -1

@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
      }
  #endif
 
-    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
  }

I think this takes the Error conversion too far.

qemu_signalfd() is like the signalfd() system call, only portable, and setting FD_CLOEXEC. In particular, it reports failure just like a system call: it sets errno and returns -1. I'd prefer to keep it that way. Instead...

Quoted 14 lines

util/main-loop.c +1 -1

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
      }
  }
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
  {
      int sigfd;
      sigset_t set;

util/main-loop.c static int qemu_signal_init(void) · line 101

Quoted 10 lines

util/main-loop.c +1 -2

@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
      pthread_sigmask(SIG_BLOCK, &set, NULL);
 
      sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
      if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
          return -errno;
      }
 

... change this function so:

        pthread_sigmask(SIG_BLOCK, &set, NULL);
    
        sigdelset(&set, SIG_IPI);
        sigfd = qemu_signalfd(&set);
        if (sigfd == -1) {
   -        fprintf(stderr, "failed to create signalfd\n");
   +        error_setg_errno(errp, errno, "failed to create signalfd");
            return -errno;
        }

Does this make sense?

Quoted 18 lines

util/main-loop.c +2 -2

@@ -109,7 +108,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,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
 
      init_clocks(qemu_timer_notify_cb);
 
-    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
      if (ret) {
          return ret;
      }

Quoted 7 lines · Markus Armbruster

Quoted 3 lines · Fei Li

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.

The bug is in qemu_init_main_loop():

Quoted 7 lines

     ret = qemu_signal_init();
     if (ret) {
         return ret;
     }

Fails without setting an error, unlike the other failures. Its callers crash then.

Thanks!

Quoted 107 lines

Quoted 62 lines

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  include/qemu/osdep.h | 2 +-
  util/compatfd.c      | 9 ++++++---
  util/main-loop.c     | 9 ++++-----
  3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -0

index 4f8559e550..f1f56763a0 100644
@@ -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);
  #endif

util/compatfd.c +1 -0

index 980bd33e52..d3ed890405 100644
@@ -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)

util/compatfd.c +4 -0

+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 allocate signalfd memory");
          errno = ENOMEM;
          return -1;
      }
        if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
          free(info);
          return -1;
      }

@@ -94,7 +97,7 @@ 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;

util/compatfd.c int qemu_signalfd(const sigset_t *mask) · line 113

Quoted 5 lines

util/compatfd.c +1 -0

@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
      }
  #endif
  -    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
  }

I think this takes the Error conversion too far.

qemu_signalfd() is like the signalfd() system call, only portable, and setting FD_CLOEXEC. In particular, it reports failure just like a system call: it sets errno and returns -1. I'd prefer to keep it that way. Instead...

Quoted 13 lines

util/main-loop.c +1 -0

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
      }
  }
  -static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
  {
      int sigfd;
      sigset_t set;

util/main-loop.c static int qemu_signal_init(void) · line 100

Quoted 9 lines

util/main-loop.c +1 -2

@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
      pthread_sigmask(SIG_BLOCK, &set, NULL);
        sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
      if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
          return -errno;
      }
 

... change this function so:

        pthread_sigmask(SIG_BLOCK, &set, NULL);
            sigdelset(&set, SIG_IPI);
        sigfd = qemu_signalfd(&set);
        if (sigfd == -1) {
   -        fprintf(stderr, "failed to create signalfd\n");
   +        error_setg_errno(errp, errno, "failed to create signalfd");
            return -errno;
        }

Quoted 1 line

Does this make sense?

Yes, it looks more concise if we only have this patch, but triggers one errno-set
issue after applying patch 7/7, which adds a Error **errp parameter for qemu_thread_create() to let its callers handle the error instead of exit() directly
to add the robustness.

For the patch series' current implementation, the  modified qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and set the
error reason into the passed errp, and did not set the errno. Actually another
similar errno-set issue has been talked in last patch. :) If we set the errno in future qemu_thread_create(), we need to distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to set errno.
But for Windows, I am not sure if we can use

"errno = GetLastError()"

to set errno, as this seems a little weird. Do you have any idea about this?

BTW, if we have a decent errno-set way for Windows, I will adopt your above proposal for this patch.

Have a nice day, thanks for the review :)
Fei

Quoted 15 lines

Quoted 14 lines

util/main-loop.c +2 -0

@@ -109,7 +108,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,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
        init_clocks(qemu_timer_notify_cb);
  -    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
      if (ret) {
          return ret;
      }

Quoted 13 lines · Fei Li

Quoted 12 lines · Markus Armbruster

Quoted 3 lines · Fei Li

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.

The bug is in qemu_init_main_loop():

      ret = qemu_signal_init();
      if (ret) {
          return ret;
      }

Quoted 3 lines

Quoted 2 lines

Fails without setting an error, unlike the other failures. Its callers crash then.

Thanks!

Consider working that into your commit message.

Quoted 109 lines

Quoted 109 lines

Quoted 63 lines

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
   include/qemu/osdep.h | 2 +-
   util/compatfd.c      | 9 ++++++---
   util/main-loop.c     | 9 ++++-----
   3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -0

index 4f8559e550..f1f56763a0 100644
@@ -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);
   #endif

util/compatfd.c +1 -0

index 980bd33e52..d3ed890405 100644
@@ -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)

util/compatfd.c +4 -0

+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 allocate signalfd memory");
           errno = ENOMEM;
           return -1;
       }
         if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
           free(info);
           return -1;
       }

@@ -94,7 +97,7 @@ 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;

util/compatfd.c int qemu_signalfd(const sigset_t *mask) · line 113

Quoted 5 lines

util/compatfd.c +1 -0

@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
       }
   #endif
   -    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
   }

I think this takes the Error conversion too far.

qemu_signalfd() is like the signalfd() system call, only portable, and setting FD_CLOEXEC. In particular, it reports failure just like a system call: it sets errno and returns -1. I'd prefer to keep it that way. Instead...

Quoted 13 lines

util/main-loop.c +1 -0

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
       }
   }
   -static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
   {
       int sigfd;
       sigset_t set;

util/main-loop.c static int qemu_signal_init(void) · line 100

Quoted 9 lines

util/main-loop.c +1 -2

@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
       pthread_sigmask(SIG_BLOCK, &set, NULL);
         sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
       if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
           return -errno;
       }
 

... change this function so:

         pthread_sigmask(SIG_BLOCK, &set, NULL);
             sigdelset(&set, SIG_IPI);
         sigfd = qemu_signalfd(&set);
         if (sigfd == -1) {
    -        fprintf(stderr, "failed to create signalfd\n");
    +        error_setg_errno(errp, errno, "failed to create signalfd");
             return -errno;
         }

Does this make sense?

Quoted 6 lines

Yes, it looks more concise if we only have this patch, but triggers one errno-set
issue after applying patch 7/7, which adds a Error **errp parameter for qemu_thread_create() to let its callers handle the error instead of exit() directly
to add the robustness.

I guess you're talking about the qemu_thread_create() in qemu_signalfd_compat(). Correct?

Quoted 14 lines

For the patch series' current implementation, the  modified qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and set the
error reason into the passed errp, and did not set the errno. Actually another
similar errno-set issue has been talked in last patch. :) If we set the errno in future qemu_thread_create(), we need to distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to set errno.
But for Windows, I am not sure if we can use

Quoted 1 line

"errno = GetLastError()"

No, that won't work.

Quoted 4 lines

to set errno, as this seems a little weird. Do you have any idea about this?

BTW, if we have a decent errno-set way for Windows, I will adopt your above proposal for this patch.

According to MS docs, _beginthreadex() sets errno on failure:

If successful, each of these functions returns a handle to the newly created thread; however, if the newly created thread exits too quickly, _beginthread might not return a valid handle. (See the discussion in the Remarks section.) On an error, _beginthread returns -1L, and errno is set to EAGAIN if there are too many threads, to EINVAL if the argument is invalid or the stack size is incorrect, or to EACCES if there are insufficient resources (such as memory). On an error, _beginthreadex returns 0, and errno and _doserrno are set.

https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex

Looks like the current code's use of GetLastError() after _beginthreadex() failure is wrong.

Fix that, and both qemu_thread_create() implementations set errno on failure, which in turn lets you make qemu_signalfd_compat() and thus qemu_signalfd() behave sanely regardless of which qemu_thread_create() implementation they use below the hood.

Quoted 17 lines

Have a nice day, thanks for the review :)
Fei

Quoted 14 lines

Quoted 14 lines

util/main-loop.c +2 -0

@@ -109,7 +108,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,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
         init_clocks(qemu_timer_notify_cb);
   -    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
       if (ret) {
           return ret;
       }

Quoted 17 lines · Markus Armbruster

Quoted 15 lines · Fei Li

Quoted 14 lines · Markus Armbruster

Quoted 3 lines · Fei Li

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.

The bug is in qemu_init_main_loop():

      ret = qemu_signal_init();
      if (ret) {
          return ret;
      }

Fails without setting an error, unlike the other failures. Its callers crash then.

Quoted 2 lines

Quoted 1 line

Thanks!

Consider working that into your commit message.

Ok. I'd like to reword as follows:
Currently in qemu_init_main_loop(), qemu_signal_init() fails without setting an error
like the other failures. Its callers crash then when runs error_report_err(err).

Quoted 109 lines

Quoted 109 lines

Quoted 109 lines

Quoted 63 lines

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
   include/qemu/osdep.h | 2 +-
   util/compatfd.c      | 9 ++++++---
   util/main-loop.c     | 9 ++++-----
   3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -0

index 4f8559e550..f1f56763a0 100644
@@ -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);
   #endif

util/compatfd.c +1 -0

index 980bd33e52..d3ed890405 100644
@@ -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)

util/compatfd.c +4 -0

+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 allocate signalfd memory");
           errno = ENOMEM;
           return -1;
       }
         if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
           free(info);
           return -1;
       }

@@ -94,7 +97,7 @@ 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;

util/compatfd.c int qemu_signalfd(const sigset_t *mask) · line 113

Quoted 5 lines

util/compatfd.c +1 -0

@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
       }
   #endif
   -    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
   }

I think this takes the Error conversion too far.

qemu_signalfd() is like the signalfd() system call, only portable, and setting FD_CLOEXEC. In particular, it reports failure just like a system call: it sets errno and returns -1. I'd prefer to keep it that way. Instead...

Quoted 13 lines

util/main-loop.c +1 -0

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
       }
   }
   -static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
   {
       int sigfd;
       sigset_t set;

util/main-loop.c static int qemu_signal_init(void) · line 100

Quoted 9 lines

util/main-loop.c +1 -2

@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
       pthread_sigmask(SIG_BLOCK, &set, NULL);
         sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
       if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
           return -errno;
       }
 

... change this function so:

         pthread_sigmask(SIG_BLOCK, &set, NULL);
             sigdelset(&set, SIG_IPI);
         sigfd = qemu_signalfd(&set);
         if (sigfd == -1) {
    -        fprintf(stderr, "failed to create signalfd\n");
    +        error_setg_errno(errp, errno, "failed to create signalfd");
             return -errno;
         }

Does this make sense?

Quoted 8 lines

Quoted 6 lines

Yes, it looks more concise if we only have this patch, but triggers one errno-set
issue after applying patch 7/7, which adds a Error **errp parameter for qemu_thread_create() to let its callers handle the error instead of exit() directly
to add the robustness.

I guess you're talking about the qemu_thread_create() in qemu_signalfd_compat(). Correct?

Yes.

Quoted 35 lines

Quoted 14 lines

For the patch series' current implementation, the  modified qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and set the
error reason into the passed errp, and did not set the errno. Actually another
similar errno-set issue has been talked in last patch. :) If we set the errno in future qemu_thread_create(), we need to distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to set errno.
But for Windows, I am not sure if we can use

Quoted 1 line

"errno = GetLastError()"

No, that won't work.

Quoted 4 lines

to set errno, as this seems a little weird. Do you have any idea about this?

BTW, if we have a decent errno-set way for Windows, I will adopt your above proposal for this patch.

According to MS docs, _beginthreadex() sets errno on failure:

If successful, each of these functions returns a handle to the newly created thread; however, if the newly created thread exits too quickly, _beginthread might not return a valid handle. (See the discussion in the Remarks section.) On an error, _beginthread returns -1L, and errno is set to EAGAIN if there are too many threads, to EINVAL if the argument is invalid or the stack size is incorrect, or to EACCES if there are insufficient resources (such as memory). On an error, _beginthreadex returns 0, and errno and _doserrno are set.

https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex

Quoted 7 lines

Looks like the current code's use of GetLastError() after _beginthreadex() failure is wrong.

Fix that, and both qemu_thread_create() implementations set errno on failure, which in turn lets you make qemu_signalfd_compat() and thus qemu_signalfd() behave sanely regardless of which qemu_thread_create() implementation they use below the hood.

Thanks for the detail explanation. :)
To fix that, how about replacing the "GetLastError()" with the returned value "hThread" (actually returns 0)? I mean
   ...
    hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) {
        if (data->mode != QEMU_THREAD_DETACHED) { DeleteCriticalSection(&data->cs);
        }
        error_setg_win32(errp, hThread,
                         "failed to create win32_start_routine"); g_free(data);
        return false;
    }

Quoted 18 lines · Fei Li

Quoted 17 lines · Markus Armbruster

Quoted 15 lines · Fei Li

Quoted 14 lines · Markus Armbruster

Quoted 3 lines · Fei Li

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.

The bug is in qemu_init_main_loop():

       ret = qemu_signal_init();
       if (ret) {
           return ret;
       }

Fails without setting an error, unlike the other failures. Its callers crash then.

Quoted 7 lines

Quoted 2 lines

Quoted 1 line

Thanks!

Consider working that into your commit message.

Ok. I'd like to reword as follows:
Currently in qemu_init_main_loop(), qemu_signal_init() fails without setting an error
like the other failures. Its callers crash then when runs error_report_err(err).

Polishing the English a bit:

When qemu_signal_init() fails in qemu_init_main_loop(), we return without setting an error. Its callers crash then when they try to report the error with error_report_err().

Quoted 158 lines

Quoted 108 lines

Quoted 108 lines

Quoted 108 lines

Quoted 62 lines

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 <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
    include/qemu/osdep.h | 2 +-
    util/compatfd.c      | 9 ++++++---
    util/main-loop.c     | 9 ++++-----
    3 files changed, 11 insertions(+), 9 deletions(-)

include/qemu/osdep.h +1 -0

index 4f8559e550..f1f56763a0 100644
@@ -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);
    #endif

util/compatfd.c +1 -0

index 980bd33e52..d3ed890405 100644
@@ -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)

util/compatfd.c +4 -0

+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 allocate signalfd memory");
            errno = ENOMEM;
            return -1;
        }
          if (pipe(fds) == -1) {
+        error_setg(errp, "Failed to create signalfd pipe");
            free(info);
            return -1;
        }

@@ -94,7 +97,7 @@ 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;

util/compatfd.c int qemu_signalfd(const sigset_t *mask) · line 113

Quoted 5 lines

util/compatfd.c +1 -0

@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
        }
    #endif
    -    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
    }

I think this takes the Error conversion too far.

qemu_signalfd() is like the signalfd() system call, only portable, and setting FD_CLOEXEC. In particular, it reports failure just like a system call: it sets errno and returns -1. I'd prefer to keep it that way. Instead...

Quoted 13 lines

util/main-loop.c +1 -0

index affe0403c5..9671b6d226 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
        }
    }
    -static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
    {
        int sigfd;
        sigset_t set;

util/main-loop.c static int qemu_signal_init(void) · line 100

Quoted 9 lines

util/main-loop.c +1 -2

@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
        pthread_sigmask(SIG_BLOCK, &set, NULL);
          sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
        if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
            return -errno;
        }
 

... change this function so:

          pthread_sigmask(SIG_BLOCK, &set, NULL);
              sigdelset(&set, SIG_IPI);
          sigfd = qemu_signalfd(&set);
          if (sigfd == -1) {
     -        fprintf(stderr, "failed to create signalfd\n");
     +        error_setg_errno(errp, errno, "failed to create signalfd");
              return -errno;
          }

Does this make sense?

Quoted 8 lines

Quoted 6 lines

Yes, it looks more concise if we only have this patch, but triggers one errno-set
issue after applying patch 7/7, which adds a Error **errp parameter for qemu_thread_create() to let its callers handle the error instead of exit() directly
to add the robustness.

I guess you're talking about the qemu_thread_create() in qemu_signalfd_compat(). Correct?

Yes.

Quoted 34 lines

Quoted 13 lines

For the patch series' current implementation, the  modified qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and set the
error reason into the passed errp, and did not set the errno. Actually another
similar errno-set issue has been talked in last patch. :) If we set the errno in future qemu_thread_create(), we need to distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to set errno.
But for Windows, I am not sure if we can use

Quoted 1 line

"errno = GetLastError()"

No, that won't work.

Quoted 4 lines

to set errno, as this seems a little weird. Do you have any idea about this?

BTW, if we have a decent errno-set way for Windows, I will adopt your above proposal for this patch.

According to MS docs, _beginthreadex() sets errno on failure:

If successful, each of these functions returns a handle to the newly created thread; however, if the newly created thread exits too quickly, _beginthread might not return a valid handle. (See the discussion in the Remarks section.) On an error, _beginthread returns -1L, and errno is set to EAGAIN if there are too many threads, to EINVAL if the argument is invalid or the stack size is incorrect, or to EACCES if there are insufficient resources (such as memory). On an error, _beginthreadex returns 0, and errno and _doserrno are set.

https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex

Quoted 7 lines

Looks like the current code's use of GetLastError() after _beginthreadex() failure is wrong.

Fix that, and both qemu_thread_create() implementations set errno on failure, which in turn lets you make qemu_signalfd_compat() and thus qemu_signalfd() behave sanely regardless of which qemu_thread_create() implementation they use below the hood.

Quoted 15 lines

Thanks for the detail explanation. :)
To fix that, how about replacing the "GetLastError()" with the returned value "hThread" (actually returns 0)? I mean
   ...
    hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) {
        if (data->mode != QEMU_THREAD_DETACHED) { DeleteCriticalSection(&data->cs);
        }
        error_setg_win32(errp, hThread,
                         "failed to create win32_start_routine"); g_free(data);
        return false;
    }

No. On failure, _beginthreadex() returns *zero*, not an error code. It also sets errno. That's the error code you need to use:

     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                       data, 0, &thread->tid);
     if (!hThread) {
         error_setg_errno(errp, errno, "can't create thread");
     }

Except I really wouldn't convert qemu_thread_create() to Error! I'd make it return zero on success, a negative errno code on failure, and leave the Error business to its callers. Basically, replace error_exit(err, ...) by return err.

The caller qemu_signalfd_compat() can then do

     ret = qemu_thread_create(&thread, "signalfd_compat",
                              sigwait_compat, info, QEMU_THREAD_DETACHED);
     if (ret < 0) {
         errno = ret;
         return -1;
     }

A caller that already has an Error **errp parameter could do

     ret = qemu_thread_create(...);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "<error message goes here>");
     }

Callers that want to continue aborting on failure simply do

     ret = qemu_thread_create(...);
     assert(ret >= 0);

If that turns out to be too much of a bother, you could create a convenience wrapper for it:

     void qemu_thread_create_nofail(QemuThread *thread, const char *name,
                                    void *(*start_routine)(void*),
                                    void *arg, int mode)
     {
         int err = qemu_thread_create(thread, name, start_routine, arg, mode);
         if (err) {
             error_exit(err, __func__);
         }
     }

Sorry for the late reply! Omitted this one..

Quoted 8 lines · Markus Armbruster

Quoted 6 lines · Fei Li

Quoted 5 lines · Markus Armbruster

Quoted 3 lines · Fei Li

Quoted 2 lines · Markus Armbruster

Fei Li <[email protected]> writes:

Quoted 60 lines

Quoted 42 lines

Quoted 35 lines

Quoted 13 lines

For the patch series' current implementation, the  modified qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and set the
error reason into the passed errp, and did not set the errno. Actually another
similar errno-set issue has been talked in last patch. :) If we set the errno in future qemu_thread_create(), we need to distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to set errno.
But for Windows, I am not sure if we can use

Quoted 1 line

"errno = GetLastError()"

No, that won't work.

Quoted 4 lines

to set errno, as this seems a little weird. Do you have any idea about this?

BTW, if we have a decent errno-set way for Windows, I will adopt your above proposal for this patch.

According to MS docs, _beginthreadex() sets errno on failure:

      If successful, each of these functions returns a handle to the newly created thread; however, if the newly created thread exits too quickly, _beginthread might not return a valid handle. (See the discussion in the Remarks section.) On an error, _beginthread returns -1L, and errno is set to EAGAIN if there are too many threads, to EINVAL if the argument is invalid or the stack size is incorrect, or to EACCES if there are insufficient resources (such as memory). On an error, _beginthreadex returns 0, and errno and _doserrno are set.

https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex

Quoted 7 lines

Looks like the current code's use of GetLastError() after _beginthreadex() failure is wrong.

Fix that, and both qemu_thread_create() implementations set errno on failure, which in turn lets you make qemu_signalfd_compat() and thus qemu_signalfd() behave sanely regardless of which qemu_thread_create() implementation they use below the hood.

Quoted 15 lines

Thanks for the detail explanation. :)
To fix that, how about replacing the "GetLastError()" with the returned value "hThread" (actually returns 0)? I mean
    ...
     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) {
         if (data->mode != QEMU_THREAD_DETACHED) { DeleteCriticalSection(&data->cs);
         }
         error_setg_win32(errp, hThread,
                          "failed to create win32_start_routine"); g_free(data);
         return false;
     }

No.  On failure, _beginthreadex() returns *zero*, not an error code.  It also sets errno.  That's the error code you need to use:

Quoted 5 lines

     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) {
         error_setg_errno(errp, errno, "can't create thread"); }

Ok, clearer, we also want the errno message to be printed, along with _beginthreadex() sets the errno's value by default. Thanks.

Quoted 4 lines

Except I really wouldn't convert qemu_thread_create() to Error!  I'd make it return zero on success, a negative errno code on failure, and leave the Error business to its callers.  Basically, replace error_exit(err, ...) by return err.

Emm, I am afraid not converting to Error means it is a little bit trickier to handle for the callers. Especially for migration callers [see patch 7/7], they have no initial errp passed, thus error_setg_xx() seems less useful. Instead in the caller I choose the error_reportf_err(local_err, ...) to only print the detail error message and return that caller's original failing tag. (But for those callers who already have the errp passed, both "return ret" and "convert qemu_thread_create() to Error" are fine to me.)

Besides, there is only one caller needs the errno value, that is qemu_signal_init():
"return -errno". Other callers do not use errno to indicate if it succeeds.

Thus the current patches choose pass Error to hold the detail error message and return a bool to indicate if the function succeeds.

Would you like to share your reason for not converting this function to Error? [1#begin]

Quoted 8 lines

The caller qemu_signalfd_compat() can then do

     ret = qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info, QEMU_THREAD_DETACHED); if (ret < 0) {
         errno = ret;
         return -1;
     }

[1#end]
[2#begin]

Quoted 6 lines

A caller that already has an Error **errp parameter could do

     ret = qemu_thread_create(...);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "<error message goes here>"); }

[2#end]
[3#begin]

Quoted 4 lines

Callers that want to continue aborting on failure simply do

     ret = qemu_thread_create(...);
     assert(ret >= 0);

[3#end]

Quoted 12 lines

If that turns out to be too much of a bother, you could create a convenience wrapper for it:

     void qemu_thread_create_nofail(QemuThread *thread, const char *name, void *(*start_routine)(void*), void *arg, int mode)
     {
         int err = qemu_thread_create(thread, name, start_routine, arg, mode); if (err) {
             error_exit(err, __func__);
         }
     }

I am wondering the above qemu_thread_create_nofail is a convenience for "[3#begin] => [3#end]"  or  "[1#begin] => [3#end]"..
If for "[3#begin] => [3#end]", I'd like to use the xxx_nofail wrapper as the error message is more detailed.
If for "[1#begin] => [3#end]", I'd like to explain more for this patch series: we want our qemu code to be more robust by not making qemu exit(-1) after qemu_thread_create() fails and let the callers handle this. E.g. for hmp/qmp callers, make qemu abort() seems too violent if they fails.

Have a nice day
Fei

Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error.

Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 include/ui/console.h |  2 +-
 ui/vnc-jobs.c        |  9 ++++++---
 ui/vnc-jobs.h        |  2 +-
 ui/vnc.c             | 12 +++++++++---
 4 files changed, 17 insertions(+), 8 deletions(-)

include/ui/console.h +1 -1

index fb969caf70..c17803c530 100644
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
 void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
 
 /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
 void vnc_display_open(const char *id, Error **errp);
 void vnc_display_add_client(const char *id, int csock, bool skipauth);
 int vnc_display_password(const char *id, const char *password);

ui/vnc-jobs.c +2 -3

index 929391f85d..8807d7217c 100644
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
     return queue; /* Check global queue */
 }
 
-void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
 {
     VncJobQueue *q;
 
-    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
 void vnc_jobs_join(VncState *vs);
 
 void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
 
 /* Locks */
 static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +0 -0

index cf221c83cc..f3806e76db 100644

ui/vnc.c +3 -1

@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
     vd->connections_limit = 32;
 
     qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
 
     vd->dcl.ops = &dcl_ops;
     register_displaychangelistener(&vd->dcl);

@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
     char *id = (char *)qemu_opts_id(opts);
 

Quoted 48 lines · Fei Li

Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error.

Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  include/ui/console.h |  2 +-
  ui/vnc-jobs.c        |  9 ++++++---
  ui/vnc-jobs.h        |  2 +-
  ui/vnc.c             | 12 +++++++++---
  4 files changed, 17 insertions(+), 8 deletions(-)

include/ui/console.h +1 -1

index fb969caf70..c17803c530 100644
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
  void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
 
  /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
  void vnc_display_open(const char *id, Error **errp);
  void vnc_display_add_client(const char *id, int csock, bool skipauth);
  int vnc_display_password(const char *id, const char *password);

ui/vnc-jobs.c +4 -3

index 929391f85d..8807d7217c 100644
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
      return queue; /* Check global queue */
  }
 
-void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
  {
      VncJobQueue *q;
 
-    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {
+        goto out;
+    }
 

ui/vnc-jobs.c static bool vnc_worker_thread_running(void) · line 348

Quoted 7 lines

ui/vnc-jobs.c +2 -0

      q = vnc_queue_init();
      qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
                         QEMU_THREAD_DETACHED);
      queue = q; /* Set global queue */
+out:
+    return true;
  }

This function cannot fail. Why convert it to Error, and complicate its caller?

Quoted 18 lines

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
  void vnc_jobs_join(VncState *vs);
 
  void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
 
  /* Locks */
  static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +0 -0

index cf221c83cc..f3806e76db 100644

ui/vnc.c static const DisplayChangeListenerOps dcl_ops = { · line 3211

Quoted 8 lines

ui/vnc.c +1 -1

@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
      .dpy_cursor_define    = vnc_dpy_cursor_define,
  };
 
-void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
  {
      VncDisplay *vd;
 

        if (vnc_display_find(id) != NULL) {
            return;
        }
        vd = g_malloc0(sizeof(*vd));

        vd->id = strdup(id);
        QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);

        QTAILQ_INIT(&vd->clients);
        vd->expires = TIME_MAX;

        if (keyboard_layout) {
            trace_vnc_key_map_init(keyboard_layout);
            vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
        } else {
            vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
        }

        if (!vd->kbd_layout) {
            exit(1);

Uh, init_keyboard_layout() reports errors to stderr, and the exit(1) here makes them fatal. Okay before this patch, but inappropriate afterwards. I'm afraid you have to convert init_keyboard_layout() to Error first.

        }

        vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;

Quoted 14 lines

ui/vnc.c +3 -1

@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
      vd->connections_limit = 32;
 
      qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
 
      vd->dcl.ops = &dcl_ops;
      register_displaychangelistener(&vd->dcl);

@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
      char *id = (char *)qemu_opts_id(opts);
 

ui/vnc.c int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) · line 4091

Quoted 10 lines

ui/vnc.c +5 -1

      assert(id);
-    vnc_display_init(id);
+    vnc_display_init(id, &local_err);
+    if (local_err) {
+        error_reportf_err(local_err, "Failed to init VNC server: ");
+        exit(1);
+    }
      vnc_display_open(id, &local_err);
      if (local_err != NULL) {
          error_reportf_err(local_err, "Failed to start VNC server: ");

Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in vnc_init_func()". Your patch shows that mine is incomplete.

Without my patch, there's no real reason for yours, however. The two should be squashed together.

Quoted 47 lines · Markus Armbruster

Quoted 45 lines · Fei Li

Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error.

Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  include/ui/console.h |  2 +-
  ui/vnc-jobs.c        |  9 ++++++---
  ui/vnc-jobs.h        |  2 +-
  ui/vnc.c             | 12 +++++++++---
  4 files changed, 17 insertions(+), 8 deletions(-)

include/ui/console.h +1 -1

index fb969caf70..c17803c530 100644
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
  void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
    /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
  void vnc_display_open(const char *id, Error **errp);
  void vnc_display_add_client(const char *id, int csock, bool skipauth);
  int vnc_display_password(const char *id, const char *password);

ui/vnc-jobs.c +4 -1

index 929391f85d..8807d7217c 100644
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
      return queue; /* Check global queue */
  }
  -void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
  {
      VncJobQueue *q;
  -    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {
+        goto out;
+    }
        q = vnc_queue_init();

Quoted 8 lines

ui/vnc-jobs.c static bool vnc_worker_thread_running(void) · line 348

Quoted 6 lines

ui/vnc-jobs.c +2 -0

      qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
                         QEMU_THREAD_DETACHED);
      queue = q; /* Set global queue */
+out:
+    return true;
  }

This function cannot fail. Why convert it to Error, and complicate its caller?

[Issue1]
Actually, this is to pave the way for patch 7/7, in case qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly connects the broken errp for callers who initially have the errp.

[I am back... If the other codes in this patches be squashed, maybe merge [Issue1]
into patch 7/7 looks more intuitive.]

Quoted 44 lines

Quoted 16 lines

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
  void vnc_jobs_join(VncState *vs);
    void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
    /* Locks */
  static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +0 -0

index cf221c83cc..f3806e76db 100644

ui/vnc.c static const DisplayChangeListenerOps dcl_ops = { · line 3211

Quoted 7 lines

ui/vnc.c +1 -0

@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
      .dpy_cursor_define    = vnc_dpy_cursor_define,
  };
  -void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
  {
      VncDisplay *vd;
 

        if (vnc_display_find(id) != NULL) {
            return;
        }
        vd = g_malloc0(sizeof(*vd));

        vd->id = strdup(id);
        QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);

        QTAILQ_INIT(&vd->clients);
        vd->expires = TIME_MAX;

        if (keyboard_layout) {
            trace_vnc_key_map_init(keyboard_layout);
            vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
        } else {
            vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
        }

        if (!vd->kbd_layout) {
            exit(1);

Quoted 4 lines

Uh, init_keyboard_layout() reports errors to stderr, and the exit(1) here makes them fatal. Okay before this patch, but inappropriate afterwards. I'm afraid you have to convert init_keyboard_layout() to Error first.

[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static value and also
will be used by others, how about passing the errp parameter to init_keyboard_layout()
as follows? And for its other callers, just pass NULL.

(quoted diff) +0 -1

@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
 
      if (keyboard_layout) {
          trace_vnc_key_map_init(keyboard_layout);
-        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);

Quoted 28 lines

        }

        vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;

Quoted 11 lines

ui/vnc.c +3 -1

@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
      vd->connections_limit = 32;
        qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
        vd->dcl.ops = &dcl_ops;
      register_displaychangelistener(&vd->dcl);

@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
      char *id = (char *)qemu_opts_id(opts);

ui/vnc.c int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) · line 4090

Quoted 10 lines

ui/vnc.c +5 -1

        assert(id);
-    vnc_display_init(id);
+    vnc_display_init(id, &local_err);
+    if (local_err) {
+        error_reportf_err(local_err, "Failed to init VNC server: ");
+        exit(1);
+    }
      vnc_display_open(id, &local_err);
      if (local_err != NULL) {
          error_reportf_err(local_err, "Failed to start VNC server: ");

Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in vnc_init_func()". Your patch shows that mine is incomplete.

Quoted 2 lines

Without my patch, there's no real reason for yours, however. The two should be squashed together.

Ah, I noticed your patch 24/31. OK, let's squash. Should I write a new patch by
- updating to error_propagate(...) if vnc_display_init() fails &&
- modifying [Issue2] ?
Or you do this in your original patch?

BTW, for your patch 24/31, the updated passed errp for vnc_init_func is &error_fatal,
then the system will exit(1) when running error_propagate(...) which calls error_handle_fatal(...). This means the following two lines will not be touched.
But if we want the following prepended error message, could we move it earlier than
the error_propagare? I mean:

     if (local_err != NULL) {
-        error_reportf_err(local_err, "Failed to start VNC server: "); -        exit(1);
+        error_prepend(&local_err, "Failed to start VNC server: "); +        error_propagate(errp, local_err);
+        return -1;
     }

Quoted 131 lines · Fei Li

Quoted 47 lines · Markus Armbruster

Quoted 45 lines · Fei Li

Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error.

Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
   include/ui/console.h |  2 +-
   ui/vnc-jobs.c        |  9 ++++++---
   ui/vnc-jobs.h        |  2 +-
   ui/vnc.c             | 12 +++++++++---
   4 files changed, 17 insertions(+), 8 deletions(-)

include/ui/console.h +1 -1

index fb969caf70..c17803c530 100644
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
   void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
     /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
   void vnc_display_open(const char *id, Error **errp);
   void vnc_display_add_client(const char *id, int csock, bool skipauth);
   int vnc_display_password(const char *id, const char *password);

ui/vnc-jobs.c +4 -1

index 929391f85d..8807d7217c 100644
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
       return queue; /* Check global queue */
   }
   -void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
   {
       VncJobQueue *q;
   -    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {
+        goto out;
+    }
         q = vnc_queue_init();

Quoted 8 lines

ui/vnc-jobs.c static bool vnc_worker_thread_running(void) · line 348

Quoted 6 lines

ui/vnc-jobs.c +2 -0

       qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
                          QEMU_THREAD_DETACHED);
       queue = q; /* Set global queue */
+out:
+    return true;
   }

This function cannot fail. Why convert it to Error, and complicate its caller?

[Issue1]
Actually, this is to pave the way for patch 7/7, in case qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly connects the broken errp for callers who initially have the errp.

[I am back... If the other codes in this patches be squashed, maybe merge [Issue1]
into patch 7/7 looks more intuitive.]

Quoted 44 lines

Quoted 16 lines

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
   void vnc_jobs_join(VncState *vs);
     void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
     /* Locks */
   static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +0 -0

index cf221c83cc..f3806e76db 100644

ui/vnc.c static const DisplayChangeListenerOps dcl_ops = { · line 3211

Quoted 7 lines

ui/vnc.c +1 -0

@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
       .dpy_cursor_define    = vnc_dpy_cursor_define,
   };
   -void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
   {
       VncDisplay *vd;
 

         if (vnc_display_find(id) != NULL) {
             return;
         }
         vd = g_malloc0(sizeof(*vd));

         vd->id = strdup(id);
         QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);

         QTAILQ_INIT(&vd->clients);
         vd->expires = TIME_MAX;

         if (keyboard_layout) {
             trace_vnc_key_map_init(keyboard_layout);
             vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
         } else {
             vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
         }

         if (!vd->kbd_layout) {
             exit(1);

Quoted 4 lines

Uh, init_keyboard_layout() reports errors to stderr, and the exit(1) here makes them fatal. Okay before this patch, but inappropriate afterwards. I'm afraid you have to convert init_keyboard_layout() to Error first.

[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static value and also
will be used by others, how about passing the errp parameter to init_keyboard_layout()
as follows? And for its other callers, just pass NULL.

(quoted diff) +2 -2

@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
 
      if (keyboard_layout) {
          trace_vnc_key_map_init(keyboard_layout);
-        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+        vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout, errp);
      } else {
-        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
      }
 

Review void vnc_display_init(const char *id, Error **errp) · line 3232

Quoted 4 lines

(quoted diff) +1 -1

      if (!vd->kbd_layout) {
-        exit(1);
+        return;
      }

Sounds good to me, except you should pass &error_fatal instead of NULL to preserve "report error and exit" semantics.

Quoted 30 lines

(quoted diff) +0 -0

 >          }

Quoted 28 lines

         vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;

Quoted 11 lines

ui/vnc.c +3 -1

@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
       vd->connections_limit = 32;
         qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
         vd->dcl.ops = &dcl_ops;
       register_displaychangelistener(&vd->dcl);

@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
       char *id = (char *)qemu_opts_id(opts);

ui/vnc.c int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) · line 4090

Quoted 10 lines

ui/vnc.c +5 -1

         assert(id);
-    vnc_display_init(id);
+    vnc_display_init(id, &local_err);
+    if (local_err) {
+        error_reportf_err(local_err, "Failed to init VNC server: ");
+        exit(1);
+    }
       vnc_display_open(id, &local_err);
       if (local_err != NULL) {
           error_reportf_err(local_err, "Failed to start VNC server: ");

Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in vnc_init_func()". Your patch shows that mine is incomplete.

Without my patch, there's no real reason for yours, however. The two should be squashed together.

Quoted 6 lines

Ah, I noticed your patch 24/31. OK, let's squash. Should I write a new patch by
- updating to error_propagate(...) if vnc_display_init() fails &&
- modifying [Issue2] ?
Or you do this in your original patch?

If you send a v2 along that lines, I'll probably pick your patch into my series. Should work even in case your series gets merged first.

Quoted 10 lines

BTW, for your patch 24/31, the updated passed errp for vnc_init_func is &error_fatal,
then the system will exit(1) when running error_propagate(...) which calls error_handle_fatal(...). This means the following two lines will not be touched.
But if we want the following prepended error message, could we move it earlier than
the error_propagare? I mean:

Quoted 7 lines

     if (local_err != NULL) {
-        error_reportf_err(local_err, "Failed to start VNC server: "); -        exit(1);
+        error_prepend(&local_err, "Failed to start VNC server: "); +        error_propagate(errp, local_err);
+        return -1;
     }

Both

          error_propagate(errp, local_err);
          error_prepend(errp, "Failed to start VNC server: ");

and

         error_prepend(&local_err, "Failed to start VNC server: ");
         error_propagate(errp, local_err);

work. The former is slightly more efficient when errp is null. But you're right, it fails to include the "Failed to start VNC server: " prefix with &error_fatal. Thus, the latter is preferrable.

Quoted 138 lines · Markus Armbruster

Quoted 136 lines · Fei Li

Quoted 48 lines · Markus Armbruster

Quoted 46 lines · Fei Li

Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error.

Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
   include/ui/console.h |  2 +-
   ui/vnc-jobs.c        |  9 ++++++---
   ui/vnc-jobs.h        |  2 +-
   ui/vnc.c             | 12 +++++++++---

   4 files changed, 17 insertions(+), 8 deletions(-)

include/ui/console.h +1 -1

index fb969caf70..c17803c530 100644
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions 
 *opts);
    void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
      /* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
    void vnc_display_open(const char *id, Error **errp);
    void vnc_display_add_client(const char *id, int csock, bool skipauth);
    int vnc_display_password(const char *id, const char *password);

ui/vnc-jobs.c +0 -0

index 929391f85d..8807d7217c 100644
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)

       return queue; /* Check global queue */
   }
   -void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
   {
       VncJobQueue *q;
   -    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {
+        goto out;
+    }
         q = vnc_queue_init();

Quoted 8 lines

Quoted 6 lines

       qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, QEMU_THREAD_DETACHED);
       queue = q; /* Set global queue */
+out:
+    return true;
   }

This function cannot fail.  Why convert it to Error, and complicate its caller?

[Issue1]
Actually, this is to pave the way for patch 7/7, in case qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly connects the broken errp for callers who initially have the errp.

[I am back... If the other codes in this patches be squashed, maybe merge [Issue1]
into patch 7/7 looks more intuitive.]

Quoted 41 lines

Quoted 16 lines

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
    void vnc_jobs_join(VncState *vs);
      void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
      /* Locks */
    static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +0 -0

index cf221c83cc..f3806e76db 100644

ui/vnc.c static const DisplayChangeListenerOps · line 3211

Quoted 7 lines

ui/vnc.c +1 -0

@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps 
 dcl_ops = {
        .dpy_cursor_define    = vnc_dpy_cursor_define,
    };
    -void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
    {
        VncDisplay *vd;

         if (vnc_display_find(id) != NULL) {
             return;
         }
         vd = g_malloc0(sizeof(*vd));

         vd->id = strdup(id); QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);

         QTAILQ_INIT(&vd->clients);
         vd->expires = TIME_MAX;

         if (keyboard_layout) { trace_vnc_key_map_init(keyboard_layout);
             vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout); } else {
             vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us"); }

Quoted 7 lines

         if (!vd->kbd_layout) {
             exit(1);

Uh, init_keyboard_layout() reports errors to stderr, and the exit(1) here makes them fatal.  Okay before this patch, but inappropriate afterwards.  I'm afraid you have to convert init_keyboard_layout() to Error first.

[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static value and also
will be used by others, how about passing the errp parameter to init_keyboard_layout()
as follows? And for its other callers, just pass NULL.

(quoted diff) +2 -2

@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error 
 **errp)
 
       if (keyboard_layout) {
           trace_vnc_key_map_init(keyboard_layout);
-        vd->kbd_layout = init_keyboard_layout(name2keysym, 
 keyboard_layout);
+        vd->kbd_layout = init_keyboard_layout(name2keysym, 
 keyboard_layout, errp);
       } else {
-        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+        vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", 
 errp);
       }
 

Quoted 6 lines

Quoted 4 lines

(quoted diff) +1 -1

       if (!vd->kbd_layout) {
-        exit(1);
+        return;

      }

Sounds good to me, except you should pass &error_fatal instead of NULL to preserve "report error and exit" semantics.

Yes, you are right. I should pass &error_fatal and let the following error_setg() handle this.

(quoted diff) +1 -1

@@ -94,7 +94,7 @@ static kbd_layout_t *parse_keyboard_layout(const 
 name2keysym_t *table,
      f = filename ? fopen(filename, "r") : NULL;
      g_free(filename);
      if (!f) {
-        fprintf(stderr, "Could not read keymap file: '%s'\n", language);
+        error_setg(errp, "could not read keymap file: '%s'\n", 
 language);
          return NULL;

     }

Quoted 37 lines

Quoted 37 lines

Quoted 29 lines

         }

         vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;

Quoted 12 lines

ui/vnc.c +3 -1

@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
        vd->connections_limit = 32;
          qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
          vd->dcl.ops = &dcl_ops;
        register_displaychangelistener(&vd->dcl);

@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts 
 *opts, Error **errp)
        char *id = (char *)qemu_opts_id(opts);

ui/vnc.c int vnc_init_func(void *opaque, QemuOpts · line 4091

Quoted 10 lines

ui/vnc.c +5 -1

          assert(id);
-    vnc_display_init(id);
+    vnc_display_init(id, &local_err);
+    if (local_err) {
+        error_reportf_err(local_err, "Failed to init VNC server: ");
+        exit(1);
+    }
        vnc_display_open(id, &local_err);
        if (local_err != NULL) {
            error_reportf_err(local_err, "Failed to start VNC server: ");

Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in vnc_init_func()".  Your patch shows that mine is incomplete.

Quoted 2 lines

Without my patch, there's no real reason for yours, however.  The two should be squashed together.

Ah, I noticed your patch 24/31. OK, let's squash. Should I write a new patch by
- updating to error_propagate(...) if vnc_display_init() fails &&
- modifying [Issue2] ?
Or you do this in your original patch?

Quoted 2 lines

If you send a v2 along that lines, I'll probably pick your patch into my series.  Should work even in case your series gets merged first.

Good idea. I will send a separate v2 patch which also include the above change mentioned in [Issue1].

Quoted 22 lines

Quoted 10 lines

BTW, for your patch 24/31, the updated passed errp for vnc_init_func is &error_fatal,
then the system will exit(1) when running error_propagate(...) which calls error_handle_fatal(...). This means the following two lines will not be touched.
But if we want the following prepended error message, could we move it earlier than
the error_propagare? I mean:

Quoted 7 lines

      if (local_err != NULL) {
-        error_reportf_err(local_err, "Failed to start VNC server: "); -        exit(1);
+        error_prepend(&local_err, "Failed to start VNC server: "); +        error_propagate(errp, local_err);
+        return -1;
      }

Both

          error_propagate(errp, local_err); error_prepend(errp, "Failed to start VNC server: ");

Quoted 10 lines

and

         error_prepend(&local_err, "Failed to start VNC server: "); error_propagate(errp, local_err);

work.  The former is slightly more efficient when errp is null. But you're right, it fails to include the "Failed to start VNC server: " prefix with &error_fatal.  Thus, the latter is preferrable.

Have a nice day, thanks
Fei

The caller of qemu_init_vcpu() already passed the **errp to handle

errors. In view of this, add a new Error parameter to the following call trace to propagate the error and let the further caller check it.

Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 accel/tcg/user-exec-stub.c      |  2 +-
 cpus.c                          | 34 +++++++++++++++++++++-------------
 include/qom/cpu.h               |  2 +-
 target/alpha/cpu.c              |  4 +++-
 target/arm/cpu.c                |  4 +++-
 target/cris/cpu.c               |  4 +++-
 target/hppa/cpu.c               |  4 +++-
 target/i386/cpu.c               |  4 +++-
 target/lm32/cpu.c               |  4 +++-
 target/m68k/cpu.c               |  4 +++-
 target/microblaze/cpu.c         |  4 +++-
 target/mips/cpu.c               |  4 +++-
 target/moxie/cpu.c              |  4 +++-
 target/nios2/cpu.c              |  4 +++-
 target/openrisc/cpu.c           |  4 +++-
 target/ppc/translate_init.inc.c |  4 +++-
 target/riscv/cpu.c              |  4 +++-
 target/s390x/cpu.c              |  4 +++-
 target/sh4/cpu.c                |  4 +++-
 target/sparc/cpu.c              |  4 +++-
 target/tilegx/cpu.c             |  4 +++-
 target/tricore/cpu.c            |  4 +++-
 target/unicore32/cpu.c          |  4 +++-
 target/xtensa/cpu.c             |  4 +++-
 24 files changed, 86 insertions(+), 36 deletions(-)

accel/tcg/user-exec-stub.c +0 -0

index a32b4496af..38f6b928d4 100644

accel/tcg/user-exec-stub.c +0 -0

 }
 

cpus.c +21 -13

index 361678e459..c4db70607e 100644
@@ -1918,7 +1918,7 @@ void cpu_remove_sync(CPUState *cpu)
 /* For temporary buffers for forming a name */
 #define VCPU_THREAD_NAME_SIZE 16
 
-static void qemu_tcg_init_vcpu(CPUState *cpu)
+static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
     static QemuCond *single_tcg_halt_cond;

@@ -1974,7 +1974,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
     }
 }
 
-static void qemu_hax_start_vcpu(CPUState *cpu)
+static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -1991,7 +1991,7 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
 #endif
 }
 
-static void qemu_kvm_start_vcpu(CPUState *cpu)
+static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2004,7 +2004,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-static void qemu_hvf_start_vcpu(CPUState *cpu)
+static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2022,7 +2022,7 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-static void qemu_whpx_start_vcpu(CPUState *cpu)
+static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2038,7 +2038,7 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
 #endif
 }
 
-static void qemu_dummy_start_vcpu(CPUState *cpu)
+static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2051,11 +2051,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
                        QEMU_THREAD_JOINABLE);
 }
 
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
 {
     cpu->nr_cores = smp_cores;
     cpu->nr_threads = smp_threads;
     cpu->stopped = true;
+    Error *local_err = NULL;
 
     if (!cpu->as) {
         /* If the target cpu hasn't set up any address spaces itself,

@@ -2066,22 +2067,29 @@ void qemu_init_vcpu(CPUState *cpu)
     }
 
     if (kvm_enabled()) {
-        qemu_kvm_start_vcpu(cpu);
+        qemu_kvm_start_vcpu(cpu, &local_err);
     } else if (hax_enabled()) {
-        qemu_hax_start_vcpu(cpu);
+        qemu_hax_start_vcpu(cpu, &local_err);
     } else if (hvf_enabled()) {
-        qemu_hvf_start_vcpu(cpu);
+        qemu_hvf_start_vcpu(cpu, &local_err);
     } else if (tcg_enabled()) {
-        qemu_tcg_init_vcpu(cpu);
+        qemu_tcg_init_vcpu(cpu, &local_err);
     } else if (whpx_enabled()) {
-        qemu_whpx_start_vcpu(cpu);
+        qemu_whpx_start_vcpu(cpu, &local_err);
     } else {
-        qemu_dummy_start_vcpu(cpu);
+        qemu_dummy_start_vcpu(cpu, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return false;
     }
 
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
     }
+
+    return true;
 }
 
 void cpu_stop_current(void)

include/qom/cpu.h +1 -1

index dc130cd307..4d85dda175 100644
@@ -1012,7 +1012,7 @@ void end_exclusive(void);
  *
  * Initializes a vCPU.
  */
-void qemu_init_vcpu(CPUState *cpu);
+bool qemu_init_vcpu(CPUState *cpu, Error **errp);
 
 #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
 #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */

target/alpha/cpu.c +3 -1

index b08078e7fc..d531bd4f7e 100644
@@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     acc->parent_realize(dev, errp);
 }

target/arm/cpu.c +3 -1

index b5e61cc177..40f300174d 100644
@@ -1038,7 +1038,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     acc->parent_realize(dev, errp);

target/cris/cpu.c +3 -1

index a23aba2688..ec92d69781 100644
@@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     ccc->parent_realize(dev, errp);
 }

target/hppa/cpu.c +3 -1

index 00bf444620..08f600ced9 100644
@@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     acc->parent_realize(dev, errp);
 
 #ifndef CONFIG_USER_ONLY

target/i386/cpu.c +3 -1

index c88876dfe3..d199f91258 100644
@@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     /*
      * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU

target/lm32/cpu.c +3 -1

index b7499cb627..d50b1e4a43 100644
@@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cpu_reset(cs);
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     lcc->parent_realize(dev, errp);
 }

target/m68k/cpu.c +3 -1

index 582e3a73b3..4ab53f2d58 100644
@@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
     m68k_cpu_init_gdb(cpu);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     mcc->parent_realize(dev, errp);
 }

target/microblaze/cpu.c +3 -1

index 9b546a2c18..3906c864a3 100644
@@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     env->pvr.regs[0] = PVR0_USE_EXC_MASK \
                        | PVR0_USE_ICACHE_MASK \

target/mips/cpu.c +3 -1

index 497706b669..62be84af76 100644
@@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_mips_realize_env(&cpu->env);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     mcc->parent_realize(dev, errp);
 }

target/moxie/cpu.c +3 -1

index 8d67eb6727..8581a6d922 100644
@@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);

target/nios2/cpu.c +3 -1

index fbfaa2ce26..5c7b4b486e 100644
@@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     ncc->parent_realize(dev, errp);

target/openrisc/cpu.c +3 -1

index fb7cb5c507..a6dcdb9df9 100644
@@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     occ->parent_realize(dev, errp);

target/ppc/translate_init.inc.c +3 -1

index 263e63cb03..a6dd2318a6 100644
@@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
                                  32, "power-vsx.xml", 0);
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        goto unrealize;
+    }
 
     pcc->parent_realize(dev, errp);
 

target/riscv/cpu.c +3 -1

index d630e8fd6c..ef56215e92 100644
@@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);

target/s390x/cpu.c +3 -1

index 18ba7f85a5..2a3eac9761 100644
@@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
     s390_cpu_gdb_init(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     /*
      * KVM requires the initial CPU reset ioctl to be executed on the target

target/sh4/cpu.c +3 -1

index b9f393b7c7..d32ef2e1cb 100644
@@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     scc->parent_realize(dev, errp);
 }

target/sparc/cpu.c +3 -1

index 0f090ece54..9c22f6a7df 100644
@@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     scc->parent_realize(dev, errp);
 }

target/tilegx/cpu.c +3 -1

index bfe9be59b5..234148fabd 100644
@@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     tcc->parent_realize(dev, errp);
 }

target/tricore/cpu.c +3 -1

index 2edaef1aef..5482d6ea3f 100644
@@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
         set_feature(env, TRICORE_FEATURE_13);
     }
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     tcc->parent_realize(dev, errp);
 }

target/unicore32/cpu.c +3 -1

index 68f978d80b..1f6a33b6f3 100644
@@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     ucc->parent_realize(dev, errp);
 }

target/xtensa/cpu.c +0 -0

index a54dbe4260..d2351c9b20 100644

Quoted 1 line · Fei Li

The caller of qemu_init_vcpu() already passed the **errp to handle

Which caller? There are many. Or do you mean "The callers"?

Quoted 3 lines

errors. In view of this, add a new Error parameter to the following call trace to propagate the error and let the further caller check it.

Which "call trace"?

Quoted 41 lines

Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  accel/tcg/user-exec-stub.c      |  2 +-
  cpus.c                          | 34 +++++++++++++++++++++-------------
  include/qom/cpu.h               |  2 +-
  target/alpha/cpu.c              |  4 +++-
  target/arm/cpu.c                |  4 +++-
  target/cris/cpu.c               |  4 +++-
  target/hppa/cpu.c               |  4 +++-
  target/i386/cpu.c               |  4 +++-
  target/lm32/cpu.c               |  4 +++-
  target/m68k/cpu.c               |  4 +++-
  target/microblaze/cpu.c         |  4 +++-
  target/mips/cpu.c               |  4 +++-
  target/moxie/cpu.c              |  4 +++-
  target/nios2/cpu.c              |  4 +++-
  target/openrisc/cpu.c           |  4 +++-
  target/ppc/translate_init.inc.c |  4 +++-
  target/riscv/cpu.c              |  4 +++-
  target/s390x/cpu.c              |  4 +++-
  target/sh4/cpu.c                |  4 +++-
  target/sparc/cpu.c              |  4 +++-
  target/tilegx/cpu.c             |  4 +++-
  target/tricore/cpu.c            |  4 +++-
  target/unicore32/cpu.c          |  4 +++-
  target/xtensa/cpu.c             |  4 +++-
  24 files changed, 86 insertions(+), 36 deletions(-)

accel/tcg/user-exec-stub.c +0 -0

index a32b4496af..38f6b928d4 100644
@@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
  {
  }
 

accel/tcg/user-exec-stub.c void cpu_resume(CPUState *cpu) · line 14

Quoted 3 lines

accel/tcg/user-exec-stub.c +1 -1

-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
  {

You need to return a value here. Sure you compile-tested this?

Quoted 437 lines

accel/tcg/user-exec-stub.c +0 -0

  }
 

cpus.c +21 -13

index 361678e459..c4db70607e 100644
@@ -1918,7 +1918,7 @@ void cpu_remove_sync(CPUState *cpu)
  /* For temporary buffers for forming a name */
  #define VCPU_THREAD_NAME_SIZE 16
 
-static void qemu_tcg_init_vcpu(CPUState *cpu)
+static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
      static QemuCond *single_tcg_halt_cond;

@@ -1974,7 +1974,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
      }
  }
 
-static void qemu_hax_start_vcpu(CPUState *cpu)
+static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -1991,7 +1991,7 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
  #endif
  }
 
-static void qemu_kvm_start_vcpu(CPUState *cpu)
+static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2004,7 +2004,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
                         cpu, QEMU_THREAD_JOINABLE);
  }
 
-static void qemu_hvf_start_vcpu(CPUState *cpu)
+static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2022,7 +2022,7 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
                         cpu, QEMU_THREAD_JOINABLE);
  }
 
-static void qemu_whpx_start_vcpu(CPUState *cpu)
+static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2038,7 +2038,7 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
  #endif
  }
 
-static void qemu_dummy_start_vcpu(CPUState *cpu)
+static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
  {
      char thread_name[VCPU_THREAD_NAME_SIZE];
 

@@ -2051,11 +2051,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
                         QEMU_THREAD_JOINABLE);
  }
 
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
  {
      cpu->nr_cores = smp_cores;
      cpu->nr_threads = smp_threads;
      cpu->stopped = true;
+    Error *local_err = NULL;
 
      if (!cpu->as) {
          /* If the target cpu hasn't set up any address spaces itself,

@@ -2066,22 +2067,29 @@ void qemu_init_vcpu(CPUState *cpu)
      }
 
      if (kvm_enabled()) {
-        qemu_kvm_start_vcpu(cpu);
+        qemu_kvm_start_vcpu(cpu, &local_err);
      } else if (hax_enabled()) {
-        qemu_hax_start_vcpu(cpu);
+        qemu_hax_start_vcpu(cpu, &local_err);
      } else if (hvf_enabled()) {
-        qemu_hvf_start_vcpu(cpu);
+        qemu_hvf_start_vcpu(cpu, &local_err);
      } else if (tcg_enabled()) {
-        qemu_tcg_init_vcpu(cpu);
+        qemu_tcg_init_vcpu(cpu, &local_err);
      } else if (whpx_enabled()) {
-        qemu_whpx_start_vcpu(cpu);
+        qemu_whpx_start_vcpu(cpu, &local_err);
      } else {
-        qemu_dummy_start_vcpu(cpu);
+        qemu_dummy_start_vcpu(cpu, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return false;
      }
 
      while (!cpu->created) {
          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
      }
+
+    return true;
  }
 
  void cpu_stop_current(void)

include/qom/cpu.h +1 -1

index dc130cd307..4d85dda175 100644
@@ -1012,7 +1012,7 @@ void end_exclusive(void);
   *
   * Initializes a vCPU.
   */
-void qemu_init_vcpu(CPUState *cpu);
+bool qemu_init_vcpu(CPUState *cpu, Error **errp);
 
  #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
  #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */

target/alpha/cpu.c +3 -1

index b08078e7fc..d531bd4f7e 100644
@@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      acc->parent_realize(dev, errp);
  }

target/arm/cpu.c +3 -1

index b5e61cc177..40f300174d 100644
@@ -1038,7 +1038,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
      }
  #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      cpu_reset(cs);
 
      acc->parent_realize(dev, errp);

target/cris/cpu.c +3 -1

index a23aba2688..ec92d69781 100644
@@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
      }
 
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      ccc->parent_realize(dev, errp);
  }

target/hppa/cpu.c +3 -1

index 00bf444620..08f600ced9 100644
@@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      acc->parent_realize(dev, errp);
 
  #ifndef CONFIG_USER_ONLY

target/i386/cpu.c +3 -1

index c88876dfe3..d199f91258 100644
@@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
      }
  #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      /*
       * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU

target/lm32/cpu.c +3 -1

index b7499cb627..d50b1e4a43 100644
@@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
 
      cpu_reset(cs);
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      lcc->parent_realize(dev, errp);
  }

target/m68k/cpu.c +3 -1

index 582e3a73b3..4ab53f2d58 100644
@@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
      m68k_cpu_init_gdb(cpu);
 
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      mcc->parent_realize(dev, errp);
  }

target/microblaze/cpu.c +3 -1

index 9b546a2c18..3906c864a3 100644
@@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      env->pvr.regs[0] = PVR0_USE_EXC_MASK \
                         | PVR0_USE_ICACHE_MASK \

target/mips/cpu.c +3 -1

index 497706b669..62be84af76 100644
@@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
      cpu_mips_realize_env(&cpu->env);
 
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      mcc->parent_realize(dev, errp);
  }

target/moxie/cpu.c +3 -1

index 8d67eb6727..8581a6d922 100644
@@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      cpu_reset(cs);
 
      mcc->parent_realize(dev, errp);

target/nios2/cpu.c +3 -1

index fbfaa2ce26..5c7b4b486e 100644
@@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      cpu_reset(cs);
 
      ncc->parent_realize(dev, errp);

target/openrisc/cpu.c +3 -1

index fb7cb5c507..a6dcdb9df9 100644
@@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      cpu_reset(cs);
 
      occ->parent_realize(dev, errp);

target/ppc/translate_init.inc.c +3 -1

index 263e63cb03..a6dd2318a6 100644
@@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
                                   32, "power-vsx.xml", 0);
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        goto unrealize;
+    }
 
      pcc->parent_realize(dev, errp);
 

target/riscv/cpu.c +3 -1

index d630e8fd6c..ef56215e92 100644
@@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
      cpu_reset(cs);
 
      mcc->parent_realize(dev, errp);

target/s390x/cpu.c +3 -1

index 18ba7f85a5..2a3eac9761 100644
@@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
      qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
  #endif
      s390_cpu_gdb_init(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      /*
       * KVM requires the initial CPU reset ioctl to be executed on the target

target/sh4/cpu.c +3 -1

index b9f393b7c7..d32ef2e1cb 100644
@@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
      }
 
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      scc->parent_realize(dev, errp);
  }

target/sparc/cpu.c +3 -1

index 0f090ece54..9c22f6a7df 100644
@@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      scc->parent_realize(dev, errp);
  }

target/tilegx/cpu.c +3 -1

index bfe9be59b5..234148fabd 100644
@@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
      }
 
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      tcc->parent_realize(dev, errp);
  }

target/tricore/cpu.c +3 -1

index 2edaef1aef..5482d6ea3f 100644
@@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
          set_feature(env, TRICORE_FEATURE_13);
      }
      cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      tcc->parent_realize(dev, errp);
  }

target/unicore32/cpu.c +3 -1

index 68f978d80b..1f6a33b6f3 100644
@@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
          return;
      }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
      ucc->parent_realize(dev, errp);
  }

target/xtensa/cpu.c +3 -1

index a54dbe4260..d2351c9b20 100644
@@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 
      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 

target/xtensa/cpu.c static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp) · line 139

Quoted 2 lines

target/xtensa/cpu.c +0 -0

      xcc->parent_realize(dev, errp);
  }

I see how you changed the code to pass an Error from the qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't see how such errors can be created. Without a way to create any, the patch is pointless. What am I missing?

Quoted 4 lines · Markus Armbruster

Quoted 1 line · Fei Li

The caller of qemu_init_vcpu() already passed the **errp to handle

Which caller? There are many. Or do you mean "The callers"?

Oh, sorry, I mean "The callers" :)

Quoted 4 lines

Quoted 3 lines

errors. In view of this, add a new Error parameter to the following call trace to propagate the error and let the further caller check it.

Which "call trace"?

Actually, I want to say all functions called by qemu_init_vcpu()..

Quoted 33 lines

Quoted 33 lines

Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
  accel/tcg/user-exec-stub.c      |  2 +-
  cpus.c                          | 34 +++++++++++++++++++++-------------
  include/qom/cpu.h               |  2 +-
  target/alpha/cpu.c              |  4 +++-
  target/arm/cpu.c                |  4 +++-
  target/cris/cpu.c               |  4 +++-
  target/hppa/cpu.c               |  4 +++-
  target/i386/cpu.c               |  4 +++-
  target/lm32/cpu.c               |  4 +++-
  target/m68k/cpu.c               |  4 +++-
  target/microblaze/cpu.c         |  4 +++-
  target/mips/cpu.c               |  4 +++-
  target/moxie/cpu.c              |  4 +++-
  target/nios2/cpu.c              |  4 +++-
  target/openrisc/cpu.c           |  4 +++-
  target/ppc/translate_init.inc.c |  4 +++-
  target/riscv/cpu.c              |  4 +++-
  target/s390x/cpu.c              |  4 +++-
  target/sh4/cpu.c                |  4 +++-
  target/sparc/cpu.c              |  4 +++-
  target/tilegx/cpu.c             |  4 +++-
  target/tricore/cpu.c            |  4 +++-
  target/unicore32/cpu.c          |  4 +++-
  target/xtensa/cpu.c             |  4 +++-
  24 files changed, 86 insertions(+), 36 deletions(-)

Quoted 11 lines

accel/tcg/user-exec-stub.c void cpu_resume(CPUState *cpu) · line 14

Quoted 10 lines

accel/tcg/user-exec-stub.c +1 -0

index a32b4496af..38f6b928d4 100644
@@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
  {
  }
  -void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
  {

You need to return a value here. Sure you compile-tested this?

Oops! I forget the TCG case.. The return true should be added. Thanks for pointing this out!

Quoted 2 lines

Quoted 2 lines

accel/tcg/user-exec-stub.c +0 -0

  }
  diff --git a/cpus.c b/cpus.c

Quoted 4 lines

I see how you changed the code to pass an Error from the qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't see how such errors can be created. Without a way to create any, the patch is pointless. What am I missing?

This patch is also the pre-patch for the updated qemu_thread_create() in patch 7/7
just as I explained in patch 2/7 [Issue1].

Quoted 53 lines · Fei Li

Quoted 4 lines · Markus Armbruster

Quoted 1 line · Fei Li

The caller of qemu_init_vcpu() already passed the **errp to handle

Which caller? There are many. Or do you mean "The callers"?

Oh, sorry, I mean "The callers" :)

Quoted 3 lines

Quoted 2 lines

errors. In view of this, add a new Error parameter to the following call trace to propagate the error and let the further caller check it.

Which "call trace"?

Actually, I want to say all functions called by qemu_init_vcpu()..

Quoted 32 lines

Quoted 32 lines

Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
   accel/tcg/user-exec-stub.c      |  2 +-
   cpus.c                          | 34 +++++++++++++++++++++-------------
   include/qom/cpu.h               |  2 +-
   target/alpha/cpu.c              |  4 +++-
   target/arm/cpu.c                |  4 +++-
   target/cris/cpu.c               |  4 +++-
   target/hppa/cpu.c               |  4 +++-
   target/i386/cpu.c               |  4 +++-
   target/lm32/cpu.c               |  4 +++-
   target/m68k/cpu.c               |  4 +++-
   target/microblaze/cpu.c         |  4 +++-
   target/mips/cpu.c               |  4 +++-
   target/moxie/cpu.c              |  4 +++-
   target/nios2/cpu.c              |  4 +++-
   target/openrisc/cpu.c           |  4 +++-
   target/ppc/translate_init.inc.c |  4 +++-
   target/riscv/cpu.c              |  4 +++-
   target/s390x/cpu.c              |  4 +++-
   target/sh4/cpu.c                |  4 +++-
   target/sparc/cpu.c              |  4 +++-
   target/tilegx/cpu.c             |  4 +++-
   target/tricore/cpu.c            |  4 +++-
   target/unicore32/cpu.c          |  4 +++-
   target/xtensa/cpu.c             |  4 +++-
   24 files changed, 86 insertions(+), 36 deletions(-)

Quoted 11 lines

accel/tcg/user-exec-stub.c void cpu_resume(CPUState *cpu) · line 14

Quoted 10 lines

accel/tcg/user-exec-stub.c +1 -0

index a32b4496af..38f6b928d4 100644
@@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
   {
   }
   -void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
   {

You need to return a value here. Sure you compile-tested this?

Quoted 2 lines

Oops! I forget the TCG case.. The return true should be added. Thanks for pointing this out!

You're welcome.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

accel/tcg/user-exec-stub.c +0 -0

   }
   diff --git a/cpus.c b/cpus.c

[...]

Quoted 4 lines

Quoted 4 lines

I see how you changed the code to pass an Error from the qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't see how such errors can be created. Without a way to create any, the patch is pointless. What am I missing?