[PATCH for-5.2] spapr: Simplify error handling in spapr_phb_realize()

[PATCH for-5.2] spapr: Simplify error handling in spapr_phb_realize()

[Greg Kurz]

The spapr_phb_realize() function has a local_err variable which
is used to:

1) check failures of spapr_irq_findone() and spapr_irq_claim()

2) prepend extra information to the error message

Recent work from Markus Armbruster highlighted we get better
code when testing the return value of a function, rather than
setting up all the local_err boiler plate. For similar reasons,
it is now preferred to use ERRP_GUARD() and error_prepend()
rather than error_propagate_prepend().

Since spapr_irq_findone() and spapr_irq_claim() return negative
values in case of failure, do both changes.

This is just cleanup, no functional impact.

Signed-off-by: Greg Kurz <groug@kaod.org>
---

Since we add ERRP_GUARD(), we could theoretically check *errp
rather than the return value, and thus avoid the uint32_t to
int32_t change but I personally find it clearer the other way.
---
 hw/ppc/spapr_pci.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 21681215d405..b1ce51327db4 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
      * tries to add a sPAPR PHB to a non-pseries machine.
      */
@@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     uint64_t msi_window_size = 4096;
     SpaprTceTable *tcet;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
-    Error *local_err = NULL;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
@@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
 
     /* Initialize the LSI table */
     for (i = 0; i < PCI_NUM_PINS; i++) {
-        uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
+        int32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
 
         if (smc->legacy_irq_allocation) {
-            irq = spapr_irq_findone(spapr, &local_err);
-            if (local_err) {
-                error_propagate_prepend(errp, local_err,
-                                        "can't allocate LSIs: ");
+            irq = spapr_irq_findone(spapr, errp);
+            if (irq < 0) {
+                error_prepend(errp, "can't allocate LSIs: ");
                 /*
                  * Older machines will never support PHB hotplug, ie, this is an
                  * init only path and QEMU will terminate. No need to rollback.
@@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
             }
         }
 
-        spapr_irq_claim(spapr, irq, true, &local_err);
-        if (local_err) {
-            error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
+        if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
+            error_prepend(errp, "can't allocate LSIs: ");
             goto unrealize;
         }
 

Re: [PATCH for-5.2] spapr: Simplify error handling in spapr_phb_realize()

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> The spapr_phb_realize() function has a local_err variable which
> is used to:
>
> 1) check failures of spapr_irq_findone() and spapr_irq_claim()
>
> 2) prepend extra information to the error message
>
> Recent work from Markus Armbruster highlighted we get better
> code when testing the return value of a function, rather than
> setting up all the local_err boiler plate. For similar reasons,
> it is now preferred to use ERRP_GUARD() and error_prepend()
> rather than error_propagate_prepend().
>
> Since spapr_irq_findone() and spapr_irq_claim() return negative
> values in case of failure, do both changes.
>
> This is just cleanup, no functional impact.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>
> Since we add ERRP_GUARD(), we could theoretically check *errp
> rather than the return value, and thus avoid the uint32_t to
> int32_t change but I personally find it clearer the other way.
> ---
>  hw/ppc/spapr_pci.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 21681215d405..b1ce51327db4 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
>  
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
>       * tries to add a sPAPR PHB to a non-pseries machine.
>       */
> @@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      uint64_t msi_window_size = 4096;
>      SpaprTceTable *tcet;
>      const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> -    Error *local_err = NULL;
>  
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> @@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  
>      /* Initialize the LSI table */
>      for (i = 0; i < PCI_NUM_PINS; i++) {
> -        uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
> +        int32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;

(1)

>  
>          if (smc->legacy_irq_allocation) {
> -            irq = spapr_irq_findone(spapr, &local_err);
> -            if (local_err) {
> -                error_propagate_prepend(errp, local_err,
> -                                        "can't allocate LSIs: ");
> +            irq = spapr_irq_findone(spapr, errp);

(2)

> +            if (irq < 0) {
> +                error_prepend(errp, "can't allocate LSIs: ");
>                  /*
>                   * Older machines will never support PHB hotplug, ie, this is an
>                   * init only path and QEMU will terminate. No need to rollback.
> @@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              }
>          }
>  
> -        spapr_irq_claim(spapr, irq, true, &local_err);
> -        if (local_err) {
> -            error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
> +        if (spapr_irq_claim(spapr, irq, true, errp) < 0) {

(3)

> +            error_prepend(errp, "can't allocate LSIs: ");
>              goto unrealize;
>          }

           sphb->lsi_table[i].irq = irq;

(4)

       }

The error propagation elimination looks good to me, but I wonder whether
int32_t is the best choice for @irq.

Before the patch:

(1) The initialization converts unsigned (I think) to uint32_t.

(2) Converts from int (value of spapr_irq_findone()) to uint32_t.

(3) spapr_irq_claim() takes int, we convert back to int.

(4) The assignment does not convert.

After the patch:

(1) The initialization converts unsigned (I think) to int32_t.

(2) Converts from int (value of spapr_irq_findone()) to int32_t.

(3) spapr_irq_claim() takes int, we convert back to int.

(4) Converts from int32_t to uint32_t

I assume the conversions are all safe before and after the patch
(spapr_irq_claim() asserts @irq is between 0x1000 and 0x1000 + small
change).  Still, too many conversions for my taste.  What about making
irq plain int?  Then:

(1) The initialization converts unsigned (I think) to int.

(2) Does not convert.

(3) Does not convert.

(4) Converts from int to uint32_t.

Feels neater to me.

Regardless:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-5.2] spapr: Simplify error handling in spapr_phb_realize()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5154 bytes --]

On Mon, Jul 20, 2020 at 11:29:06AM +0200, Markus Armbruster wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
> > The spapr_phb_realize() function has a local_err variable which
> > is used to:
> >
> > 1) check failures of spapr_irq_findone() and spapr_irq_claim()
> >
> > 2) prepend extra information to the error message
> >
> > Recent work from Markus Armbruster highlighted we get better
> > code when testing the return value of a function, rather than
> > setting up all the local_err boiler plate. For similar reasons,
> > it is now preferred to use ERRP_GUARD() and error_prepend()
> > rather than error_propagate_prepend().
> >
> > Since spapr_irq_findone() and spapr_irq_claim() return negative
> > values in case of failure, do both changes.
> >
> > This is just cleanup, no functional impact.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >
> > Since we add ERRP_GUARD(), we could theoretically check *errp
> > rather than the return value, and thus avoid the uint32_t to
> > int32_t change but I personally find it clearer the other way.
> > ---
> >  hw/ppc/spapr_pci.c |   16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 21681215d405..b1ce51327db4 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
> >  
> >  static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> >       * tries to add a sPAPR PHB to a non-pseries machine.
> >       */
> > @@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >      uint64_t msi_window_size = 4096;
> >      SpaprTceTable *tcet;
> >      const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> > -    Error *local_err = NULL;
> >  
> >      if (!spapr) {
> >          error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> > @@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >  
> >      /* Initialize the LSI table */
> >      for (i = 0; i < PCI_NUM_PINS; i++) {
> > -        uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
> > +        int32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
> 
> (1)
> 
> >  
> >          if (smc->legacy_irq_allocation) {
> > -            irq = spapr_irq_findone(spapr, &local_err);
> > -            if (local_err) {
> > -                error_propagate_prepend(errp, local_err,
> > -                                        "can't allocate LSIs: ");
> > +            irq = spapr_irq_findone(spapr, errp);
> 
> (2)
> 
> > +            if (irq < 0) {
> > +                error_prepend(errp, "can't allocate LSIs: ");
> >                  /*
> >                   * Older machines will never support PHB hotplug, ie, this is an
> >                   * init only path and QEMU will terminate. No need to rollback.
> > @@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >              }
> >          }
> >  
> > -        spapr_irq_claim(spapr, irq, true, &local_err);
> > -        if (local_err) {
> > -            error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
> > +        if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
> 
> (3)
> 
> > +            error_prepend(errp, "can't allocate LSIs: ");
> >              goto unrealize;
> >          }
> 
>            sphb->lsi_table[i].irq = irq;
> 
> (4)
> 
>        }
> 
> The error propagation elimination looks good to me, but I wonder whether
> int32_t is the best choice for @irq.
> 
> Before the patch:
> 
> (1) The initialization converts unsigned (I think) to uint32_t.
> 
> (2) Converts from int (value of spapr_irq_findone()) to uint32_t.
> 
> (3) spapr_irq_claim() takes int, we convert back to int.
> 
> (4) The assignment does not convert.
> 
> After the patch:
> 
> (1) The initialization converts unsigned (I think) to int32_t.
> 
> (2) Converts from int (value of spapr_irq_findone()) to int32_t.
> 
> (3) spapr_irq_claim() takes int, we convert back to int.
> 
> (4) Converts from int32_t to uint32_t
> 
> I assume the conversions are all safe before and after the patch
> (spapr_irq_claim() asserts @irq is between 0x1000 and 0x1000 + small
> change).  Still, too many conversions for my taste.  What about making
> irq plain int?  Then:
> 
> (1) The initialization converts unsigned (I think) to int.
> 
> (2) Does not convert.
> 
> (3) Does not convert.
> 
> (4) Converts from int to uint32_t.
> 
> Feels neater to me.

Sounds like a good idea.  Greg, care to post a followup patch?
> 
> Regardless:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]