On Mon, May 22, 2017 at 09:41:48AM +0200, Markus Armbruster wrote: > Greg Kurz <gr...@kaod.org> writes: > > > On Sat, 20 May 2017 16:45:09 +1000 > > David Gibson <da...@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 <gr...@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
signature.asc
Description: PGP signature