Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate

[Fei Li]

On 09/04/2018 07:22 PM, Daniel P. Berrangé wrote:
On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
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 final caller check it.

Signed-off-by: Fei Li <f...@suse.com>
---
  cpus.c                          | 32 +++++++++++++++++++-------------
  include/qom/cpu.h               |  2 +-
  target/alpha/cpu.c              |  6 +++++-
  target/arm/cpu.c                |  6 +++++-
  target/cris/cpu.c               |  6 +++++-
  target/hppa/cpu.c               |  6 +++++-
  target/i386/cpu.c               |  6 +++++-
  target/lm32/cpu.c               |  6 +++++-
  target/m68k/cpu.c               |  6 +++++-
  target/microblaze/cpu.c         |  6 +++++-
  target/mips/cpu.c               |  6 +++++-
  target/moxie/cpu.c              |  6 +++++-
  target/nios2/cpu.c              |  6 +++++-
  target/openrisc/cpu.c           |  6 +++++-
  target/ppc/translate_init.inc.c |  6 +++++-
  target/riscv/cpu.c              |  6 +++++-
  target/s390x/cpu.c              |  5 ++++-
  target/sh4/cpu.c                |  6 +++++-
  target/sparc/cpu.c              |  6 +++++-
  target/tilegx/cpu.c             |  6 +++++-
  target/tricore/cpu.c            |  6 +++++-
  target/unicore32/cpu.c          |  6 +++++-
  target/xtensa/cpu.c             |  6 +++++-
  23 files changed, 124 insertions(+), 35 deletions(-)

diff --git a/cpus.c b/cpus.c
index 8ee6e5db93..41efddc218 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1898,7 +1898,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;
@@ -1954,7 +1954,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];
  
@@ -1971,7 +1971,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];
  
@@ -1984,7 +1984,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];
  
@@ -2002,7 +2002,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];
  
@@ -2018,7 +2018,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];
  
@@ -2031,11 +2031,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
                         QEMU_THREAD_JOINABLE);
  }
  
-void qemu_init_vcpu(CPUState *cpu)
+void 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,
@@ -2046,17 +2047,22 @@ 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;
      }
I'd be inclined to make this method return a boolean, so....


diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index b08078e7fc..5b0b4892f2 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error 
**errp)
          return;
      }
  
-    qemu_init_vcpu(cs);
+    qemu_init_vcpu(cs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
...this can be simplified to get rid of the local error object
Emm, for these arch_cpu_realizefn() functions, I notice they already 
have the local_err
and use error_propagate to handle the error. I guess the reason is what 
I explained in
patch1: considering their initial caller passes the &error_fatal, then 
just propagate and
let the further caller handle such error instead of exit() here.
So maybe just keep their tradition here?

Have a nice day, thanks
Fei

   if (!qemu_init_vcpu(cs, errp)) {
       return;
   }

likewise for the rest of the patch below...


Regards,
Daniel