[Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs

[Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs

[Peter Crosthwaite]

Hi Peter,

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

Regards,
Peter


Peter Crosthwaite (3):
  xen: hvm: Abstract away memory region name ref
  qom: return const for object_get_canon_path component
  qom: object.h: Update object_get_canon_path* doc

 backends/hostmem-ram.c | 3 +--
 hw/mem/pc-dimm.c       | 3 +--
 include/qom/object.h   | 8 +++++---
 iothread.c             | 2 +-
 numa.c                 | 4 +++-
 qom/object.c           | 9 ++++-----
 xen-hvm.c              | 9 ++++++---
 7 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref

[Peter Crosthwaite]

The mr->name field is removed. This slipped through compile testing.
Fix.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 xen-hvm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index 91de2e2..9fd9d6e 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -291,6 +291,7 @@ static int xen_add_to_physmap(XenIOState *state,
     hwaddr pfn, start_gpfn;
     hwaddr phys_offset = memory_region_get_ram_addr(mr);
     char path[80], value[17];
+    const char *mr_name;
 
     if (get_physmapping(state, start_addr, size)) {
         return 0;
@@ -326,11 +327,13 @@ go_physmap:
         }
     }
 
+    mr_name = memory_region_name(mr);
+
     physmap = g_malloc(sizeof (XenPhysmap));
 
     physmap->start_addr = start_addr;
     physmap->size = size;
-    physmap->name = (char *)mr->name;
+    physmap->name = (char *)mr_name;
     physmap->phys_offset = phys_offset;
 
     QLIST_INSERT_HEAD(&state->physmap, physmap, list);
@@ -354,11 +357,11 @@ go_physmap:
     if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
         return -1;
     }
-    if (mr->name) {
+    if (mr_name) {
         snprintf(path, sizeof(path),
                 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
                 xen_domid, (uint64_t)phys_offset);
-        if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) {
+        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
             return -1;
         }
     }
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build)

[Stefan Weil]

Am 20.08.2014 um 07:07 schrieb Peter Crosthwaite:
> The mr->name field is removed. This slipped through compile testing.
> Fix.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  xen-hvm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 91de2e2..9fd9d6e 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -291,6 +291,7 @@ static int xen_add_to_physmap(XenIOState *state,
>      hwaddr pfn, start_gpfn;
>      hwaddr phys_offset = memory_region_get_ram_addr(mr);
>      char path[80], value[17];
> +    const char *mr_name;
>  
>      if (get_physmapping(state, start_addr, size)) {
>          return 0;
> @@ -326,11 +327,13 @@ go_physmap:
>          }
>      }
>  
> +    mr_name = memory_region_name(mr);
> +
>      physmap = g_malloc(sizeof (XenPhysmap));
>  
>      physmap->start_addr = start_addr;
>      physmap->size = size;
> -    physmap->name = (char *)mr->name;
> +    physmap->name = (char *)mr_name;

This type cast can be removed. Just add the const in XenPhysmap's name
declaration.

>      physmap->phys_offset = phys_offset;
>  
>      QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> @@ -354,11 +357,11 @@ go_physmap:
>      if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
>          return -1;
>      }
> -    if (mr->name) {
> +    if (mr_name) {
>          snprintf(path, sizeof(path),
>                  "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
>                  xen_domid, (uint64_t)phys_offset);
> -        if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) {
> +        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
>              return -1;
>          }
>      }
> 


Otherwise ok. Below is my own variant which I tested before I noticed
your patch.

Reviewed-by: Stefan Weil <sw@weilnetz.de>


@@ -69,11 +69,11 @@ static inline ioreq_t
*xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 #define BUFFER_IO_MAX_DELAY  100

 typedef struct XenPhysmap {
     hwaddr start_addr;
     ram_addr_t size;
-    char *name;
+    const char *name;
     hwaddr phys_offset;

     QLIST_ENTRY(XenPhysmap) list;
 } XenPhysmap;

@@ -328,11 +328,11 @@ go_physmap:

     physmap = g_malloc(sizeof (XenPhysmap));

     physmap->start_addr = start_addr;
     physmap->size = size;
-    physmap->name = (char *)mr->name;
+    physmap->name = memory_region_name(mr);
     physmap->phys_offset = phys_offset;

     QLIST_INSERT_HEAD(&state->physmap, physmap, list);

     xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
@@ -352,15 +352,16 @@ go_physmap:
             xen_domid, (uint64_t)phys_offset);
     snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
     if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
         return -1;
     }
-    if (mr->name) {
+    if (physmap->name) {
         snprintf(path, sizeof(path),
                 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
                 xen_domid, (uint64_t)phys_offset);
-        if (!xs_write(state->xenstore, 0, path, mr->name,
strlen(mr->name))) {
+        if (!xs_write(state->xenstore, 0, path, physmap->name,
+                      strlen(physmap->name))) {
             return -1;
         }
     }

     return 0;

Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build)

[Peter Maydell]

On 20 August 2014 09:53, Stefan Weil <sw@weilnetz.de> wrote:
> Am 20.08.2014 um 07:07 schrieb Peter Crosthwaite:
>> The mr->name field is removed. This slipped through compile testing.
>> Fix.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  xen-hvm.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen-hvm.c b/xen-hvm.c
>> index 91de2e2..9fd9d6e 100644
>> --- a/xen-hvm.c
>> +++ b/xen-hvm.c
>> @@ -291,6 +291,7 @@ static int xen_add_to_physmap(XenIOState *state,
>>      hwaddr pfn, start_gpfn;
>>      hwaddr phys_offset = memory_region_get_ram_addr(mr);
>>      char path[80], value[17];
>> +    const char *mr_name;
>>
>>      if (get_physmapping(state, start_addr, size)) {
>>          return 0;
>> @@ -326,11 +327,13 @@ go_physmap:
>>          }
>>      }
>>
>> +    mr_name = memory_region_name(mr);
>> +
>>      physmap = g_malloc(sizeof (XenPhysmap));
>>
>>      physmap->start_addr = start_addr;
>>      physmap->size = size;
>> -    physmap->name = (char *)mr->name;
>> +    physmap->name = (char *)mr_name;
>
> This type cast can be removed. Just add the const in XenPhysmap's name
> declaration.

Yes, I noticed that dodgy cast too. Stefano, is there
a reason why the XenPhysmap field isn't just a
const pointer?

thanks
-- PMM

Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build)

[Stefano Stabellini]

On Wed, 20 Aug 2014, Peter Maydell wrote:
> On 20 August 2014 09:53, Stefan Weil <sw@weilnetz.de> wrote:
> > Am 20.08.2014 um 07:07 schrieb Peter Crosthwaite:
> >> The mr->name field is removed. This slipped through compile testing.
> >> Fix.
> >>
> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> ---
> >>
> >>  xen-hvm.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen-hvm.c b/xen-hvm.c
> >> index 91de2e2..9fd9d6e 100644
> >> --- a/xen-hvm.c
> >> +++ b/xen-hvm.c
> >> @@ -291,6 +291,7 @@ static int xen_add_to_physmap(XenIOState *state,
> >>      hwaddr pfn, start_gpfn;
> >>      hwaddr phys_offset = memory_region_get_ram_addr(mr);
> >>      char path[80], value[17];
> >> +    const char *mr_name;
> >>
> >>      if (get_physmapping(state, start_addr, size)) {
> >>          return 0;
> >> @@ -326,11 +327,13 @@ go_physmap:
> >>          }
> >>      }
> >>
> >> +    mr_name = memory_region_name(mr);
> >> +
> >>      physmap = g_malloc(sizeof (XenPhysmap));
> >>
> >>      physmap->start_addr = start_addr;
> >>      physmap->size = size;
> >> -    physmap->name = (char *)mr->name;
> >> +    physmap->name = (char *)mr_name;
> >
> > This type cast can be removed. Just add the const in XenPhysmap's name
> > declaration.
> 
> Yes, I noticed that dodgy cast too. Stefano, is there
> a reason why the XenPhysmap field isn't just a
> const pointer?
 
No reason, we could just switch to const char*. I would be happy to take
a patch for that.

Sorry for the late reply, I am attending XenSummit/LinuxCon.

[Qemu-devel] [PATCH bugfix v1 2/3] qom: return const for object_get_canon_path component

[Peter Crosthwaite]

Most (if not all) calls sites of object_get_canonical_path component
use it to get a read-only version for usage as an object name. Avoid
the need for the callers to explicitly free the returned string and mark
it const accordingly.

Frees are removed from call sites accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 backends/hostmem-ram.c | 3 +--
 hw/mem/pc-dimm.c       | 3 +--
 include/qom/object.h   | 2 +-
 iothread.c             | 2 +-
 numa.c                 | 4 +++-
 qom/object.c           | 9 ++++-----
 6 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index d9a8290..a13ec23 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -18,7 +18,7 @@
 static void
 ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
-    char *path;
+    const char *path;
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
@@ -28,7 +28,6 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     path = object_get_canonical_path_component(OBJECT(backend));
     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                            backend->size);
-    g_free(path);
 }
 
 static void
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 5bfc5b7..bc0ef18 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -223,9 +223,8 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name,
 
     mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp);
     if (memory_region_is_mapped(mr)) {
-        char *path = object_get_canonical_path_component(val);
+        const char *path = object_get_canonical_path_component(val);
         error_setg(errp, "can't use already busy memdev: %s", path);
-        g_free(path);
     } else {
         qdev_prop_allow_set_link_before_realize(obj, name, val, errp);
     }
diff --git a/include/qom/object.h b/include/qom/object.h
index 8a05a81..8618e49 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1031,7 +1031,7 @@ Object *object_get_root(void);
  * Returns: The final component in the object's canonical path.  The canonical
  * path is the path within the composition tree starting from the root.
  */
-gchar *object_get_canonical_path_component(Object *obj);
+const gchar *object_get_canonical_path_component(Object *obj);
 
 /**
  * object_get_canonical_path:
diff --git a/iothread.c b/iothread.c
index d9403cf..37cb0af 100644
--- a/iothread.c
+++ b/iothread.c
@@ -126,7 +126,7 @@ IOThread *iothread_find(const char *id)
 
 char *iothread_get_id(IOThread *iothread)
 {
-    return object_get_canonical_path_component(OBJECT(iothread));
+    return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
 }
 
 AioContext *iothread_get_aio_context(IOThread *iothread)
diff --git a/numa.c b/numa.c
index c78cec9..3e05ac5 100644
--- a/numa.c
+++ b/numa.c
@@ -302,7 +302,9 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
         }
 
         if (memory_region_is_mapped(seg)) {
-            char *path = object_get_canonical_path_component(OBJECT(backend));
+            const char *path;
+
+            path = object_get_canonical_path_component(OBJECT(backend));
             error_report("memory backend %s is used multiple times. Each "
                          "-numa option must use a different memdev value.",
                          path);
diff --git a/qom/object.c b/qom/object.c
index 1b00831..a7fd480 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1219,7 +1219,7 @@ out:
     g_free(full_type);
 }
 
-gchar *object_get_canonical_path_component(Object *obj)
+const gchar *object_get_canonical_path_component(Object *obj)
 {
     ObjectProperty *prop = NULL;
 
@@ -1232,7 +1232,7 @@ gchar *object_get_canonical_path_component(Object *obj)
         }
 
         if (prop->opaque == obj) {
-            return g_strdup(prop->name);
+            return prop->name;
         }
     }
 
@@ -1247,15 +1247,14 @@ gchar *object_get_canonical_path(Object *obj)
     char *newpath, *path = NULL;
 
     while (obj != root) {
-        char *component = object_get_canonical_path_component(obj);
+        const char *component = object_get_canonical_path_component(obj);
 
         if (path) {
             newpath = g_strdup_printf("%s/%s", component, path);
-            g_free(component);
             g_free(path);
             path = newpath;
         } else {
-            path = component;
+            path = g_strdup(component);
         }
 
         obj = obj->parent;
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Peter Crosthwaite]

With information about return value ownership.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/qom/object.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 8618e49..87de889 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1029,7 +1029,8 @@ Object *object_get_root(void);
  * object_get_canonical_path_component:
  *
  * Returns: The final component in the object's canonical path.  The canonical
- * path is the path within the composition tree starting from the root.
+ * path is the path within the composition tree starting from the root. The
+ * returned value may not be modified.
  */
 const gchar *object_get_canonical_path_component(Object *obj);
 
@@ -1037,7 +1038,8 @@ const gchar *object_get_canonical_path_component(Object *obj);
  * object_get_canonical_path:
  *
  * Returns: The canonical path for a object.  This is the path within the
- * composition tree starting from the root.
+ * composition tree starting from the root.  The returned value is allocated
+ * memory and the client is responsible for freeing the returned value.
  */
 gchar *object_get_canonical_path(Object *obj);
 
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Peter Maydell]

On 20 August 2014 06:08, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> With information about return value ownership.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  include/qom/object.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8618e49..87de889 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1029,7 +1029,8 @@ Object *object_get_root(void);
>   * object_get_canonical_path_component:
>   *
>   * Returns: The final component in the object's canonical path.  The canonical
> - * path is the path within the composition tree starting from the root.
> + * path is the path within the composition tree starting from the root. The
> + * returned value may not be modified.
>   */
>  const gchar *object_get_canonical_path_component(Object *obj);

The other thing you need to say is that the returned string is
only valid for as long as the object remains a child property
of its parent. (Is that right? I'm not clear. It also sounds like
a really awkward lifetime management requirement, which
suggests to me that really the "return-a-copy" semantics are
nicer.)

Having object_get_canonical_path_component() and
object_get_canonical_path() having different return value
ownership semantics is likely to be a bit confusing I think.

If we do do this I think I'd put the doc comment change in
the same patch that changes the semantics.

thanks
-- PMM

Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Peter Crosthwaite]

On Wed, Aug 20, 2014 at 7:07 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 August 2014 06:08, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> With information about return value ownership.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  include/qom/object.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 8618e49..87de889 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1029,7 +1029,8 @@ Object *object_get_root(void);
>>   * object_get_canonical_path_component:
>>   *
>>   * Returns: The final component in the object's canonical path.  The canonical
>> - * path is the path within the composition tree starting from the root.
>> + * path is the path within the composition tree starting from the root. The
>> + * returned value may not be modified.
>>   */
>>  const gchar *object_get_canonical_path_component(Object *obj);
>
> The other thing you need to say is that the returned string is
> only valid for as long as the object remains a child property
> of its parent. (Is that right? I'm not clear. It also sounds like
> a really awkward lifetime management requirement, which
> suggests to me that really the "return-a-copy" semantics are
> nicer.)

It is. But in QOM that is the naming scheme. An object is uniquely
identified by it's full canonical path. If you don't have a parent,
you don't have a name. If you have a dup of the name, but then the
object gets reparented or destroyed your name is stale and I think the
caller should have to deal with that.

>
> Having object_get_canonical_path_component() and
> object_get_canonical_path() having different return value
> ownership semantics is likely to be a bit confusing I think.
>

Alternatives I can think of include,

1: All memory_region_name() callsites have to do a free.
2: Memory API keeps a single copy of the name (lazily inited in
memory_region_name perhaps).
3: Patch struct object with a char instance_name[] field and have QOM
ensure the fixed location is always a valid name ("\0" for an
unparented object).
4: This

Please let me know your preference.

Regards,
Peter

> If we do do this I think I'd put the doc comment change in
> the same patch that changes the semantics.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Paolo Bonzini]

Il 20/08/2014 11:07, Peter Maydell ha scritto:
> The other thing you need to say is that the returned string is
> only valid for as long as the object remains a child property
> of its parent. (Is that right? I'm not clear. It also sounds like
> a really awkward lifetime management requirement, which
> suggests to me that really the "return-a-copy" semantics are
> nicer.)

They are indeed nicer, but harder to use in C.

Unparenting is (currently) always done under the BQL, so guaranteeing
that the returned string stays alive as long as you use it is currently
not a problem.  This might change in the future, but I don't see many
reasons to get rid completely of the BQL (as opposed to the surgical
moving of stuff outside it that we've been doing so far).

> Having object_get_canonical_path_component() and
> object_get_canonical_path() having different return value
> ownership semantics is likely to be a bit confusing I think.

Right, though that could be solved by renaming the _component() function.

Paolo

Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Peter Crosthwaite]

On Thu, Aug 21, 2014 at 1:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 20/08/2014 11:07, Peter Maydell ha scritto:
>> The other thing you need to say is that the returned string is
>> only valid for as long as the object remains a child property
>> of its parent. (Is that right? I'm not clear. It also sounds like
>> a really awkward lifetime management requirement, which
>> suggests to me that really the "return-a-copy" semantics are
>> nicer.)
>
> They are indeed nicer, but harder to use in C.
>
> Unparenting is (currently) always done under the BQL, so guaranteeing
> that the returned string stays alive as long as you use it is currently
> not a problem.  This might change in the future, but I don't see many
> reasons to get rid completely of the BQL (as opposed to the surgical
> moving of stuff outside it that we've been doing so far).
>
>> Having object_get_canonical_path_component() and
>> object_get_canonical_path() having different return value
>> ownership semantics is likely to be a bit confusing I think.
>
> Right, though that could be solved by renaming the _component() function.
>

Yes that had crossed my mind too. object_get_canonical_path_component
is a mouthful and it's usages suggest object_get_name or
object_get_instance_name would be more fitting.

The fact that it is a canonical_path_component is more of an
implementation detail rather than its externally useful functionality.

Regards,
Peter

> Paolo
>