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

["Daniel P. Berrangé" <berrange@redhat.com>]

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

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

likewise for the rest of the patch below...


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|