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

[Eric Blake]

Adding Markus, as the maintainer of Error APIs, to make sure he sees 
this thread.

On 09/04/2018 06:22 AM, Daniel P. Berrangé wrote:
> On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:

In the subject line: s/paramater/parameter/

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

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

Returning a bool introduces an interesting semantic question on whether 
0 is success or failure (-1/0 vs. false/true means you have to pay 
attention to return type to decide between !func() or func()<0 when 
using the return value to learn if errp was set).  But Markus has 
expressed some thoughts on the matter before:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00124.html

and favored a bool return if only for consistency with glib and to avoid 
abuse of trying to overload the returned int when using errp is better.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org