qemu_thread_create: propagate the error to callers to handle

Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it. And make the function be: void multifd_save_cleanup(void).

Cc Dr. David Alan Gilbert <[email protected]>

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

Reviewed-by Juan Quintela <[email protected]>

Quoted 8 lines · Fei Li

Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it. And make the function be: void multifd_save_cleanup(void).

Cc Dr. David Alan Gilbert <[email protected]>

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

Reviewed-by Juan Quintela <[email protected]>

The commit message is confusing. Suggest:

migration: multifd_save_cleanup() can't fail, simplify

multifd_save_cleanup() takes an Error ** argument and returns an error code even though it can't actually fail. Its callers dutifully check for failure. Remove the useless argument and return value, and simplify the callers.

I think multifd_load_cleanup() has exactly the same issue. Should we clean it up, too? Juan, what do you think?

In the current code, if process_incoming_migration_co() fails we do the same error handing: set the error state, close the source file, do the cleanup for multifd, and then exit(EXIT_FAILURE). To make the code clearer, add a "goto fail" to unify the error handling.

Cc Markus Armbruster <[email protected]>

Cc Dr. David Alan Gilbert <[email protected]>

Cc Peter Xu <[email protected]>

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

---
 migration/migration.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

migration/migration.c +6 -13

index 5d322eb9d6..ded151b1bf 100644
@@ -438,15 +438,13 @@ static void process_incoming_migration_co(void *opaque)
         /* Make sure all file formats flush their mutable metadata */
         bdrv_invalidate_cache_all(&local_err);
         if (local_err) {
-            migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                    MIGRATION_STATUS_FAILED);
             error_report_err(local_err);
-            exit(EXIT_FAILURE);
+            goto fail;
         }
 
         if (colo_init_ram_cache() < 0) {
             error_report("Init ram cache failed");
-            exit(EXIT_FAILURE);
+            goto fail;
         }
 
         qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",

@@ -461,20 +459,22 @@ static void process_incoming_migration_co(void *opaque)
     }
 
     if (ret < 0) {
-        Error *local_err = NULL;
-
-        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
         error_report("load of migration failed: %s", strerror(-ret));
-        qemu_fclose(mis->from_src_file);
-        if (multifd_load_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
-        exit(EXIT_FAILURE);
+        goto fail;
     }
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
     qemu_bh_schedule(mis->bh);
     mis->migration_incoming_co = NULL;
+    return;
+fail:
+    local_err = NULL;

Signature · 1 line

2.13.7

Quoted 66 lines · * Fei Li ([email protected]) wrote:

In the current code, if process_incoming_migration_co() fails we do the same error handing: set the error state, close the source file, do the cleanup for multifd, and then exit(EXIT_FAILURE). To make the code clearer, add a "goto fail" to unify the error handling.

Cc Markus Armbruster <[email protected]>

Cc Dr. David Alan Gilbert <[email protected]>

Cc Peter Xu <[email protected]>

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

---
  migration/migration.c | 26 +++++++++++++-------------
  1 file changed, 13 insertions(+), 13 deletions(-)

migration/migration.c +13 -13

index 5d322eb9d6..ded151b1bf 100644
@@ -438,15 +438,13 @@ static void process_incoming_migration_co(void *opaque)
          /* Make sure all file formats flush their mutable metadata */
          bdrv_invalidate_cache_all(&local_err);
          if (local_err) {
-            migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                    MIGRATION_STATUS_FAILED);
              error_report_err(local_err);
-            exit(EXIT_FAILURE);
+            goto fail;
          }
 
          if (colo_init_ram_cache() < 0) {
              error_report("Init ram cache failed");
-            exit(EXIT_FAILURE);
+            goto fail;
          }
 
          qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",

@@ -461,20 +459,22 @@ static void process_incoming_migration_co(void *opaque)
      }
 
      if (ret < 0) {
-        Error *local_err = NULL;
-
-        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
          error_report("load of migration failed: %s", strerror(-ret));
-        qemu_fclose(mis->from_src_file);
-        if (multifd_load_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
-        exit(EXIT_FAILURE);
+        goto fail;
      }
      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
      qemu_bh_schedule(mis->bh);
      mis->migration_incoming_co = NULL;
+    return;
+fail:
+    local_err = NULL;
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_FAILED);
+    qemu_fclose(mis->from_src_file);
+    if (multifd_load_cleanup(&local_err) != 0) {
+        error_report_err(local_err);
+    }
+    exit(EXIT_FAILURE);
  }
 

migration/migration.c static void process_incoming_migration_co(void *opaque) · line 480

Quoted 1 line

migration/migration.c +0 -0

  static void migration_incoming_setup(QEMUFile *f)

OK, so this is really unifying the normal error case and the two colo-incoming error cases; so I think that's fine.

Reviewed-by Dr. David Alan Gilbert <[email protected]>

Quoted 4 lines

Signature · 2 lines

-- 
2.13.7

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

qemu_thread_create() abort()s on error. Not nice. Give it a return value and an Error ** argument, so it can return success/failure.

Considering qemu_thread_create() is quite widely used in qemu, split this into two steps: this patch passes the &error_abort to qemu_thread_create() everywhere, and the next 9 patches will improve on &error_abort for callers who need.

Cc Markus Armbruster <[email protected]>

Cc Paolo Bonzini <[email protected]>

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

---
 cpus.c                      | 23 +++++++++++++++--------
 dump.c                      |  3 ++-
 hw/misc/edu.c               |  4 +++-
 hw/ppc/spapr_hcall.c        |  4 +++-
 hw/rdma/rdma_backend.c      |  3 ++-
 hw/usb/ccid-card-emulated.c |  5 +++--
 include/qemu/thread.h       |  4 ++--
 io/task.c                   |  3 ++-
 iothread.c                  |  3 ++-
 migration/migration.c       | 11 ++++++++---
 migration/postcopy-ram.c    |  4 +++-
 migration/ram.c             | 12 ++++++++----
 migration/savevm.c          |  3 ++-
 tests/atomic_add-bench.c    |  3 ++-
 tests/iothread.c            |  2 +-
 tests/qht-bench.c           |  3 ++-
 tests/rcutorture.c          |  3 ++-
 tests/test-aio.c            |  2 +-
 tests/test-rcu-list.c       |  3 ++-
 ui/vnc-jobs.c               |  6 ++++--
 util/compatfd.c             |  6 ++++--
 util/oslib-posix.c          |  3 ++-
 util/qemu-thread-posix.c    | 27 ++++++++++++++++++++-------
 util/qemu-thread-win32.c    | 16 ++++++++++++----
 util/rcu.c                  |  3 ++-
 util/thread-pool.c          |  4 +++-
 26 files changed, 112 insertions(+), 51 deletions(-)

cpus.c +2 -1

index 0ddeeefc14..25df03326b 100644
@@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                  cpu->cpu_index);
 
+            /* TODO: let the callers handle the error instead of abort() here */
             qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 
         } else {

include/qemu/thread.h +2 -2

index 55d83a907c..12291f4ccd 100644
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
-                        void *arg, int mode);
+                        void *arg, int mode, Error **errp);
 void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c +15 -4

+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
     sigset_t set, oldset;
     int err;

@@ -511,7 +516,9 @@ void qemu_thread_create(QemuThread *thread, const char *name,
 
     err = pthread_attr_init(&attr);
     if (err) {
-        error_exit(err, __func__);
+        errno = err;
+        error_setg_errno(errp, errno, "pthread_attr_init failed");
+        return false;
     }
 
     if (mode == QEMU_THREAD_DETACHED) {

@@ -529,13 +536,19 @@ void qemu_thread_create(QemuThread *thread, const char *name,
 
     err = pthread_create(&thread->thread, &attr,
                          qemu_thread_start, qemu_thread_args);
-
-    if (err)
-        error_exit(err, __func__);
+    if (err) {
+        errno = err;
+        error_setg_errno(errp, errno, "pthread_create failed");
+        pthread_attr_destroy(&attr);
+        g_free(qemu_thread_args->name);
+        g_free(qemu_thread_args);
+        return false;
+    }
 
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
     pthread_attr_destroy(&attr);
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

util/qemu-thread-win32.c +12 -4

index 4a363ca675..57b1143e97 100644
@@ -20,6 +20,7 @@
 #include "qemu/thread.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 #include <process.h>
 
 static bool name_threads;

@@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void *),
-                       void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
     HANDLE hThread;
     struct QemuThreadData *data;

@@ -409,10 +410,17 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                       data, 0, &thread->tid);
     if (!hThread) {
-        error_exit(GetLastError(), __func__);
+        if (data->mode != QEMU_THREAD_DETACHED) {
+            DeleteCriticalSection(&data->cs);
+        }
+        error_setg_errno(errp, errno,
+                         "failed to create win32_start_routine");
+        g_free(data);
+        return false;
     }
     CloseHandle(hThread);
     thread->data = data;
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

Quoted 8 lines · Fei Li

qemu_thread_create() abort()s on error. Not nice. Give it a return value and an Error ** argument, so it can return success/failure.

Considering qemu_thread_create() is quite widely used in qemu, split this into two steps: this patch passes the &error_abort to qemu_thread_create() everywhere, and the next 9 patches will improve on &error_abort for callers who need.

Quoted 3 lines

Cc Markus Armbruster <[email protected]>

Cc Paolo Bonzini <[email protected]>

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

The commit message's title promises more than the patch delivers. Suggest:

qemu_thread: Make qemu_thread_create() take Error ** argument

The rest of the commit message is fine.

Quoted 51 lines

---

cpus.c                      | 23 +++++++++++++++--------
dump.c                      |  3 ++-
hw/misc/edu.c               |  4 +++-
hw/ppc/spapr_hcall.c        |  4 +++-
hw/rdma/rdma_backend.c      |  3 ++-
hw/usb/ccid-card-emulated.c |  5 +++--
include/qemu/thread.h       |  4 ++--
io/task.c                   |  3 ++-
iothread.c                  |  3 ++-
migration/migration.c       | 11 ++++++++---
migration/postcopy-ram.c    |  4 +++-
migration/ram.c             | 12 ++++++++----
migration/savevm.c          |  3 ++-
tests/atomic_add-bench.c    |  3 ++-
tests/iothread.c            |  2 +-
tests/qht-bench.c           |  3 ++-
tests/rcutorture.c          |  3 ++-
tests/test-aio.c            |  2 +-
tests/test-rcu-list.c       |  3 ++-
ui/vnc-jobs.c               |  6 ++++--
util/compatfd.c             |  6 ++++--
util/oslib-posix.c          |  3 ++-
util/qemu-thread-posix.c    | 27 ++++++++++++++++++++-------
util/qemu-thread-win32.c    | 16 ++++++++++++----
util/rcu.c                  |  3 ++-
util/thread-pool.c          |  4 +++-
26 files changed, 112 insertions(+), 51 deletions(-)

cpus.c +4 -2

index 0ddeeefc14..25df03326b 100644
@@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                 cpu->cpu_index);
 
+            /* TODO: let the callers handle the error instead of abort() here */
            qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 
        } else {
            /* share a single thread for all cpus with TCG */
            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
+            /* TODO: let the callers handle the error instead of abort() here */
            qemu_thread_create(cpu->thread, thread_name,
                               qemu_tcg_rr_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 

cpus.c static void qemu_tcg_init_vcpu(CPUState *cpu) · line 1977

Quoted 2 lines

cpus.c +0 -0

            single_tcg_halt_cond = cpu->halt_cond;
            single_tcg_cpu_thread = cpu->thread;

You add this TODO comment to 24 out of 37 calls. Can you give your reasons for adding it to some calls, but not to others?

[...]

Quoted 16 lines

include/qemu/thread.h +2 -2

index 55d83a907c..12291f4ccd 100644
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void *),
-                        void *arg, int mode);
+                        void *arg, int mode, Error **errp);
 void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);

[...]

Quoted 13 lines

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 507

Quoted 10 lines

util/qemu-thread-posix.c +4 -3

@@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
    return r;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+/*
+ * Return a boolean: true/false to indicate whether it succeeds.
+ * If fails, propagate the error to Error **errp and set the errno.
+ */

Let's write something that can pass as a function contract:

/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/

Personally, I'd return negative errno instead of false, and dispense with setting errno.

Quoted 85 lines

util/qemu-thread-posix.c +15 -4

+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
    sigset_t set, oldset;
    int err;

@@ -511,7 +516,9 @@ void qemu_thread_create(QemuThread *thread, const char *name,
 
    err = pthread_attr_init(&attr);
    if (err) {
-        error_exit(err, __func__);
+        errno = err;
+        error_setg_errno(errp, errno, "pthread_attr_init failed");
+        return false;
    }
 
    if (mode == QEMU_THREAD_DETACHED) {

@@ -529,13 +536,19 @@ void qemu_thread_create(QemuThread *thread, const char *name,
 
    err = pthread_create(&thread->thread, &attr,
                         qemu_thread_start, qemu_thread_args);
-
-    if (err)
-        error_exit(err, __func__);
+    if (err) {
+        errno = err;
+        error_setg_errno(errp, errno, "pthread_create failed");
+        pthread_attr_destroy(&attr);
+        g_free(qemu_thread_args->name);
+        g_free(qemu_thread_args);
+        return false;
+    }
 
    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
    pthread_attr_destroy(&attr);
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

util/qemu-thread-win32.c +12 -4

index 4a363ca675..57b1143e97 100644
@@ -20,6 +20,7 @@
 #include "qemu/thread.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 #include <process.h>
 
 static bool name_threads;

@@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
    return ret;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void *),
-                       void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
    HANDLE hThread;
    struct QemuThreadData *data;

@@ -409,10 +410,17 @@ void qemu_thread_create(QemuThread *thread, const char *name,
    hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                      data, 0, &thread->tid);
    if (!hThread) {
-        error_exit(GetLastError(), __func__);
+        if (data->mode != QEMU_THREAD_DETACHED) {
+            DeleteCriticalSection(&data->cs);
+        }
+        error_setg_errno(errp, errno,
+                         "failed to create win32_start_routine");
+        g_free(data);
+        return false;
    }
    CloseHandle(hThread);
    thread->data = data;
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

[...]

Quoted 12 lines · 在 2019年1月8日，01:18，Markus Armbruster <[email protected]> 写道：

Quoted 8 lines · Fei Li

qemu_thread_create() abort()s on error. Not nice. Give it a return value and an Error ** argument, so it can return success/failure.

Considering qemu_thread_create() is quite widely used in qemu, split this into two steps: this patch passes the &error_abort to qemu_thread_create() everywhere, and the next 9 patches will improve on &error_abort for callers who need.

Quoted 8 lines

Quoted 3 lines

Cc Markus Armbruster <[email protected]>

Cc Paolo Bonzini <[email protected]>

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

The commit message's title promises more than the patch delivers. Suggest:

qemu_thread: Make qemu_thread_create() take Error ** argument

Ok, thanks for the suggestion. :)

Quoted 54 lines

The rest of the commit message is fine.

Quoted 51 lines

---

cpus.c                      | 23 +++++++++++++++--------
dump.c                      |  3 ++-
hw/misc/edu.c               |  4 +++-
hw/ppc/spapr_hcall.c        |  4 +++-
hw/rdma/rdma_backend.c      |  3 ++-
hw/usb/ccid-card-emulated.c |  5 +++--
include/qemu/thread.h       |  4 ++--
io/task.c                   |  3 ++-
iothread.c                  |  3 ++-
migration/migration.c       | 11 ++++++++---
migration/postcopy-ram.c    |  4 +++-
migration/ram.c             | 12 ++++++++----
migration/savevm.c          |  3 ++-
tests/atomic_add-bench.c    |  3 ++-
tests/iothread.c            |  2 +-
tests/qht-bench.c           |  3 ++-
tests/rcutorture.c          |  3 ++-
tests/test-aio.c            |  2 +-
tests/test-rcu-list.c       |  3 ++-
ui/vnc-jobs.c               |  6 ++++--
util/compatfd.c             |  6 ++++--
util/oslib-posix.c          |  3 ++-
util/qemu-thread-posix.c    | 27 ++++++++++++++++++++-------
util/qemu-thread-win32.c    | 16 ++++++++++++----
util/rcu.c                  |  3 ++-
util/thread-pool.c          |  4 +++-
26 files changed, 112 insertions(+), 51 deletions(-)

cpus.c +4 -2

index 0ddeeefc14..25df03326b 100644
@@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                 cpu->cpu_index);
 
+            /* TODO: let the callers handle the error instead of abort() here */
            qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 
        } else {
            /* share a single thread for all cpus with TCG */
            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
+            /* TODO: let the callers handle the error instead of abort() here */
            qemu_thread_create(cpu->thread, thread_name,
                               qemu_tcg_rr_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 

Quoted 5 lines

cpus.c static void qemu_tcg_init_vcpu(CPUState *cpu) · line 1977

Quoted 2 lines

cpus.c +0 -0

            single_tcg_halt_cond = cpu->halt_cond;
            single_tcg_cpu_thread = cpu->thread;

You add this TODO comment to 24 out of 37 calls. Can you give your reasons for adding it to some calls, but not to others?

For those have TODO, I polish them in the next following patches, and for those do not have TODO I just let them use &error_abort.

Quoted 45 lines

[...]

Quoted 16 lines

include/qemu/thread.h +2 -2

index 55d83a907c..12291f4ccd 100644
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void *),
-                        void *arg, int mode);
+                        void *arg, int mode, Error **errp);
 void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);

[...]

Quoted 13 lines

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 507

Quoted 10 lines

util/qemu-thread-posix.c +4 -3

@@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
    return r;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+/*
+ * Return a boolean: true/false to indicate whether it succeeds.
+ * If fails, propagate the error to Error **errp and set the errno.
+ */

Let's write something that can pass as a function contract:

Quoted 10 lines

/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/

Thanks so much for amending! :)

Quoted 2 lines

Personally, I'd return negative errno instead of false, and dispense with setting errno.

Emm, I think I have replied this in last version, but due to several reasons I did not wait for your feedback and sent the v9. Sorry for that.. And I like to paste my two considerations here:
“- Actually only one caller needs the errno, that is the above qemu_signalfd_compat().

- For the returning value, I remember there's once a email thread talking about it: returning a bool (and let the passed errp hold the error message) is to keep the consistency with glib.

So IMO I am wondering whether it is really needed to change the bool (true/false) to int (0/-errno), just for that sole function: qemu_signalfd_compat() which needs the errno. Besides if we return -errno, for each caller we need add a local variable like “ret= qemu_thread_create()” to store the -errno.

Quoted 73 lines · fei

Quoted 12 lines · 在 2019年1月8日，01:18，Markus Armbruster <[email protected]> 写道：

Quoted 8 lines · Fei Li

qemu_thread_create() abort()s on error. Not nice. Give it a return value and an Error ** argument, so it can return success/failure.

Considering qemu_thread_create() is quite widely used in qemu, split this into two steps: this patch passes the &error_abort to qemu_thread_create() everywhere, and the next 9 patches will improve on &error_abort for callers who need.

Quoted 7 lines

Quoted 3 lines

Cc Markus Armbruster <[email protected]>

Cc Paolo Bonzini <[email protected]>

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

The commit message's title promises more than the patch delivers. Suggest:

qemu_thread: Make qemu_thread_create() take Error ** argument

Ok, thanks for the suggestion. :)

Quoted 53 lines

The rest of the commit message is fine.

Quoted 51 lines

---

cpus.c                      | 23 +++++++++++++++--------
dump.c                      |  3 ++-
hw/misc/edu.c               |  4 +++-
hw/ppc/spapr_hcall.c        |  4 +++-
hw/rdma/rdma_backend.c      |  3 ++-
hw/usb/ccid-card-emulated.c |  5 +++--
include/qemu/thread.h       |  4 ++--
io/task.c                   |  3 ++-
iothread.c                  |  3 ++-
migration/migration.c       | 11 ++++++++---
migration/postcopy-ram.c    |  4 +++-
migration/ram.c             | 12 ++++++++----
migration/savevm.c          |  3 ++-
tests/atomic_add-bench.c    |  3 ++-
tests/iothread.c            |  2 +-
tests/qht-bench.c           |  3 ++-
tests/rcutorture.c          |  3 ++-
tests/test-aio.c            |  2 +-
tests/test-rcu-list.c       |  3 ++-
ui/vnc-jobs.c               |  6 ++++--
util/compatfd.c             |  6 ++++--
util/oslib-posix.c          |  3 ++-
util/qemu-thread-posix.c    | 27 ++++++++++++++++++++-------
util/qemu-thread-win32.c    | 16 ++++++++++++----
util/rcu.c                  |  3 ++-
util/thread-pool.c          |  4 +++-
26 files changed, 112 insertions(+), 51 deletions(-)

cpus.c +4 -2

index 0ddeeefc14..25df03326b 100644
@@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                  cpu->cpu_index);
 
+            /* TODO: let the callers handle the error instead of abort() here */
             qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 
         } else {
             /* share a single thread for all cpus with TCG */
             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
+            /* TODO: let the callers handle the error instead of abort() here */
             qemu_thread_create(cpu->thread, thread_name,
                                qemu_tcg_rr_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
 

Quoted 5 lines

Quoted 4 lines

cpus.c static void qemu_tcg_init_vcpu(CPUState *cpu) · line 1977

Quoted 2 lines

cpus.c +0 -0

             single_tcg_halt_cond = cpu->halt_cond;
             single_tcg_cpu_thread = cpu->thread;

You add this TODO comment to 24 out of 37 calls. Can you give your reasons for adding it to some calls, but not to others?

For those have TODO, I polish them in the next following patches, and for those do not have TODO I just let them use &error_abort.

Please mention that in the commit message.

Quoted 54 lines

Quoted 44 lines

[...]

Quoted 16 lines

include/qemu/thread.h +2 -2

index 55d83a907c..12291f4ccd 100644
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
-                        void *arg, int mode);
+                        void *arg, int mode, Error **errp);
 void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);

[...]

Quoted 13 lines

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 507

Quoted 10 lines

util/qemu-thread-posix.c +4 -3

@@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
     return r;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+/*
+ * Return a boolean: true/false to indicate whether it succeeds.
+ * If fails, propagate the error to Error **errp and set the errno.
+ */

Let's write something that can pass as a function contract:

Quoted 10 lines

/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/

Quoted 5 lines

Thanks so much for amending! :)

Quoted 2 lines

Personally, I'd return negative errno instead of false, and dispense with setting errno.

Emm, I think I have replied this in last version, but due to several reasons I did not wait for your feedback and sent the v9. Sorry for that.. And I like to paste my two considerations here:
“- Actually only one caller needs the errno, that is the above qemu_signalfd_compat().

Yes.

Quoted 1 line

- For the returning value, I remember there's once a email thread talking about it: returning a bool (and let the passed errp hold the error message) is to keep the consistency with glib.

GLib doesn't discourage return types other than boolean. It only asks that if you return boolean, then true should mean success and false should mean failure. See

https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

under "Rules for use of GError", item "By convention, if you return a boolean value".

The discussion you remember was about a convention we used to enforce in QEMU, namely to avoid returning boolean success, and return void instead. That was basically a bad idea.

Quoted 1 line

So IMO I am wondering whether it is really needed to change the bool (true/false) to int (0/-errno), just for that sole function: qemu_signalfd_compat() which needs the errno. Besides if we return -errno, for each caller we need add a local variable like “ret= qemu_thread_create()” to store the -errno.

Well, you either assign the error code to errno just for that caller, or you return the error code just for that caller. I'd do the latter because I consider it slightly simpler. Compare

* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.

to

* On success, return zero.
* On failure, store an error through @errp and return negative errno.

where the second sentence describes just two instead of three actions.

[...]

Quoted 5 lines · Markus Armbruster

Quoted 3 lines · fei

Quoted 3 lines · 在 2019年1月8日，01:18，Markus Armbruster <[email protected]> 写道：

Fei Li <[email protected]> writes:

Quoted 65 lines

Quoted 35 lines

Quoted 25 lines

Quoted 13 lines

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 507

Quoted 10 lines

util/qemu-thread-posix.c +4 -3

@@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
     return r;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+/*
+ * Return a boolean: true/false to indicate whether it succeeds.
+ * If fails, propagate the error to Error **errp and set the errno.
+ */

Let's write something that can pass as a function contract:

Quoted 10 lines

/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/

Quoted 5 lines

Thanks so much for amending! :)

Quoted 2 lines

Personally, I'd return negative errno instead of false, and dispense with setting errno.

Emm, I think I have replied this in last version, but due to several reasons I did not wait for your feedback and sent the v9. Sorry for that.. And I like to paste my two considerations here:
“- Actually only one caller needs the errno, that is the above qemu_signalfd_compat().

Yes.

Quoted 1 line

- For the returning value, I remember there's once a email thread talking about it: returning a bool (and let the passed errp hold the error message) is to keep the consistency with glib.

GLib doesn't discourage return types other than boolean. It only asks that if you return boolean, then true should mean success and false should mean failure. See

https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

under "Rules for use of GError", item "By convention, if you return a boolean value".

The discussion you remember was about a convention we used to enforce in QEMU, namely to avoid returning boolean success, and return void instead. That was basically a bad idea.

Quoted 1 line

So IMO I am wondering whether it is really needed to change the bool (true/false) to int (0/-errno), just for that sole function: qemu_signalfd_compat() which needs the errno. Besides if we return -errno, for each caller we need add a local variable like “ret= qemu_thread_create()” to store the -errno.

Well, you either assign the error code to errno just for that caller, or you return the error code just for that caller. I'd do the latter because I consider it slightly simpler. Compare

* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.

Quoted 8 lines

to

* On success, return zero.
* On failure, store an error through @errp and return negative errno.

where the second sentence describes just two instead of three actions.

[...]

Ok, decribing two actions than three is indeed simpler. But I still have one uncertain:
for those callers do not need the errno value, could we just check the return value
to see whether it is negative, but not cache the unused return value? I mean

In the caller:

{...
    if (qemu_thread_create() < 0) {// do some cleanup} ...}

instead of

{    int ret;
...
    ret = qemu_thread_create();
    if (ret < 0) { //do some cleanup }

...}

As the first one can lessen quite a lot of codes. :)

Have a nice day, thanks

Fei

Quoted 6 lines · Fei Li

Quoted 6 lines · Markus Armbruster

Quoted 3 lines · fei

Quoted 3 lines · 在 2019年1月8日，01:18，Markus Armbruster <[email protected]> 写道：

Fei Li <[email protected]> writes:

[...]

Quoted 79 lines

Quoted 65 lines

Quoted 35 lines

Quoted 25 lines

Quoted 13 lines

util/qemu-thread-posix.c +1 -0

index 865e476df5..39834b0551 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;
 

util/qemu-thread-posix.c static void *qemu_thread_start(void *args) · line 507

Quoted 10 lines

util/qemu-thread-posix.c +4 -3

@@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
    return r;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+/*
+ * Return a boolean: true/false to indicate whether it succeeds.
+ * If fails, propagate the error to Error **errp and set the errno.
+ */

Let's write something that can pass as a function contract:

Quoted 10 lines

/*
* Create a new thread with name @name
* The thread executes @start_routine() with argument @arg.
* The thread will be created in a detached state if @mode is
* QEMU_THREAD_DETACHED, and in a jounable state if it's
* QEMU_THREAD_JOINABLE.
* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.
*/

Quoted 5 lines

Thanks so much for amending! :)

Quoted 2 lines

Personally, I'd return negative errno instead of false, and dispense with setting errno.

Emm, I think I have replied this in last version, but due to several reasons I did not wait for your feedback and sent the v9. Sorry for that.. And I like to paste my two considerations here:
“- Actually only one caller needs the errno, that is the above qemu_signalfd_compat().

Yes.

Quoted 1 line

- For the returning value, I remember there's once a email thread talking about it: returning a bool (and let the passed errp hold the error message) is to keep the consistency with glib.

GLib doesn't discourage return types other than boolean. It only asks that if you return boolean, then true should mean success and false should mean failure. See

https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

under "Rules for use of GError", item "By convention, if you return a boolean value".

The discussion you remember was about a convention we used to enforce in QEMU, namely to avoid returning boolean success, and return void instead. That was basically a bad idea.

Quoted 1 line

So IMO I am wondering whether it is really needed to change the bool (true/false) to int (0/-errno), just for that sole function: qemu_signalfd_compat() which needs the errno. Besides if we return -errno, for each caller we need add a local variable like “ret= qemu_thread_create()” to store the -errno.

Well, you either assign the error code to errno just for that caller, or you return the error code just for that caller. I'd do the latter because I consider it slightly simpler. Compare

* On success, return true.
* On failure, set @errno, store an error through @errp and return
* false.

Quoted 8 lines

to

* On success, return zero.
* On failure, store an error through @errp and return negative errno.

where the second sentence describes just two instead of three actions.

[...]

Ok, decribing two actions than three is indeed simpler. But I still have one uncertain:
for those callers do not need the errno value, could we just check the return value
to see whether it is negative, but not cache the unused return value? I mean

Quoted 5 lines

In the caller:

{...

    if (qemu_thread_create() < 0) {// do some cleanup}

...}

This is just fine when you handle all errors the same.

Quoted 2 lines

instead of

Quoted 6 lines

{ int ret;
...

    ret = qemu_thread_create();
    if (ret < 0) { //do some cleanup }

...}

I'd object to this one unless the value of @ret gets used elsewhere.

Quoted 6 lines

As the first one can lessen quite a lot of codes. :)

Have a nice day, thanks

Fei

Utilize the existed errp to propagate the error instead of the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc Marc-André Lureau <[email protected]>

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

---
 dump.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

dump.c +0 -0

index c35d6ddd22..ef5ea324fa 100644

dump.c +0 -0

     } else {
         /* sync dump */
         dump_process(s, errp);

Quoted 15 lines · Fei Li

Utilize the existed errp to propagate the error instead of the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc Marc-André Lureau <[email protected]>

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

---
dump.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)

dump.c +0 -0

index c35d6ddd22..ef5ea324fa 100644

dump.c void qmp_dump_guest_memory(bool paging, const char *file, · line 2026

Quoted 10 lines

dump.c +4 -3

@@ -2020,9 +2020,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
    if (detach_p) {
        /* detached dump */
        s->detached = true;
-        /* TODO: let the further caller handle the error instead of abort() */
-        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
-                           s, QEMU_THREAD_DETACHED, &error_abort);
+        if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+                           s, QEMU_THREAD_DETACHED, errp)) {
+            /* keep 'if' here in case there is further error handling logic */
+        }

I don't think keeping the conditional "just in case" is worthwhile. Plain

          qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
                             s, QEMU_THREAD_DETACHED, errp);

should do fine.

Quoted 4 lines

dump.c +0 -0

    } else {
        /* sync dump */
        dump_process(s, errp);

Utilize the existed errp to propagate the error instead of the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc Jiri Slaby <[email protected]>

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

---
 hw/misc/edu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/misc/edu.c +0 -0

index 3f4ba7ded3..011fe6e0b7 100644

   static void pci_edu_uninit(PCIDevice *pdev)
   {
       EduState *edu = EDU(pdev);

       qemu_mutex_lock(&edu->thr_mutex);
       edu->stopping = true;
       qemu_mutex_unlock(&edu->thr_mutex);
       qemu_cond_signal(&edu->thr_cond);
       qemu_thread_join(&edu->thread);

       qemu_cond_destroy(&edu->thr_cond);
       qemu_mutex_destroy(&edu->thr_mutex);

       timer_del(&edu->dma_timer);

}

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

Quoted 14 lines · Markus Armbruster

   static void pci_edu_uninit(PCIDevice *pdev)
   {
       EduState *edu = EDU(pdev);

       qemu_mutex_lock(&edu->thr_mutex);
       edu->stopping = true;
       qemu_mutex_unlock(&edu->thr_mutex);
       qemu_cond_signal(&edu->thr_cond);
       qemu_thread_join(&edu->thread);

       qemu_cond_destroy(&edu->thr_cond);
       qemu_mutex_destroy(&edu->thr_mutex);

       timer_del(&edu->dma_timer);

Quoted 3 lines

}

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

I don't know, the MSI support was added in:
commit eabb5782f70b4a10975b24ccd7129929a05ac932

Author: Peter Xu <[email protected]>
Date:   Wed Sep 28 21:03:39 2016 +0800

hw/misc/edu: support MSI interrupt

Hence CCing Peter.

Quoted 26 lines · Jiri Slaby

Quoted 14 lines · Markus Armbruster

   static void pci_edu_uninit(PCIDevice *pdev)
   {
       EduState *edu = EDU(pdev);

       qemu_mutex_lock(&edu->thr_mutex);
       edu->stopping = true;
       qemu_mutex_unlock(&edu->thr_mutex);
       qemu_cond_signal(&edu->thr_cond);
       qemu_thread_join(&edu->thread);

       qemu_cond_destroy(&edu->thr_cond);
       qemu_mutex_destroy(&edu->thr_mutex);

       timer_del(&edu->dma_timer);

Quoted 3 lines

}

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

I don't know, the MSI support was added in:
commit eabb5782f70b4a10975b24ccd7129929a05ac932

Author: Peter Xu <[email protected]>
Date:   Wed Sep 28 21:03:39 2016 +0800

hw/misc/edu: support MSI interrupt

Quoted 1 line

Hence CCing Peter.

Hi, Jiri, Markus, Fei,

IMHO msi_uninit() is optional since it only operates on the config space of the device to remove the capability or fix up the flags without really doing any real destruction of objects so nothing will be leaked (unlike msix_uninit, which should be required).

But I do
agree that calling msi_uninit() could be even nicer here.

Anyone would like to post a patch? Or should I?

Quoted 27 lines · Peter Xu

Quoted 26 lines · Jiri Slaby

Quoted 16 lines · Markus Armbruster

    static void pci_edu_uninit(PCIDevice *pdev)
    {
        EduState *edu = EDU(pdev);

        qemu_mutex_lock(&edu->thr_mutex);
        edu->stopping = true;
        qemu_mutex_unlock(&edu->thr_mutex);
        qemu_cond_signal(&edu->thr_cond);
        qemu_thread_join(&edu->thread);

        qemu_cond_destroy(&edu->thr_cond);
        qemu_mutex_destroy(&edu->thr_mutex);

        timer_del(&edu->dma_timer);
    }

Quoted 1 line

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

I don't know, the MSI support was added in:
commit eabb5782f70b4a10975b24ccd7129929a05ac932

Author: Peter Xu <[email protected]>
Date:   Wed Sep 28 21:03:39 2016 +0800

hw/misc/edu: support MSI interrupt

Hence CCing Peter.

Quoted 6 lines

Hi, Jiri, Markus, Fei,

IMHO msi_uninit() is optional since it only operates on the config space of the device to remove the capability or fix up the flags without really doing any real destruction of objects so nothing will be leaked (unlike msix_uninit, which should be required).

Michael, Marcel, is neglecting to call msi_uninit() okay, a harmless bug, or a harmful bug?

Quoted 4 lines

But I do
agree that calling msi_uninit() could be even nicer here.

Anyone would like to post a patch? Or should I?

Please coordinate fixing this with Fei Li.

Quoted 35 lines · Markus Armbruster

Quoted 33 lines · Peter Xu

Quoted 18 lines · Jiri Slaby

Quoted 16 lines · Markus Armbruster

    static void pci_edu_uninit(PCIDevice *pdev)
    {
        EduState *edu = EDU(pdev);

        qemu_mutex_lock(&edu->thr_mutex);
        edu->stopping = true;
        qemu_mutex_unlock(&edu->thr_mutex);
        qemu_cond_signal(&edu->thr_cond);
        qemu_thread_join(&edu->thread);

        qemu_cond_destroy(&edu->thr_cond);
        qemu_mutex_destroy(&edu->thr_mutex);

        timer_del(&edu->dma_timer);
    }

Quoted 1 line

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

Quoted 8 lines

I don't know, the MSI support was added in:
commit eabb5782f70b4a10975b24ccd7129929a05ac932

Author: Peter Xu <[email protected]>
Date:   Wed Sep 28 21:03:39 2016 +0800

hw/misc/edu: support MSI interrupt

Hence CCing Peter.

Hi, Jiri, Markus, Fei,

IMHO msi_uninit() is optional since it only operates on the config space of the device to remove the capability or fix up the flags without really doing any real destruction of objects so nothing will be leaked (unlike msix_uninit, which should be required).

Quoted 2 lines

Michael, Marcel, is neglecting to call msi_uninit() okay, a harmless bug, or a harmful bug?

Kindly ping. :)

If corresponding change is needed, I'd like to do the update in the next version.

Quoted 36 lines · Fei Li

Quoted 35 lines · Markus Armbruster

Quoted 33 lines · Peter Xu

Quoted 18 lines · Jiri Slaby

Quoted 16 lines · Markus Armbruster

     static void pci_edu_uninit(PCIDevice *pdev)
     {
         EduState *edu = EDU(pdev);

         qemu_mutex_lock(&edu->thr_mutex);
         edu->stopping = true;
         qemu_mutex_unlock(&edu->thr_mutex);
         qemu_cond_signal(&edu->thr_cond);
         qemu_thread_join(&edu->thread);

         qemu_cond_destroy(&edu->thr_cond);
         qemu_mutex_destroy(&edu->thr_mutex);

         timer_del(&edu->dma_timer);
     }

Quoted 1 line

Preexisting: pci_edu_uninit() neglects to call msi_uninit(). Jiri?\

Quoted 8 lines

I don't know, the MSI support was added in:
commit eabb5782f70b4a10975b24ccd7129929a05ac932

Author: Peter Xu <[email protected]>
Date:   Wed Sep 28 21:03:39 2016 +0800

hw/misc/edu: support MSI interrupt

Hence CCing Peter.

Hi, Jiri, Markus, Fei,

IMHO msi_uninit() is optional since it only operates on the config space of the device to remove the capability or fix up the flags without really doing any real destruction of objects so nothing will be leaked (unlike msix_uninit, which should be required).

Quoted 6 lines

Quoted 2 lines

Michael, Marcel, is neglecting to call msi_uninit() okay, a harmless bug, or a harmful bug?

Kindly ping. :)

If corresponding change is needed, I'd like to do the update in the next version.

Fei,

If you're going to post the edu patch, please post it as a standalone patch. More patches mean harder that the series could be accepted quickly. So it would be good to split patches sometimes especially if they are irrelevant.

Regards,

Quoted 16 lines · Fei Li

Utilize the existed errp to propagate the error instead of the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc Jiri Slaby <[email protected]>

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

---
  hw/misc/edu.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

hw/misc/edu.c +0 -0

index 3f4ba7ded3..011fe6e0b7 100644
@@ -356,9 +356,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
 

hw/misc/edu.c static void pci_edu_realize(PCIDevice *pdev, Error **errp) · line 361

Quoted 8 lines

hw/misc/edu.c +3 -3

      qemu_mutex_init(&edu->thr_mutex);
      qemu_cond_init(&edu->thr_cond);
-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
-                       edu, QEMU_THREAD_JOINABLE, &error_abort);
+    if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+                            edu, QEMU_THREAD_JOINABLE, errp)) {
+        return;

You need to clean up everything that got initialized so far. You might want to call qemu_thread_create() earlier so you have less to clean up.

Just to make sure about how to do the cleanup. I notice that in device_set_realized(), the current code does not call "dc->unrealize(dev, NULL);" when dc->realize() fails.

        if (dc->realize) {
            dc->realize(dev, &local_err);
        }

        if (local_err != NULL) {
            goto fail;
        }

Is this on purpose? (Maybe due to some devices' realize() do their own cleanup
when fails? Sorry for the unsure, it is such a common function that I did not
check all. :( ) Or else, I prefer to do the cleanup in a unified manner, e.g. call "dc->unrealize(dev, NULL);" which is the pci_qdev_unrealize() for pci devices.

Quoted 2 lines · Fei Li

Just to make sure about how to do the cleanup. I notice that in device_set_realized(), the current code does not call "dc->unrealize(dev, NULL);" when dc->realize() fails.

Quoted 9 lines

        if (dc->realize) {
            dc->realize(dev, &local_err);
        }

        if (local_err != NULL) {
            goto fail;
        }

Quoted 3 lines

Is this on purpose? (Maybe due to some devices' realize() do their own cleanup
when fails? Sorry for the unsure, it is such a common function that I did not
check all. :( ) Or else, I prefer to do the cleanup in a unified manner, e.g. call "dc->unrealize(dev, NULL);" which is the pci_qdev_unrealize() for pci devices.

Yes, this is on purpose.

When a realize() method fails, it must revert everything it has done so far. Results in sane "either succeed completely, or fail and do nothing" semantics.

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · Fei Li

Just to make sure about how to do the cleanup. I notice that in device_set_realized(), the current code does not call "dc->unrealize(dev, NULL);" when dc->realize() fails.

Sorry that I am still uncertain.. I guess the code below I pasted was misleading,
actually I want to stress the *dc->unrealize() is not called when dc->realize() fails*
and the incomplete below "goto fail" does not include the dc->unrealize(), but instead the dc->unrealize() is included in later
child_realize_fail: & post_realize_fail:.

Emm, IMHO, I think when dc->realize() fails, the dc->unrealize() is either should be
called in the common function: device_set_realized() in a unified way, that is

        if (local_err != NULL) {
+          if (dc->unrealize) {
+              dc->unrealize(dev, local_err);
+          }
            goto fail;
        }

or do the unrealize() locally for each device earlier when dc->realize() fails.

But I checked several dc->realize() function, they did not call unrealize() when fails. Besides, it may mean verbose code if unrealize() locally. Thus I think the above way is the right way to do the cleanup when realize() fails.

Quoted 9 lines

Quoted 9 lines

        if (dc->realize) {
            dc->realize(dev, &local_err);
        }

        if (local_err != NULL) {
            goto fail;
        }

Quoted 8 lines

Quoted 3 lines

Is this on purpose? (Maybe due to some devices' realize() do their own cleanup
when fails? Sorry for the unsure, it is such a common function that I did not
check all. :( ) Or else, I prefer to do the cleanup in a unified manner, e.g. call "dc->unrealize(dev, NULL);" which is the pci_qdev_unrealize() for pci devices.

Yes, this is on purpose.

When a realize() method fails, it must revert everything it has done so far. Results in sane "either succeed completely, or fail and do nothing" semantics.

Have a nice day, thanks

Fei

Quoted 26 lines · Fei Li

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · Fei Li

Just to make sure about how to do the cleanup. I notice that in device_set_realized(), the current code does not call "dc->unrealize(dev, NULL);" when dc->realize() fails.

Sorry that I am still uncertain.. I guess the code below I pasted was misleading,
actually I want to stress the *dc->unrealize() is not called when dc->realize() fails*
and the incomplete below "goto fail" does not include the dc->unrealize(), but instead the dc->unrealize() is included in later
child_realize_fail: & post_realize_fail:.

Emm, IMHO, I think when dc->realize() fails, the dc->unrealize() is either should be
called in the common function: device_set_realized() in a unified way, that is

        if (local_err != NULL) {
+          if (dc->unrealize) {
+              dc->unrealize(dev, local_err);
+          }
            goto fail;
        }

Quoted 7 lines

or do the unrealize() locally for each device earlier when dc->realize() fails.

But I checked several dc->realize() function, they did not call unrealize() when fails. Besides, it may mean verbose code if unrealize() locally. Thus I think the above way is the right way to do the cleanup when realize() fails.

The realize() method is specified to either succeed completely or fail completely, i.e. fail and do nothing. The "either succeed completely or fail completely" aspect of the specification is sane and perfectly ordinary.

How a concrete implementation accomplishes "fail completely" is up to the implementation.

An implementation may choose to structure its FOO_realize() and FOO_unrealize() in a way that lets FOO_realize() call FOO_unrealize() to clean up on failure.

An implementation may also choose to clean up differently.

This freedom of choice is by design.

Changing the specification now would involve auditing and updating all realize() and unrealize() methods. Not going to happen without an extremely compelling reason.

Quoted 19 lines

Quoted 8 lines

Quoted 8 lines

          if (dc->realize) {
              dc->realize(dev, &local_err);
          }

          if (local_err != NULL) {
              goto fail;
          }

Quoted 8 lines

Quoted 3 lines

Is this on purpose? (Maybe due to some devices' realize() do their own cleanup
when fails? Sorry for the unsure, it is such a common function that I did not
check all. :( ) Or else, I prefer to do the cleanup in a unified manner, e.g. call "dc->unrealize(dev, NULL);" which is the pci_qdev_unrealize() for pci devices.

Yes, this is on purpose.

When a realize() method fails, it must revert everything it has done so far. Results in sane "either succeed completely, or fail and do nothing" semantics.

Have a nice day, thanks

Fei

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

---
 hw/ppc/spapr_hcall.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +1 -0

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
     sPAPRPendingHPT *pending = spapr->pending_hpt;
     uint64_t current_ram_size;
     int rc;
+    Error *local_err = NULL;
 
     if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
         return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
     pending->shift = shift;
     pending->ret = H_HARDWARE;
 

hw/ppc/spapr_hcall.c +1 -0

+    }
 
     spapr->pending_hpt = pending;
 

Quoted 6 lines · Fei Li

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature.

Quoted 21 lines

---
  hw/ppc/spapr_hcall.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +1 -0

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
      sPAPRPendingHPT *pending = spapr->pending_hpt;
      uint64_t current_ram_size;
      int rc;
+    Error *local_err = NULL;
 
      if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
          return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
      pending->shift = shift;
      pending->ret = H_HARDWARE;
 

hw/ppc/spapr_hcall.c static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, · line 547

Quoted 10 lines

hw/ppc/spapr_hcall.c +6 -4

-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending,
-                       QEMU_THREAD_DETACHED, &error_abort);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,
+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;

I also think H_HARDWARE would be a better choice here. Although the failure is due to a resource constraint, it's not because the guest asked for too much, just because the host is in dire straits. From the guest's point of view it's basically a hardware failure.

Quoted 5 lines

hw/ppc/spapr_hcall.c +1 -0

+    }
 
      spapr->pending_hpt = pending;
 

Quoted 4 lines · David Gibson

Quoted 3 lines · Fei Li

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Quoted 5 lines

Quoted 3 lines

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature.

Sorry that I am not sure whether I understand. Do you mean using &error_abort is more suitable for this handling, rather than report the &local_err & return a failure reason?

Quoted 25 lines

Quoted 25 lines

---
  hw/ppc/spapr_hcall.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +3 -4

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
      sPAPRPendingHPT *pending = spapr->pending_hpt;
      uint64_t current_ram_size;
      int rc;
+    Error *local_err = NULL;
      if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
          return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
      pending->shift = shift;
      pending->ret = H_HARDWARE;
-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending,
-                       QEMU_THREAD_DETACHED, &error_abort);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,

Quoted 8 lines

hw/ppc/spapr_hcall.c static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, · line 547

Quoted 4 lines

hw/ppc/spapr_hcall.c +4 -0

+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;

I also think H_HARDWARE would be a better choice here. Although the failure is due to a resource constraint, it's not because the guest asked for too much, just because the host is in dire straits. From the guest's point of view it's basically a hardware failure.

Ok, thanks. Will use H_HARDWARE instead.

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

Quoted 3 lines

Quoted 3 lines

hw/ppc/spapr_hcall.c +1 -0

+    }
      spapr->pending_hpt = pending;

Quoted 8 lines · 李菲

Quoted 7 lines · David Gibson

Quoted 6 lines · Fei Li

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

Quoted 5 lines

Quoted 2 lines

This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature.

Sorry that I am not sure whether I understand. Do you mean using &error_abort is more suitable for this handling, rather than report the &local_err & return a failure reason?

No, I just mean that context has been altered by a global change and the patch will need to be fixed up to cope with that.

Quoted 39 lines

Quoted 25 lines

Quoted 25 lines

---
   hw/ppc/spapr_hcall.c | 12 ++++++++----
   1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +3 -4

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
       sPAPRPendingHPT *pending = spapr->pending_hpt;
       uint64_t current_ram_size;
       int rc;
+    Error *local_err = NULL;
       if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
           return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
       pending->shift = shift;
       pending->ret = H_HARDWARE;
-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending,
-                       QEMU_THREAD_DETACHED, &error_abort);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,

Quoted 8 lines

hw/ppc/spapr_hcall.c static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, · line 547

Quoted 4 lines

hw/ppc/spapr_hcall.c +4 -0

+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;

I also think H_HARDWARE would be a better choice here. Although the failure is due to a resource constraint, it's not because the guest asked for too much, just because the host is in dire straits. From the guest's point of view it's basically a hardware failure.

Ok, thanks. Will use H_HARDWARE instead.

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

Quoted 2 lines

Quoted 2 lines

hw/ppc/spapr_hcall.c +1 -0

+    }
       spapr->pending_hpt = pending;

Quoted 9 lines · David Gibson

Quoted 8 lines · 李菲

Quoted 7 lines · David Gibson

Quoted 6 lines · Fei Li

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature.

Sorry that I am not sure whether I understand. Do you mean using &error_abort is more suitable for this handling, rather than report the &local_err & return a failure reason?

No, I just mean that context has been altered by a global change and the patch will need to be fixed up to cope with that.

Just to be clearer: does the "global change" mean the "[patch 06/16]
qemu_thread: Make qemu_thread_create() handle errors properly", or another patch not in this patch series?

If it means the [patch 06/16], I want to explain more: the 06/16 handles all qemu_thread_create() by passing &error_abort as the parameter, and the following patches are to improve on the &error_abort for callers who can handle more properly. E.g. if qemu_thread_create() fails in h_resize_hpt_prepare(),
I think reporting the &local_err & returning the failure reason is more proper
than just abort() inside qemu_thread_create() when calls error_setg_errno().

In other words, this patch is actually written to apply to patch 06. And I have
no clue where it needs to be fixed up. Please correct me if I understand wrong.

Have a nice day, thanks :)

Fei

Quoted 39 lines

Quoted 39 lines

Quoted 25 lines

Quoted 25 lines

---
   hw/ppc/spapr_hcall.c | 12 ++++++++----
   1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +3 -4

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
       sPAPRPendingHPT *pending = spapr->pending_hpt;
       uint64_t current_ram_size;
       int rc;
+    Error *local_err = NULL;
       if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
           return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
       pending->shift = shift;
       pending->ret = H_HARDWARE;
-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending,
-                       QEMU_THREAD_DETACHED, &error_abort);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,

Quoted 8 lines

hw/ppc/spapr_hcall.c static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, · line 547

Quoted 4 lines

hw/ppc/spapr_hcall.c +4 -0

+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;

I also think H_HARDWARE would be a better choice here. Although the failure is due to a resource constraint, it's not because the guest asked for too much, just because the host is in dire straits. From the guest's point of view it's basically a hardware failure.

Ok, thanks. Will use H_HARDWARE instead.

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

Quoted 2 lines

Quoted 2 lines

hw/ppc/spapr_hcall.c +1 -0

+    }
       spapr->pending_hpt = pending;

Quoted 36 lines · Fei Li

Quoted 9 lines · David Gibson

Quoted 8 lines · 李菲

Quoted 7 lines · David Gibson

Quoted 6 lines · Fei Li

Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort.

Cc Markus Armbruster <[email protected]>

Cc David Gibson <[email protected]>

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

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature.

Sorry that I am not sure whether I understand. Do you mean using &error_abort is more suitable for this handling, rather than report the &local_err & return a failure reason?

No, I just mean that context has been altered by a global change and the patch will need to be fixed up to cope with that.

Just to be clearer: does the "global change" mean the "[patch 06/16]
qemu_thread: Make qemu_thread_create() handle errors properly", or another patch not in this patch series?

If it means the [patch 06/16], I want to explain more: the 06/16 handles all qemu_thread_create() by passing &error_abort as the parameter, and the following patches are to improve on the &error_abort for callers who can handle more properly. E.g. if qemu_thread_create() fails in h_resize_hpt_prepare(),
I think reporting the &local_err & returning the failure reason is more proper
than just abort() inside qemu_thread_create() when calls error_setg_errno().

In other words, this patch is actually written to apply to patch 06. And I have
no clue where it needs to be fixed up. Please correct me if I understand wrong.

Quoted 1 line

Have a nice day, thanks :)

Ah, sorry. Since I was only CCed on this patch, not the rest of the series, I assumed it was independent and didn't think to check the earlier patches of the series.

So, yes, I think the global change I'm referring to is 6/16, which I didn't have, so that explains the problem.

In that case it's probably best if this goes in via the same tree the rest of the series is going to. So, with the H_HARDWARE change made:

Acked-by David Gibson <[email protected]>

Quoted 43 lines

Fei

Quoted 38 lines

Quoted 38 lines

Quoted 24 lines

Quoted 24 lines

---
    hw/ppc/spapr_hcall.c | 12 ++++++++----
    1 file changed, 8 insertions(+), 4 deletions(-)

hw/ppc/spapr_hcall.c +3 -4

index 5bc2cf4540..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
        sPAPRPendingHPT *pending = spapr->pending_hpt;
        uint64_t current_ram_size;
        int rc;
+    Error *local_err = NULL;
        if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
            return H_AUTHORITY;

@@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
        pending->shift = shift;
        pending->ret = H_HARDWARE;
-    /* TODO: let the further caller handle the error instead of abort() here */
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending,
-                       QEMU_THREAD_DETACHED, &error_abort);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,

Quoted 8 lines

hw/ppc/spapr_hcall.c static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, · line 547

Quoted 4 lines

hw/ppc/spapr_hcall.c +4 -0

+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;

I also think H_HARDWARE would be a better choice here. Although the failure is due to a resource constraint, it's not because the guest asked for too much, just because the host is in dire straits. From the guest's point of view it's basically a hardware failure.

Ok, thanks. Will use H_HARDWARE instead.

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

Quoted 2 lines

Quoted 2 lines

hw/ppc/spapr_hcall.c +1 -0

+    }
        spapr->pending_hpt = pending;