Memory Region Naming - take 2

It's constant, and sourced from existing const strings. Avoid dodgy casts by converting to const.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 xen-hvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

xen-hvm.c +1 -1

index 91de2e2..d763e86 100644
@@ -71,7 +71,7 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 typedef struct XenPhysmap {
     hwaddr start_addr;
     ram_addr_t size;
-    char *name;
+    const char *name;
     hwaddr phys_offset;
 
     QLIST_ENTRY(XenPhysmap) list;

To support name retrieval of MemoryRegions that were created dynamically (that is, not via memory_region_init and friends). We cache the name in MemoryRegion's state as object_get_canonical_path_component mallocs the returned value so it's not suitable for direct return to callers. Memory already frees the name field, so this will be garbage collected along with the MR object.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 memory.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

memory.c +0 -1

index 42317a2..fc16e5f 100644
@@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
     }
-    mr->name = g_strdup(name);
 
     if (name) {
         object_property_add_child_array(owner, name, OBJECT(mr));

To support name retrieval of MemoryRegions that were created dynamically (that is, not via memory_region_init and friends). We cache the name in MemoryRegion's state as object_get_canonical_path_component mallocs the returned value so it's not suitable for direct return to callers. Memory already frees the name field, so this will be garbage collected along with the MR object.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 memory.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

memory.c +0 -0

index 42317a2..fc16e5f 100644

memory.c +4 -0

     if (name) {
         object_property_add_child_array(owner, name, OBJECT(mr));

@@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
 
 const char *memory_region_name(const MemoryRegion *mr)
 {
+    if (!mr->name) {
+        ((MemoryRegion *)mr)->name =
+            object_get_canonical_path_component(OBJECT(mr));
+    }
     return mr->name;
 }
 

Il 26/08/2014 05:10, Peter Crosthwaite ha scritto:

Quoted 14 lines

To support name retrieval of MemoryRegions that were created dynamically (that is, not via memory_region_init and friends). We cache the name in MemoryRegion's state as object_get_canonical_path_component mallocs the returned value so it's not suitable for direct return to callers. Memory already frees the name field, so this will be garbage collected along with the MR object.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 memory.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

memory.c void memory_region_init(MemoryRegion *mr, · line 917

Quoted 9 lines

memory.c +0 -1

index 42317a2..fc16e5f 100644
@@ -914,7 +914,6 @@ void memory_region_init(MemoryRegion *mr,
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
     }
-    mr->name = g_strdup(name);

This causes a bunch of [0] to appear in the "info mtree" output. They're a bit ugly, so I'm removing this line for now.

Paolo

Quoted 14 lines

memory.c +4 -0

     if (name) {
         object_property_add_child_array(owner, name, OBJECT(mr));

@@ -1309,6 +1308,10 @@ uint64_t memory_region_size(MemoryRegion *mr)
 
 const char *memory_region_name(const MemoryRegion *mr)
 {
+    if (!mr->name) {
+        ((MemoryRegion *)mr)->name =
+            object_get_canonical_path_component(OBJECT(mr));
+    }
     return mr->name;
 }
 

Yeh, so a solution to that was to patch qom to drop the [*] for singletons added via arrayified property adders. One possible semantic that I can think of is the 0th element always has a property alias so that foo and foo[0] mean the same thing. This tricky part is, when "[1]" comes along the preferred name (the canonical path) for the 0th element should switch from "foo" to "foo[0]". Is it sane to switch child and alias strings for "[0]" when "[1]" comes along?

Il 28/08/2014 16:27, Peter Crosthwaite ha scritto:

Quoted 7 lines

Yeh, so a solution to that was to patch qom to drop the [*] for singletons added via arrayified property adders. One possible semantic that I can think of is the 0th element always has a property alias so that foo and foo[0] mean the same thing. This tricky part is, when "[1]" comes along the preferred name (the canonical path) for the 0th element should switch from "foo" to "foo[0]". Is it sane to switch child and alias strings for "[0]" when "[1]" comes along?

You and I both know that the right solution is to move the [*] from memory.c to the callers... :)

Paolo

Hi,

Here is an attempt at a proper fix to the memory region naming bugs as reported by Peter. The Xen compile bug and the memory leak.

Regards,
Peter

Changed since v1:
Constify xen physmap name string (Stefan W review). Change memory_region_name to caching based approach.

Peter Crosthwaite (3):
xen-hvm: Constify string
xen: hvm: Abstract away memory region name ref
memory: Lazy init name from QOM name as needed

 memory.c  |  5 ++++-
 xen-hvm.c | 11 +++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

Signature · 1 line

2.1.0.1.g27b9230