| Message ID | 149518993198.24289.7946457258780782948.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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(). > } > 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. > + return; > + } > } > } > >
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. 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. :) > > + return; > > + } > > } > > } > > > > >
On Sun, May 21, 2017 at 07:03:33PM +0200, Greg Kurz wrote: > 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. This isn't the case here but... Fair point; patch applied to ppc-for-2.10. > > > > } > > > 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. :) > > > > + return; > > > + } > > > } > > > } > > > > > > > > >
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. > 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. Having both an Error parameter and an error return value begs the question whether the two agree. 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 >> > + return; >> > + } >> > } >> > } >> > >> > >>
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.
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); } 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) { + return; + } } }
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> --- v2: - total rewrite --- hw/ppc/spapr.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)