Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5182 bytes --]

On Mon, May 22, 2017 at 09:41:48AM +0200, Markus Armbruster wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
> > On Sat, 20 May 2017 16:45:09 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> >> On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
> >> > If the user explicitely asked for kernel-irqchip support and "xics-kvm"
> >> > initialization fails, we shouldn't fallback to emulated "xics" as we
> >> > do now. It is also awkward to print an error message when we have an
> >> > errp pointer argument.
> >> > 
> >> > Let's use the errp argument to report the error and let the caller decide.
> >> > This simplifies the code as we don't need a local Error * here.
> >> > 
> >> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> >> 
> >> Concept looks good, but..
> >> 
> >> > ---
> >> > v2: - total rewrite
> >> > ---
> >> >  hw/ppc/spapr.c |   13 ++++++-------
> >> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >> > 
> >> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> > index 91f7434861a8..75e298b4c6be 100644
> >> > --- a/hw/ppc/spapr.c
> >> > +++ b/hw/ppc/spapr.c
> >> > @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >> >  
> >> >      if (kvm_enabled()) {
> >> > -        Error *err = NULL;
> >> > -
> >> >          if (machine_kernel_irqchip_allowed(machine) &&
> >> >              !xics_kvm_init(spapr, errp)) {
> >> >              spapr->icp_type = TYPE_KVM_ICP;
> >> > -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, &err);
> >> > +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);  
> >> 
> >> I believe there are reasons you're not supposed to just pass an errp
> >> through to a subordinate function.  Instead you're supposed to have a
> >> local Error * and use error_propagate().
> >> 
> >
> > You only need to have a local Error * if it is used to check the return status
> > of the function (ie, you cannot check *errp because errp could be NULL) as
> > described in error.h.
> 
> Correct.  Quote:
> 
>  * Receive an error and pass it on to the caller:
>  *     Error *err = NULL;
>  *     foo(arg, &err);
>  *     if (err) {
>  *         handle the error...
>  *         error_propagate(errp, err);
>  *     }
>  * where Error **errp is a parameter, by convention the last one.
>  *
>  * Do *not* "optimize" this to
>  *     foo(arg, errp);
>  *     if (*errp) { // WRONG!
>  *         handle the error...
>  *     }
>  * because errp may be NULL!
>  *
>  * But when all you do with the error is pass it on, please use
>  *     foo(arg, errp);
>  * for readability.

So, I already merged based on Greg's comment, but it's nice to have
confirmation; thanks Markus.


> >                       This isn't the case here but...
> >
> >> >          }
> >> >          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> >> > -            error_reportf_err(err,
> >> > -                              "kernel_irqchip requested but unavailable: ");
> >> > -        } else {
> >> > -            error_free(err);
> >> > +            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> >> > +            return;
> >> >          }
> >> >      }
> >> >  
> >> > @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >> >          xics_spapr_init(spapr);
> >> >          spapr->icp_type = TYPE_ICP;
> >> >          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> >> > +        if (!spapr->ics) {  
> >> 
> >> It would also be more standard to check the returned error, rather
> >> than the other result.
> >> 
> >
> > ... if you prefer to use a local Error *, I'll gladly do that. :)
> 
> Opinions and practice vary on this one.
> 
> I prefer checking the return value because it lets me avoid the
> error_propagate() boiler-plate more often.

Noted for future reference.

> Having both an Error parameter and an error return value begs the
> question whether the two agree.

[Irrelevant aside: this is not what "begging the question" means.  Or
 at least, it's not what it used to mean; it's probably a lost cause
 at this point, even with those who don't get a free pass for being
 non-native speakers.  https://en.wikipedia.org/wiki/Begging_the_question]
 
> You can assert they do, but it's distracting.  We generally don't.
> 
> When there's no success value to transmit, you avoid the problem by
> making the function return void.  We used to favor that, but it has
> turned out not to be a success, because it leads to cumbersome code.
> For what it's worth, GLib wants you to transmit success / failure in the
> return value, too:
> 
> https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules

Also noted, thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]