| Message ID | 149484837602.20089.6188552126304330653.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 05/15/2017 01:39 PM, Greg Kurz wrote: > The xics_system_init() function passes its errp argument to xics_kvm_init(). > If the call fails and the user requested in-kernel irqchip, it then ends up > passing a NULL Error * to error_reportf_err() and the error message is > silently dropped. > > Passing an errp argument is generally wrong, unless you really just need > to convey an error to the caller. > > This patch converts xics_system_init() to *only* pass a pointer to its own > Error * and then to propagate it with error_propagate(), as recommended > in error.h. > > The local_err name is used for consistency with the rest of the code. > > Signed-off-by: Greg Kurz <groug@kaod.org> Thanks for the fix. Reviewed-by: Cédric Le Goater <clg@kaod.org> C. > --- > hw/ppc/spapr.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f477d7b8a210..44f7dc7f40e9 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > { > sPAPRMachineState *spapr = SPAPR_MACHINE(machine); > + Error *local_err = NULL; > > if (kvm_enabled()) { > - Error *err = NULL; > - > if (machine_kernel_irqchip_allowed(machine) && > - !xics_kvm_init(spapr, errp)) { > + !xics_kvm_init(spapr, &local_err)) { > 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, > + &local_err); > } > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > - error_reportf_err(err, > + error_reportf_err(local_err, > "kernel_irqchip requested but unavailable: "); > exit(EXIT_FAILURE); > } else { > - error_free(err); > + error_free(local_err); > } > } > > if (!spapr->ics) { > xics_spapr_init(spapr); > spapr->icp_type = TYPE_ICP; > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp); > + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, > + &local_err); > } > + > + error_propagate(errp, local_err); > } > > static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu, >
On Mon, May 15, 2017 at 01:39:36PM +0200, Greg Kurz wrote: > The xics_system_init() function passes its errp argument to xics_kvm_init(). > If the call fails and the user requested in-kernel irqchip, it then ends up > passing a NULL Error * to error_reportf_err() and the error message is > silently dropped. > > Passing an errp argument is generally wrong, unless you really just need > to convey an error to the caller. > > This patch converts xics_system_init() to *only* pass a pointer to its own > Error * and then to propagate it with error_propagate(), as recommended > in error.h. > > The local_err name is used for consistency with the rest of the code. > > Signed-off-by: Greg Kurz <groug@kaod.org> The change is good, but will need updating to work with the problem in the previous patch. > --- > hw/ppc/spapr.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f477d7b8a210..44f7dc7f40e9 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > { > sPAPRMachineState *spapr = SPAPR_MACHINE(machine); > + Error *local_err = NULL; > > if (kvm_enabled()) { > - Error *err = NULL; > - > if (machine_kernel_irqchip_allowed(machine) && > - !xics_kvm_init(spapr, errp)) { > + !xics_kvm_init(spapr, &local_err)) { > 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, > + &local_err); > } > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > - error_reportf_err(err, > + error_reportf_err(local_err, > "kernel_irqchip requested but unavailable: "); > exit(EXIT_FAILURE); > } else { > - error_free(err); > + error_free(local_err); > } > } > > if (!spapr->ics) { > xics_spapr_init(spapr); > spapr->icp_type = TYPE_ICP; > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp); > + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, > + &local_err); > } > + > + error_propagate(errp, local_err); > } > > static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu, >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index f477d7b8a210..44f7dc7f40e9 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr, static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) { sPAPRMachineState *spapr = SPAPR_MACHINE(machine); + Error *local_err = NULL; if (kvm_enabled()) { - Error *err = NULL; - if (machine_kernel_irqchip_allowed(machine) && - !xics_kvm_init(spapr, errp)) { + !xics_kvm_init(spapr, &local_err)) { 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, + &local_err); } if (machine_kernel_irqchip_required(machine) && !spapr->ics) { - error_reportf_err(err, + error_reportf_err(local_err, "kernel_irqchip requested but unavailable: "); exit(EXIT_FAILURE); } else { - error_free(err); + error_free(local_err); } } if (!spapr->ics) { xics_spapr_init(spapr); spapr->icp_type = TYPE_ICP; - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp); + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, + &local_err); } + + error_propagate(errp, local_err); } static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
The xics_system_init() function passes its errp argument to xics_kvm_init(). If the call fails and the user requested in-kernel irqchip, it then ends up passing a NULL Error * to error_reportf_err() and the error message is silently dropped. Passing an errp argument is generally wrong, unless you really just need to convey an error to the caller. This patch converts xics_system_init() to *only* pass a pointer to its own Error * and then to propagate it with error_propagate(), as recommended in error.h. The local_err name is used for consistency with the rest of the code. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)