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

[Fei Li]

On 09/06/2018 01:15 AM, Eric Blake wrote:
> Adding Markus, as the maintainer of Error APIs, to make sure he sees 
> this thread.
Thanks. :)
> 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/
Thanks for pointing this out! :)
> > > 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.
Thanks for providing these instructive discussions, I read the link, and 
yes,
I agree with "return a bool: false/true inside the callee". Just as the 
following:

    if (!foo(arg, errp)) { // assuming foo() returns a bool: false/true
        handle the error...
    }

As this is "Nicely concise." :)
I will prepare the next version according to all the comments.

Have a nice day, thanks all
Fei