xics/kvm: Improve error handling

[Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

Posted by Greg Kurz 6 years, 4 months ago

Passing both errp and &local_err to functions is a recipe for messing
things up.

Since we must use &local_err for icp_kvm_realize(), use &local_err
everywhere where rollback must happen and have a single call to
error_propagate() them all. While here, add errno to the error
message.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_kvm.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 377ff88701c2..c9e25fb051bb 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
         goto fail;
     }
 
     /* Create the KVM XICS device */
     rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
     if (rc < 0) {
-        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
+        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
         goto fail;
     }
 
@@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 
         icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto fail;
         }
     }
@@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
     return 0;
 
 fail:
+    error_propagate(errp, local_err);
     kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
     kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
     kvmppc_define_rtas_kernel_token(0, "ibm,int-on");

Re: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

Posted by David Gibson 6 years, 4 months ago

On Mon, Jun 17, 2019 at 03:46:52PM +0200, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
> 
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
>          goto fail;
>      }
>  
>      /* Create the KVM XICS device */
>      rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
>      if (rc < 0) {
> -        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>          goto fail;
>      }
>  
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>          icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto fail;
>          }
>      }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      return 0;
>  
>  fail:
> +    error_propagate(errp, local_err);
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> 

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

Re: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

Posted by Cédric Le Goater 6 years, 4 months ago

On 17/06/2019 15:46, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
> 
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>  hw/intc/xics_kvm.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
>          goto fail;
>      }
>  
>      /* Create the KVM XICS device */
>      rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
>      if (rc < 0) {
> -        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>          goto fail;
>      }
>  
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>          icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto fail;
>          }
>      }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      return 0;
>  
>  fail:
> +    error_propagate(errp, local_err);
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
>

[Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration
[Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()
[Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent
[Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()
[Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions
[Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()