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(-)

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 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,
> 

-- 
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