* [Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs @ 2014-08-20 5:06 Peter Crosthwaite 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref Peter Crosthwaite ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-20 5:06 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref 2014-08-20 5:06 [Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs Peter Crosthwaite @ 2014-08-20 5:07 ` Peter Crosthwaite 2014-08-20 8:53 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) Stefan Weil 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 2/3] qom: return const for object_get_canon_path component Peter Crosthwaite 2014-08-20 5:08 ` [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc Peter Crosthwaite 2 siblings, 1 reply; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-20 5:07 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref Peter Crosthwaite @ 2014-08-20 8:53 ` Stefan Weil 2014-08-20 8:55 ` Peter Maydell 0 siblings, 1 reply; 11+ messages in thread From: Stefan Weil @ 2014-08-20 8:53 UTC (permalink / raw) To: Peter Crosthwaite, qemu-devel, peter.maydell; +Cc: pbonzini 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) 2014-08-20 8:53 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) Stefan Weil @ 2014-08-20 8:55 ` Peter Maydell 2014-08-21 19:27 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Peter Maydell @ 2014-08-20 8:55 UTC (permalink / raw) To: Stefan Weil Cc: Paolo Bonzini, Peter Crosthwaite, QEMU Developers, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) 2014-08-20 8:55 ` Peter Maydell @ 2014-08-21 19:27 ` Stefano Stabellini 0 siblings, 0 replies; 11+ messages in thread From: Stefano Stabellini @ 2014-08-21 19:27 UTC (permalink / raw) To: Peter Maydell Cc: Stefan Weil, Peter Crosthwaite, Stefano Stabellini, QEMU Developers, Paolo Bonzini 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH bugfix v1 2/3] qom: return const for object_get_canon_path component 2014-08-20 5:06 [Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs Peter Crosthwaite 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref Peter Crosthwaite @ 2014-08-20 5:07 ` Peter Crosthwaite 2014-08-20 5:08 ` [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc Peter Crosthwaite 2 siblings, 0 replies; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-20 5:07 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc 2014-08-20 5:06 [Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs Peter Crosthwaite 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref Peter Crosthwaite 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 2/3] qom: return const for object_get_canon_path component Peter Crosthwaite @ 2014-08-20 5:08 ` Peter Crosthwaite 2014-08-20 9:07 ` Peter Maydell 2 siblings, 1 reply; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-20 5:08 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc 2014-08-20 5:08 ` [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc Peter Crosthwaite @ 2014-08-20 9:07 ` Peter Maydell 2014-08-20 14:54 ` Peter Crosthwaite 2014-08-20 15:58 ` Paolo Bonzini 0 siblings, 2 replies; 11+ messages in thread From: Peter Maydell @ 2014-08-20 9:07 UTC (permalink / raw) To: Peter Crosthwaite; +Cc: Paolo Bonzini, QEMU Developers 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc 2014-08-20 9:07 ` Peter Maydell @ 2014-08-20 14:54 ` Peter Crosthwaite 2014-08-20 15:58 ` Paolo Bonzini 1 sibling, 0 replies; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-20 14:54 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc 2014-08-20 9:07 ` Peter Maydell 2014-08-20 14:54 ` Peter Crosthwaite @ 2014-08-20 15:58 ` Paolo Bonzini 2014-08-21 3:10 ` Peter Crosthwaite 1 sibling, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2014-08-20 15:58 UTC (permalink / raw) To: Peter Maydell, Peter Crosthwaite; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc 2014-08-20 15:58 ` Paolo Bonzini @ 2014-08-21 3:10 ` Peter Crosthwaite 0 siblings, 0 replies; 11+ messages in thread From: Peter Crosthwaite @ 2014-08-21 3:10 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Peter Maydell, QEMU Developers 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-08-21 19:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-20 5:06 [Qemu-devel] [PATCH bugfix v1 0/3] Fix Memory Region Name bugs Peter Crosthwaite 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref Peter Crosthwaite 2014-08-20 8:53 ` [Qemu-devel] [PATCH bugfix v1 1/3] xen: hvm: Abstract away memory region name ref (fix broken build) Stefan Weil 2014-08-20 8:55 ` Peter Maydell 2014-08-21 19:27 ` Stefano Stabellini 2014-08-20 5:07 ` [Qemu-devel] [PATCH bugfix v1 2/3] qom: return const for object_get_canon_path component Peter Crosthwaite 2014-08-20 5:08 ` [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc Peter Crosthwaite 2014-08-20 9:07 ` Peter Maydell 2014-08-20 14:54 ` Peter Crosthwaite 2014-08-20 15:58 ` Paolo Bonzini 2014-08-21 3:10 ` Peter Crosthwaite
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.