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

[Fei Li <fli@suse.com>]

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