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 <address@hidden>
> > ---
> >   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