qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 5/5] spapr: Simplify error handling in spapr_memory_plug()
Date: Mon, 19 Oct 2020 11:10:01 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.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







reply via email to

[Prev in Thread] Current Thread [Next in Thread]