[Qemu-devel] [PATCH v0 0/2] Fix pc DIMMs capacity calculation

[Qemu-devel] [PATCH v0 0/2] Fix pc DIMMs capacity calculation

[Bharata B Rao]

1st patch fixes PC DIMMs capacity calculation and the 2nd patch does
code movement so that pc_existing_dimms_capacity() can be used by
other architectures.

Bharata B Rao (2):
  pc: Fix DIMMs capacity calculation
  pc-dimm: Make pc_existing_dimms_capacity global

 hw/i386/pc.c             | 36 +++---------------------------------
 hw/mem/pc-dimm.c         | 25 +++++++++++++++++++++++++
 include/hw/mem/pc-dimm.h |  1 +
 3 files changed, 29 insertions(+), 33 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v0 1/2] pc: Fix DIMMs capacity calculation

[Bharata B Rao]

pc_existing_dimms_capacity() is returning DIMMs count rather than capacity.
Fix this to return the capacity. Also consider only realized devices for
capacity calculation.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/pc.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e07f1fa..125cf0a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1552,25 +1552,18 @@ void qemu_register_pc_machine(QEMUMachine *m)
     g_free(name);
 }
 
-static int pc_dimm_count(Object *obj, void *opaque)
-{
-    int *count = opaque;
-
-    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        (*count)++;
-    }
-
-    object_child_foreach(obj, pc_dimm_count, opaque);
-    return 0;
-}
-
 static int pc_existing_dimms_capacity(Object *obj, void *opaque)
 {
     Error *local_err = NULL;
     uint64_t *size = opaque;
 
     if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        (*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP, &local_err);
+        DeviceState *dev = DEVICE(obj);
+
+        if (dev->realized) {
+            (*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
+                &local_err);
+        }
 
         if (local_err) {
             qerror_report_err(local_err);
@@ -1579,7 +1572,7 @@ static int pc_existing_dimms_capacity(Object *obj, void *opaque)
         }
     }
 
-    object_child_foreach(obj, pc_dimm_count, opaque);
+    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
     return 0;
 }
 
@@ -1623,8 +1616,9 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     if (existing_dimms_capacity + memory_region_size(mr) >
         machine->maxram_size - machine->ram_size) {
         error_setg(&local_err, "not enough space, currently 0x%" PRIx64
-                   " in use of total 0x" RAM_ADDR_FMT,
-                   existing_dimms_capacity, machine->maxram_size);
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   existing_dimms_capacity,
+                   machine->maxram_size - machine->ram_size);
         goto out;
     }
 
-- 
2.1.0

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

[Bharata B Rao]

Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
by PowerPC memory hotplug code too.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 125cf0a..2ec45a4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
     g_free(name);
 }
 
-static int pc_existing_dimms_capacity(Object *obj, void *opaque)
-{
-    Error *local_err = NULL;
-    uint64_t *size = opaque;
-
-    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);
-        }
-
-        if (local_err) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-            return 1;
-        }
-    }
-
-    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
-    return 0;
-}
-
 static void pc_dimm_plug(HotplugHandler *hotplug_dev,
                          DeviceState *dev, Error **errp)
 {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index d431834..f02ce6e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -22,6 +22,31 @@
 #include "qemu/config-file.h"
 #include "qapi/visitor.h"
 #include "qemu/range.h"
+#include "qapi/qmp/qerror.h"
+
+int pc_existing_dimms_capacity(Object *obj, void *opaque)
+{
+    Error *local_err = NULL;
+    uint64_t *size = opaque;
+
+    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);
+        }
+
+        if (local_err) {
+            qerror_report_err(local_err);
+            error_free(local_err);
+            return 1;
+        }
+    }
+
+    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
+    return 0;
+}
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index e1dcbbc..bbfa53f 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,4 +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);
 #endif
-- 
2.1.0

Re: [Qemu-devel] [PATCH v0 0/2] Fix pc DIMMs capacity calculation

[Bharata B Rao]

On Mon, Jan 12, 2015 at 9:32 AM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> 1st patch fixes PC DIMMs capacity calculation and the 2nd patch does
> code movement so that pc_existing_dimms_capacity() can be used by
> other architectures.
>
> Bharata B Rao (2):
>   pc: Fix DIMMs capacity calculation
>   pc-dimm: Make pc_existing_dimms_capacity global

Any feedback on this patchset ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v0 1/2] pc: Fix DIMMs capacity calculation

[Igor Mammedov]

On Mon, 12 Jan 2015 09:32:33 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> pc_existing_dimms_capacity() is returning DIMMs count rather than capacity.
> Fix this to return the capacity. Also consider only realized devices for
> capacity calculation.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e07f1fa..125cf0a 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1552,25 +1552,18 @@ void qemu_register_pc_machine(QEMUMachine *m)
>      g_free(name);
>  }
>  
> -static int pc_dimm_count(Object *obj, void *opaque)
> -{
> -    int *count = opaque;
> -
> -    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> -        (*count)++;
> -    }
> -
> -    object_child_foreach(obj, pc_dimm_count, opaque);
> -    return 0;
> -}
> -
>  static int pc_existing_dimms_capacity(Object *obj, void *opaque)
>  {
>      Error *local_err = NULL;
>      uint64_t *size = opaque;
>  
>      if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> -        (*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP, &local_err);
> +        DeviceState *dev = DEVICE(obj);
> +
> +        if (dev->realized) {
> +            (*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> +                &local_err);
> +        }
>  
>          if (local_err) {
>              qerror_report_err(local_err);
> @@ -1579,7 +1572,7 @@ static int pc_existing_dimms_capacity(Object *obj, void *opaque)
>          }
>      }
>  
> -    object_child_foreach(obj, pc_dimm_count, opaque);
> +    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
>      return 0;
>  }
>  
> @@ -1623,8 +1616,9 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      if (existing_dimms_capacity + memory_region_size(mr) >
>          machine->maxram_size - machine->ram_size) {
>          error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> -                   " in use of total 0x" RAM_ADDR_FMT,
> -                   existing_dimms_capacity, machine->maxram_size);
> +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> +                   existing_dimms_capacity,
> +                   machine->maxram_size - machine->ram_size);
>          goto out;
>      }
>  

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

[Igor Mammedov]

On Mon, 12 Jan 2015 09:32:34 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
> by PowerPC memory hotplug code too.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/i386/pc.c             | 24 ------------------------
>  hw/mem/pc-dimm.c         | 25 +++++++++++++++++++++++++
>  include/hw/mem/pc-dimm.h |  1 +
>  3 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 125cf0a..2ec45a4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
>      g_free(name);
>  }
>  
> -static int pc_existing_dimms_capacity(Object *obj, void *opaque)
> -{
> -    Error *local_err = NULL;
> -    uint64_t *size = opaque;
> -
> -    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);
> -        }
> -
> -        if (local_err) {
> -            qerror_report_err(local_err);
> -            error_free(local_err);
> -            return 1;
> -        }
> -    }
> -
> -    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> -    return 0;
> -}
> -
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>                           DeviceState *dev, Error **errp)
>  {
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index d431834..f02ce6e 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -22,6 +22,31 @@
>  #include "qemu/config-file.h"
>  #include "qapi/visitor.h"
>  #include "qemu/range.h"
> +#include "qapi/qmp/qerror.h"
> +
> +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.


> +    Error *local_err = NULL;
> +    uint64_t *size = opaque;
> +
> +    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);
> +        }
> +
> +        if (local_err) {
> +            qerror_report_err(local_err);
> +            error_free(local_err);
> +            return 1;
> +        }
> +    }
> +
> +    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> +    return 0;
> +}
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index e1dcbbc..bbfa53f 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -78,4 +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);
>  #endif

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

[Bharata B Rao]

On Tue, Jan 20, 2015 at 11:18:37AM +0100, Igor Mammedov wrote:
> On Mon, 12 Jan 2015 09:32:34 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
> > by PowerPC memory hotplug code too.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/i386/pc.c             | 24 ------------------------
> >  hw/mem/pc-dimm.c         | 25 +++++++++++++++++++++++++
> >  include/hw/mem/pc-dimm.h |  1 +
> >  3 files changed, 26 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 125cf0a..2ec45a4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
> >      g_free(name);
> >  }
> >  
> > -static int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > -{
> > -    Error *local_err = NULL;
> > -    uint64_t *size = opaque;
> > -
> > -    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);
> > -        }
> > -
> > -        if (local_err) {
> > -            qerror_report_err(local_err);
> > -            error_free(local_err);
> > -            return 1;
> > -        }
> > -    }
> > -
> > -    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > -    return 0;
> > -}
> > -
> >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >                           DeviceState *dev, Error **errp)
> >  {
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index d431834..f02ce6e 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -22,6 +22,31 @@
> >  #include "qemu/config-file.h"
> >  #include "qapi/visitor.h"
> >  #include "qemu/range.h"
> > +#include "qapi/qmp/qerror.h"
> > +
> > +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.

> 
> > +    Error *local_err = NULL;
> > +    uint64_t *size = opaque;
> > +
> > +    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);
> > +        }
> > +
> > +        if (local_err) {
> > +            qerror_report_err(local_err);
> > +            error_free(local_err);
> > +            return 1;
> > +        }
> > +    }
> > +
> > +    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > +    return 0;
> > +}

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

[Igor Mammedov]

On Wed, 21 Jan 2015 14:28:18 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jan 20, 2015 at 11:18:37AM +0100, Igor Mammedov wrote:
> > On Mon, 12 Jan 2015 09:32:34 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
> > > by PowerPC memory hotplug code too.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/i386/pc.c             | 24 ------------------------
> > >  hw/mem/pc-dimm.c         | 25 +++++++++++++++++++++++++
> > >  include/hw/mem/pc-dimm.h |  1 +
> > >  3 files changed, 26 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 125cf0a..2ec45a4 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
> > >      g_free(name);
> > >  }
> > >  
> > > -static int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > > -{
> > > -    Error *local_err = NULL;
> > > -    uint64_t *size = opaque;
> > > -
> > > -    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);
> > > -        }
> > > -
> > > -        if (local_err) {
> > > -            qerror_report_err(local_err);
> > > -            error_free(local_err);
> > > -            return 1;
> > > -        }
> > > -    }
> > > -
> > > -    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > > -    return 0;
> > > -}
> > > -
> > >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> > >                           DeviceState *dev, Error **errp)
> > >  {
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index d431834..f02ce6e 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -22,6 +22,31 @@
> > >  #include "qemu/config-file.h"
> > >  #include "qapi/visitor.h"
> > >  #include "qemu/range.h"
> > > +#include "qapi/qmp/qerror.h"
> > > +
> > > +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 (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);
> > > +        }
> > > +
> > > +        if (local_err) {
> > > +            qerror_report_err(local_err);
> > > +            error_free(local_err);
> > > +            return 1;
> > > +        }
> > > +    }
> > > +
> > > +    object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > > +    return 0;
> > > +}
> 

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

[Bharata B Rao]

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

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

[Igor Mammedov]

On Thu, 22 Jan 2015 20:04:46 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

[...]


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

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