diff mbox series

[5/5] spapr: Simplify error handling in spapr_memory_plug()

Message ID 160309734178.2739814.3488437759887793902.stgit@bahia.lan (mailing list archive)
State New, archived
Headers show
Series spapr: Error handling fixes and cleanups (round 3) | expand

Commit Message

Greg Kurz Oct. 19, 2020, 8:49 a.m. UTC 
As recommended in "qapi/error.h", add a bool return value to
spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
of local_err in spapr_memory_plug().

This allows to get rid of the error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c                |   23 ++++++++++-------------
 hw/ppc/spapr_nvdimm.c         |    5 +++--
 include/hw/ppc/spapr_nvdimm.h |    2 +-
 3 files changed, 14 insertions(+), 16 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 19, 2020, 9:10 a.m. UTC | #1 
On 10/19/20 10:49 AM, Greg Kurz wrote:
> As recommended in "qapi/error.h", add a bool return value to
> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> of local_err in spapr_memory_plug().
> 
> This allows to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/spapr.c                |   23 ++++++++++-------------
>   hw/ppc/spapr_nvdimm.c         |    5 +++--
>   include/hw/ppc/spapr_nvdimm.h |    2 +-
>   3 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 62f217a6b914..0cc19b5863a4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>       return 0;
>   }
>   
> -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                              bool dedicated_hp_event_source, Error **errp)
>   {
>       SpaprDrc *drc;
> @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                         addr / SPAPR_MEMORY_BLOCK_SIZE);
>                   spapr_drc_detach(drc);
>               }
> -            return;
> +            return false;
>           }
>           if (!hotplugged) {
>               spapr_drc_reset(drc);
> @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                              nr_lmbs);
>           }
>       }
> +    return true;
>   }
>   
>   static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>   {
> -    Error *local_err = NULL;
>       SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>       PCDIMMDevice *dimm = PC_DIMM(dev);
>       uint64_t size, addr;
> @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       if (!is_nvdimm) {
>           addr = object_property_get_uint(OBJECT(dimm),
>                                           PC_DIMM_ADDR_PROP, &error_abort);
> -        spapr_add_lmbs(dev, addr, size,
> -                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> -                       &local_err);
> +        if (!spapr_add_lmbs(dev, addr, size,
> +                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> +            goto out_unplug;
> +        }
>       } else {
>           slot = object_property_get_int(OBJECT(dimm),
>                                          PC_DIMM_SLOT_PROP, &error_abort);
>           /* We should have valid slot number at this point */
>           g_assert(slot >= 0);
> -        spapr_add_nvdimm(dev, slot, &local_err);
> -    }
> -
> -    if (local_err) {
> -        goto out_unplug;
> +        if (!spapr_add_nvdimm(dev, slot, errp)) {
> +            goto out_unplug;
> +        }
>       }
>   
>       return;
>   
>   out_unplug:
>       pc_dimm_unplug(dimm, MACHINE(ms));
> -out:
> -    error_propagate(errp, local_err);
>   }
>   
>   static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 9e3d94071fe1..a833a63b5ed3 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>   }
>   
>   
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>   {
>       SpaprDrc *drc;
>       bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>       g_assert(drc);
>   
>       if (!spapr_drc_attach(drc, dev, errp)) {
> -        return;
> +        return false;
>       }
>   
>       if (hotplugged) {
>           spapr_hotplug_req_add_by_index(drc);
>       }
> +    return true;
>   }
>   
>   static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 490b19a009f4..344582d2f5f7 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>   void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>   bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>                              uint64_t size, Error **errp);
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);

Maybe document the returned value?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>   #endif
> 
> 
>
Igor Mammedov Oct. 23, 2020, 7:17 p.m. UTC | #2 
On Mon, 19 Oct 2020 10:49:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> As recommended in "qapi/error.h", add a bool return value to
> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> of local_err in spapr_memory_plug().
> 
> This allows to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
you won't need this patch if check from spapr_drc_attach() were
moved to _pre_plug() time.

> ---
>  hw/ppc/spapr.c                |   23 ++++++++++-------------
>  hw/ppc/spapr_nvdimm.c         |    5 +++--
>  include/hw/ppc/spapr_nvdimm.h |    2 +-
>  3 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 62f217a6b914..0cc19b5863a4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      return 0;
>  }
>  
> -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                             bool dedicated_hp_event_source, Error **errp)
>  {
>      SpaprDrc *drc;
> @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                        addr / SPAPR_MEMORY_BLOCK_SIZE);
>                  spapr_drc_detach(drc);
>              }
> -            return;
> +            return false;
>          }
>          if (!hotplugged) {
>              spapr_drc_reset(drc);
> @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                             nr_lmbs);
>          }
>      }
> +    return true;
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp)
>  {
> -    Error *local_err = NULL;
>      SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      uint64_t size, addr;
> @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
>                                          PC_DIMM_ADDR_PROP, &error_abort);
> -        spapr_add_lmbs(dev, addr, size,
> -                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> -                       &local_err);
> +        if (!spapr_add_lmbs(dev, addr, size,
> +                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> +            goto out_unplug;
> +        }
>      } else {
>          slot = object_property_get_int(OBJECT(dimm),
>                                         PC_DIMM_SLOT_PROP, &error_abort);
>          /* We should have valid slot number at this point */
>          g_assert(slot >= 0);
> -        spapr_add_nvdimm(dev, slot, &local_err);
> -    }
> -
> -    if (local_err) {
> -        goto out_unplug;
> +        if (!spapr_add_nvdimm(dev, slot, errp)) {
> +            goto out_unplug;
> +        }
>      }
>  
>      return;
>  
>  out_unplug:
>      pc_dimm_unplug(dimm, MACHINE(ms));
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 9e3d94071fe1..a833a63b5ed3 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>  }
>  
>  
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>  {
>      SpaprDrc *drc;
>      bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>      g_assert(drc);
>  
>      if (!spapr_drc_attach(drc, dev, errp)) {
> -        return;
> +        return false;
>      }
>  
>      if (hotplugged) {
>          spapr_hotplug_req_add_by_index(drc);
>      }
> +    return true;
>  }
>  
>  static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 490b19a009f4..344582d2f5f7 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>  void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>                             uint64_t size, Error **errp);
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
>  
>  #endif
> 
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 62f217a6b914..0cc19b5863a4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3382,7 +3382,7 @@  int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
-static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
+static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                            bool dedicated_hp_event_source, Error **errp)
 {
     SpaprDrc *drc;
@@ -3403,7 +3403,7 @@  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                       addr / SPAPR_MEMORY_BLOCK_SIZE);
                 spapr_drc_detach(drc);
             }
-            return;
+            return false;
         }
         if (!hotplugged) {
             spapr_drc_reset(drc);
@@ -3425,12 +3425,12 @@  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                            nr_lmbs);
         }
     }
+    return true;
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
-    Error *local_err = NULL;
     SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint64_t size, addr;
@@ -3444,27 +3444,24 @@  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     if (!is_nvdimm) {
         addr = object_property_get_uint(OBJECT(dimm),
                                         PC_DIMM_ADDR_PROP, &error_abort);
-        spapr_add_lmbs(dev, addr, size,
-                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
-                       &local_err);
+        if (!spapr_add_lmbs(dev, addr, size,
+                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
+            goto out_unplug;
+        }
     } else {
         slot = object_property_get_int(OBJECT(dimm),
                                        PC_DIMM_SLOT_PROP, &error_abort);
         /* We should have valid slot number at this point */
         g_assert(slot >= 0);
-        spapr_add_nvdimm(dev, slot, &local_err);
-    }
-
-    if (local_err) {
-        goto out_unplug;
+        if (!spapr_add_nvdimm(dev, slot, errp)) {
+            goto out_unplug;
+        }
     }
 
     return;
 
 out_unplug:
     pc_dimm_unplug(dimm, MACHINE(ms));
-out:
-    error_propagate(errp, local_err);
 }
 
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 9e3d94071fe1..a833a63b5ed3 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -89,7 +89,7 @@  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 }
 
 
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
 {
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
@@ -98,12 +98,13 @@  void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
     g_assert(drc);
 
     if (!spapr_drc_attach(drc, dev, errp)) {
-        return;
+        return false;
     }
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
     }
+    return true;
 }
 
 static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index 490b19a009f4..344582d2f5f7 100644
--- a/include/hw/ppc/spapr_nvdimm.h
+++ b/include/hw/ppc/spapr_nvdimm.h
@@ -30,6 +30,6 @@  int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
 void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
 bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
                            uint64_t size, Error **errp);
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
 
 #endif