Message ID | 156077921212.433243.11716701611944816815.stgit@bahia.lan (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xics/kvm: Improve error handling | expand |
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"); >
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"); >
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");
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(-)