diff mbox

[v3,1/5] pc-dimm: refactor qmp_pc_dimm_device_list

Message ID 20180305065710.25876-2-haozhong.zhang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Haozhong Zhang March 5, 2018, 6:57 a.m. UTC 
Use pc_dimm_built_list to hide recursive callbacks from callers.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/mem/pc-dimm.c         | 83 +++++++++++++++++++++++++-----------------------
 hw/ppc/spapr.c           |  3 +-
 include/hw/mem/pc-dimm.h |  2 +-
 numa.c                   |  4 +--
 qmp.c                    |  7 +---
 stubs/qmp_pc_dimm.c      |  4 +--
 6 files changed, 50 insertions(+), 53 deletions(-)

Comments

Igor Mammedov March 7, 2018, 10:10 a.m. UTC | #1 
On Mon,  5 Mar 2018 14:57:06 +0800
Haozhong Zhang <haozhong.zhang@intel.com> wrote:

I'd suggest following commit subj/msg:

-----
 pc-dimm: make qmp_pc_dimm_device_list() sort devices by address

 Make qmp_pc_dimm_device_list() return sorted by start address
 list of devices so that it could be reused in places that
 would need sorted list*. Reuse existing pc_dimm_built_list()
 to get sorted list.

 While at it hide recursive callbacks from callers, so that:

   qmp_pc_dimm_device_list(qdev_get_machine(), &list);

 could be replaced with simpler:

   list = qmp_pc_dimm_device_list();

 * follow up patch will use it in build_srat()
-----

with commit message updated:
  Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> Use pc_dimm_built_list to hide recursive callbacks from callers.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/mem/pc-dimm.c         | 83 +++++++++++++++++++++++++-----------------------
>  hw/ppc/spapr.c           |  3 +-
>  include/hw/mem/pc-dimm.h |  2 +-
>  numa.c                   |  4 +--
>  qmp.c                    |  7 +---
>  stubs/qmp_pc_dimm.c      |  4 +--
>  6 files changed, 50 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 6e74b61cb6..4d050fe2cd 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -162,45 +162,6 @@ uint64_t get_plugged_memory_size(void)
>      return pc_existing_dimms_capacity(&error_abort);
>  }
>  
> -int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> -{
> -    MemoryDeviceInfoList ***prev = opaque;
> -
> -    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> -        DeviceState *dev = DEVICE(obj);
> -
> -        if (dev->realized) {
> -            MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
> -            MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
> -            PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
> -            DeviceClass *dc = DEVICE_GET_CLASS(obj);
> -            PCDIMMDevice *dimm = PC_DIMM(obj);
> -
> -            if (dev->id) {
> -                di->has_id = true;
> -                di->id = g_strdup(dev->id);
> -            }
> -            di->hotplugged = dev->hotplugged;
> -            di->hotpluggable = dc->hotpluggable;
> -            di->addr = dimm->addr;
> -            di->slot = dimm->slot;
> -            di->node = dimm->node;
> -            di->size = object_property_get_uint(OBJECT(dimm), PC_DIMM_SIZE_PROP,
> -                                                NULL);
> -            di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
> -
> -            info->u.dimm.data = di;
> -            elem->value = info;
> -            elem->next = NULL;
> -            **prev = elem;
> -            *prev = &elem->next;
> -        }
> -    }
> -
> -    object_child_foreach(obj, qmp_pc_dimm_device_list, opaque);
> -    return 0;
> -}
> -
>  static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
>  {
>      unsigned long *bitmap = opaque;
> @@ -276,6 +237,50 @@ static int pc_dimm_built_list(Object *obj, void *opaque)
>      return 0;
>  }
>  
> +MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
> +{
> +    GSList *dimms = NULL, *item;
> +    MemoryDeviceInfoList *list = NULL, *prev = NULL;
> +
> +    object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &dimms);
> +
> +    for (item = dimms; item; item = g_slist_next(item)) {
> +        PCDIMMDevice *dimm = PC_DIMM(item->data);
> +        Object *obj = OBJECT(dimm);
> +        MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
> +        MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
> +        PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
> +        DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +        DeviceState *dev = DEVICE(obj);
> +
> +        if (dev->id) {
> +            di->has_id = true;
> +            di->id = g_strdup(dev->id);
> +        }
> +        di->hotplugged = dev->hotplugged;
> +        di->hotpluggable = dc->hotpluggable;
> +        di->addr = dimm->addr;
> +        di->slot = dimm->slot;
> +        di->node = dimm->node;
> +        di->size = object_property_get_uint(obj, PC_DIMM_SIZE_PROP, NULL);
> +        di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
> +
> +        info->u.dimm.data = di;
> +        elem->value = info;
> +        elem->next = NULL;
> +        if (prev) {
> +            prev->next = elem;
> +        } else {
> +            list = elem;
> +        }
> +        prev = elem;
> +    }
> +
> +    g_slist_free(dimms);
> +
> +    return list;
> +}
> +
>  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>                                 uint64_t address_space_size,
>                                 uint64_t *hint, uint64_t align, uint64_t size,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 83c9d66dd5..68a81e47d2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -731,8 +731,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
>      }
>  
>      if (hotplug_lmb_start) {
> -        MemoryDeviceInfoList **prev = &dimms;
> -        qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
> +        dimms = qmp_pc_dimm_device_list();
>      }
>  
>      /* ibm,dynamic-memory */
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index d83b957829..1fc479281c 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -93,7 +93,7 @@ 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);
> +MemoryDeviceInfoList *qmp_pc_dimm_device_list(void);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
>  uint64_t get_plugged_memory_size(void);
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/numa.c b/numa.c
> index 7e0e789b02..c6734ceb8c 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -520,12 +520,10 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>  
>  static void numa_stat_memory_devices(NumaNodeMem node_mem[])
>  {
> -    MemoryDeviceInfoList *info_list = NULL;
> -    MemoryDeviceInfoList **prev = &info_list;
> +    MemoryDeviceInfoList *info_list = qmp_pc_dimm_device_list();
>      MemoryDeviceInfoList *info;
>      PCDIMMDeviceInfo     *pcdimm_info;
>  
> -    qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
>      for (info = info_list; info; info = info->next) {
>          MemoryDeviceInfo *value = info->value;
>  
> diff --git a/qmp.c b/qmp.c
> index 793f6f3323..c6ac7b05b4 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -680,12 +680,7 @@ void qmp_object_del(const char *id, Error **errp)
>  
>  MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
>  {
> -    MemoryDeviceInfoList *head = NULL;
> -    MemoryDeviceInfoList **prev = &head;
> -
> -    qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
> -
> -    return head;
> +    return qmp_pc_dimm_device_list();
>  }
>  
>  ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
> diff --git a/stubs/qmp_pc_dimm.c b/stubs/qmp_pc_dimm.c
> index 9ddc4f619a..b6b2cca89e 100644
> --- a/stubs/qmp_pc_dimm.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -2,9 +2,9 @@
>  #include "qom/object.h"
>  #include "hw/mem/pc-dimm.h"
>  
> -int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> +MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
>  {
> -   return 0;
> +   return NULL;
>  }
>  
>  uint64_t get_plugged_memory_size(void)
diff mbox

Patch

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 6e74b61cb6..4d050fe2cd 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -162,45 +162,6 @@  uint64_t get_plugged_memory_size(void)
     return pc_existing_dimms_capacity(&error_abort);
 }
 
-int qmp_pc_dimm_device_list(Object *obj, void *opaque)
-{
-    MemoryDeviceInfoList ***prev = opaque;
-
-    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        DeviceState *dev = DEVICE(obj);
-
-        if (dev->realized) {
-            MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
-            MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
-            PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
-            DeviceClass *dc = DEVICE_GET_CLASS(obj);
-            PCDIMMDevice *dimm = PC_DIMM(obj);
-
-            if (dev->id) {
-                di->has_id = true;
-                di->id = g_strdup(dev->id);
-            }
-            di->hotplugged = dev->hotplugged;
-            di->hotpluggable = dc->hotpluggable;
-            di->addr = dimm->addr;
-            di->slot = dimm->slot;
-            di->node = dimm->node;
-            di->size = object_property_get_uint(OBJECT(dimm), PC_DIMM_SIZE_PROP,
-                                                NULL);
-            di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
-
-            info->u.dimm.data = di;
-            elem->value = info;
-            elem->next = NULL;
-            **prev = elem;
-            *prev = &elem->next;
-        }
-    }
-
-    object_child_foreach(obj, qmp_pc_dimm_device_list, opaque);
-    return 0;
-}
-
 static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
 {
     unsigned long *bitmap = opaque;
@@ -276,6 +237,50 @@  static int pc_dimm_built_list(Object *obj, void *opaque)
     return 0;
 }
 
+MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
+{
+    GSList *dimms = NULL, *item;
+    MemoryDeviceInfoList *list = NULL, *prev = NULL;
+
+    object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &dimms);
+
+    for (item = dimms; item; item = g_slist_next(item)) {
+        PCDIMMDevice *dimm = PC_DIMM(item->data);
+        Object *obj = OBJECT(dimm);
+        MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
+        MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
+        PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
+        DeviceClass *dc = DEVICE_GET_CLASS(obj);
+        DeviceState *dev = DEVICE(obj);
+
+        if (dev->id) {
+            di->has_id = true;
+            di->id = g_strdup(dev->id);
+        }
+        di->hotplugged = dev->hotplugged;
+        di->hotpluggable = dc->hotpluggable;
+        di->addr = dimm->addr;
+        di->slot = dimm->slot;
+        di->node = dimm->node;
+        di->size = object_property_get_uint(obj, PC_DIMM_SIZE_PROP, NULL);
+        di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
+
+        info->u.dimm.data = di;
+        elem->value = info;
+        elem->next = NULL;
+        if (prev) {
+            prev->next = elem;
+        } else {
+            list = elem;
+        }
+        prev = elem;
+    }
+
+    g_slist_free(dimms);
+
+    return list;
+}
+
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
                                uint64_t *hint, uint64_t align, uint64_t size,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 83c9d66dd5..68a81e47d2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -731,8 +731,7 @@  static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
     }
 
     if (hotplug_lmb_start) {
-        MemoryDeviceInfoList **prev = &dimms;
-        qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
+        dimms = qmp_pc_dimm_device_list();
     }
 
     /* ibm,dynamic-memory */
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index d83b957829..1fc479281c 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -93,7 +93,7 @@  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);
+MemoryDeviceInfoList *qmp_pc_dimm_device_list(void);
 uint64_t pc_existing_dimms_capacity(Error **errp);
 uint64_t get_plugged_memory_size(void);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
diff --git a/numa.c b/numa.c
index 7e0e789b02..c6734ceb8c 100644
--- a/numa.c
+++ b/numa.c
@@ -520,12 +520,10 @@  void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
 
 static void numa_stat_memory_devices(NumaNodeMem node_mem[])
 {
-    MemoryDeviceInfoList *info_list = NULL;
-    MemoryDeviceInfoList **prev = &info_list;
+    MemoryDeviceInfoList *info_list = qmp_pc_dimm_device_list();
     MemoryDeviceInfoList *info;
     PCDIMMDeviceInfo     *pcdimm_info;
 
-    qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
     for (info = info_list; info; info = info->next) {
         MemoryDeviceInfo *value = info->value;
 
diff --git a/qmp.c b/qmp.c
index 793f6f3323..c6ac7b05b4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -680,12 +680,7 @@  void qmp_object_del(const char *id, Error **errp)
 
 MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
 {
-    MemoryDeviceInfoList *head = NULL;
-    MemoryDeviceInfoList **prev = &head;
-
-    qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
-
-    return head;
+    return qmp_pc_dimm_device_list();
 }
 
 ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
diff --git a/stubs/qmp_pc_dimm.c b/stubs/qmp_pc_dimm.c
index 9ddc4f619a..b6b2cca89e 100644
--- a/stubs/qmp_pc_dimm.c
+++ b/stubs/qmp_pc_dimm.c
@@ -2,9 +2,9 @@ 
 #include "qom/object.h"
 #include "hw/mem/pc-dimm.h"
 
-int qmp_pc_dimm_device_list(Object *obj, void *opaque)
+MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
 {
-   return 0;
+   return NULL;
 }
 
 uint64_t get_plugged_memory_size(void)