Re: [Qemu-devel] [PATCH v0 2/2] pc-dimm: Make pc_existing_dimms_capacity global

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Wed, Jan 21, 2015 at 11:38:52AM +0100, Igor Mammedov wrote:
> On Wed, 21 Jan 2015 14:28:18 +0530
> > > > +int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > > > +{
> > > since you are making it API, could you pass Error **errp argument
> > > and deal with error in caller?
> > > Along with it you can make function return void.
> > 
> > I don't think that can be done because pc_existing_dimms_capacity() calls
> > itself recursively via object_child_foreach() and hence its signature
> > if fixed.
> how about:
> 
> typedef struct somename {
>     uint64_t size;
>     Error **errp;
> } somename;
> 
> 
> somename *foo = opaque;
> 
> ...
> 
> and if you'd be able to hide this structure from API caller,
> then function could return it's existing capacity via its return value.
> For example:
> 
> static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
> {
> ...
> }
> 
> /* API */
> uint64_t pc_existing_dimms_capacity(Error **errp)
> {
>   ...
>   object_child_foreach(obj, pc_existing_dimms_capacity_internal, opaque);
>   ...
> }

If you find the below patch to be fine, I will repost the series
with this patch included.

pc-dimm: Add Error argument to pc_existing_dimms_capacity

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

Now that pc_existing_dimms_capacity() is an API, include Error pointer
as an argument and modify the caller appropriately.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/pc.c             |    4 ++--
 hw/mem/pc-dimm.c         |   31 ++++++++++++++++++++++---------
 include/hw/mem/pc-dimm.h |    2 +-
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ec45a4..c7af6aa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1584,8 +1584,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    if (pc_existing_dimms_capacity(OBJECT(machine), &existing_dimms_capacity)) {
-        error_setg(&local_err, "failed to get total size of existing DIMMs");
+    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
+    if (local_err) {
         goto out;
     }
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index f02ce6e..3e908b3 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -24,30 +24,43 @@
 #include "qemu/range.h"
 #include "qapi/qmp/qerror.h"
 
-int pc_existing_dimms_capacity(Object *obj, void *opaque)
+typedef struct pc_dimms_capacity {
+     uint64_t size;
+     Error    **errp;
+} pc_dimms_capacity;
+
+static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
 {
-    Error *local_err = NULL;
-    uint64_t *size = opaque;
+    pc_dimms_capacity *cap = opaque;
+    uint64_t *size = &cap->size;
 
     if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
         DeviceState *dev = DEVICE(obj);
 
         if (dev->realized) {
             (*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
-                &local_err);
+                cap->errp);
         }
 
-        if (local_err) {
-            qerror_report_err(local_err);
-            error_free(local_err);
+        if (cap->errp && *cap->errp) {
             return 1;
         }
     }
-
-    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
+    object_child_foreach(obj, pc_existing_dimms_capacity_internal, opaque);
     return 0;
 }
 
+uint64_t pc_existing_dimms_capacity(Error **errp)
+{
+    pc_dimms_capacity cap;
+
+    cap.size = 0;
+    cap.errp = errp;
+
+    pc_existing_dimms_capacity_internal(qdev_get_machine(), &cap);
+    return cap.size;
+}
+
 int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
     MemoryDeviceInfoList ***prev = opaque;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index bbfa53f..f7b80b4 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,5 +78,5 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
-int pc_existing_dimms_capacity(Object *obj, void *opaque);
+uint64_t pc_existing_dimms_capacity(Error **errp);
 #endif