| Message ID | 1408444983-21464-20-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 19 August 2014 11:43, Paolo Bonzini <pbonzini@redhat.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > Rather than having the name as separate state. This prepares support > for creating a MemoryRegion dynamically (i.e. without > memory_region_init() and friends) and the MemoryRegion still getting > a usable name. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> This commit seems to have broken builds with Xen: CC i386-softmmu/xen-hvm.o /home/petmay01/linaro/qemu-for-merges/xen-hvm.c: In function ‘xen_add_to_physmap’: /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:333:31: error: ‘MemoryRegion’ has no member named ‘name’ physmap->name = (char *)mr->name; ^ /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:357:11: error: ‘MemoryRegion’ has no member named ‘name’ if (mr->name) { ^ /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:361:51: error: ‘MemoryRegion’ has no member named ‘name’ if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) { ^ /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:361:68: error: ‘MemoryRegion’ has no member named ‘name’ if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) { ^ make[1]: *** [xen-hvm.o] Error 1 make: *** [subdir-i386-softmmu] Error 2 I should have spotted this as part of my pre-merge tests, but somehow it slipped through -- sorry. -- PMM
On 19 August 2014 11:43, Paolo Bonzini <pbonzini@redhat.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > Rather than having the name as separate state. This prepares support > for creating a MemoryRegion dynamically (i.e. without > memory_region_init() and friends) and the MemoryRegion still getting > a usable name. > @@ -1310,7 +1308,7 @@ uint64_t memory_region_size(MemoryRegion *mr) > > const char *memory_region_name(const MemoryRegion *mr) > { > - return mr->name; > + return object_get_canonical_path_component(OBJECT(mr)); > } This doesn't look right. It changes the semantics of this function from "returns a string which you don't own and can't change but don't need to free" to "returns a copy of a string which you have to free with g_free() when you're done". Unsurprisingly, the callsites aren't expecting this and we leak the string all over the place. I think we need to revert this (commit b0225c2c0d8) until both the Xen callsites are fixed and the leak issue is dealt with. thanks -- PMM
On Wed, Aug 20, 2014 at 4:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 August 2014 11:43, Paolo Bonzini <pbonzini@redhat.com> wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> Rather than having the name as separate state. This prepares support >> for creating a MemoryRegion dynamically (i.e. without >> memory_region_init() and friends) and the MemoryRegion still getting >> a usable name. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > This commit seems to have broken builds with Xen: > > CC i386-softmmu/xen-hvm.o > /home/petmay01/linaro/qemu-for-merges/xen-hvm.c: In function > ‘xen_add_to_physmap’: > /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:333:31: error: > ‘MemoryRegion’ has no member named ‘name’ > physmap->name = (char *)mr->name; > ^ > /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:357:11: error: > ‘MemoryRegion’ has no member named ‘name’ > if (mr->name) { > ^ > /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:361:51: error: > ‘MemoryRegion’ has no member named ‘name’ > if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) { > ^ > /home/petmay01/linaro/qemu-for-merges/xen-hvm.c:361:68: error: > ‘MemoryRegion’ has no member named ‘name’ > if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) { > ^ > make[1]: *** [xen-hvm.o] Error 1 > make: *** [subdir-i386-softmmu] Error 2 > > > I should have spotted this as part of my pre-merge > tests, but somehow it slipped through -- sorry. > You and me both. Added Xen to my pre-send compile testing. The fix for this is reasonably trivial and just follows the change pattern for the other usages as fixed in original series. Patch on list imminently. Regards, Peter > -- PMM >
On Wed, Aug 20, 2014 at 5:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 August 2014 11:43, Paolo Bonzini <pbonzini@redhat.com> wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> Rather than having the name as separate state. This prepares support >> for creating a MemoryRegion dynamically (i.e. without >> memory_region_init() and friends) and the MemoryRegion still getting >> a usable name. >> @@ -1310,7 +1308,7 @@ uint64_t memory_region_size(MemoryRegion *mr) >> >> const char *memory_region_name(const MemoryRegion *mr) >> { >> - return mr->name; >> + return object_get_canonical_path_component(OBJECT(mr)); >> } > > This doesn't look right. It changes the semantics of this function > from "returns a string which you don't own and can't change > but don't need to free" to "returns a copy of a string which you > have to free with g_free() when you're done". Unsurprisingly, > the callsites aren't expecting this and we leak the string all > over the place. > > I think we need to revert this (commit b0225c2c0d8) until > both the Xen callsites are fixed and the leak issue is > dealt with. > Have half a plan on the leak issue. With object_get_canonical_path_component() being used increasing as a "get me the already set, name of this object", I think it's better to just change to API to return const and remove the free burden from all call sites completely. If call sites really want a fresh copy they can take one themselves. Patch on list imminently. Regards, Peter > thanks > -- PMM >
On 20 August 2014 06:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Aug 20, 2014 at 5:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> I think we need to revert this (commit b0225c2c0d8) until >> both the Xen callsites are fixed and the leak issue is >> dealt with. >> > > Have half a plan on the leak issue. With > object_get_canonical_path_component() being used increasing as a "get > me the already set, name of this object", I think it's better to just > change to API to return const and remove the free burden from all call > sites completely. If call sites really want a fresh copy they can take > one themselves. Patch on list imminently. I'm going to go ahead and revert this patch for the moment so we can take our time reviewing that patch set. thanks -- PMM
Il 20/08/2014 09:50, Peter Maydell ha scritto: > On 20 August 2014 06:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> On Wed, Aug 20, 2014 at 5:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> I think we need to revert this (commit b0225c2c0d8) until >>> both the Xen callsites are fixed and the leak issue is >>> dealt with. >>> >> >> Have half a plan on the leak issue. With >> object_get_canonical_path_component() being used increasing as a "get >> me the already set, name of this object", I think it's better to just >> change to API to return const and remove the free burden from all call >> sites completely. If call sites really want a fresh copy they can take >> one themselves. Patch on list imminently. > > I'm going to go ahead and revert this patch for the moment so > we can take our time reviewing that patch set. Yes, thanks. Paolo
diff --git a/include/exec/memory.h b/include/exec/memory.h index d165b27..10f73d9 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -162,7 +162,6 @@ struct MemoryRegion { QTAILQ_HEAD(subregions, MemoryRegion) subregions; QTAILQ_ENTRY(MemoryRegion) subregions_link; QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced; - const char *name; uint8_t dirty_log_mask; unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; diff --git a/memory.c b/memory.c index b6b208f..8da29af 100644 --- a/memory.c +++ b/memory.c @@ -915,7 +915,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)); @@ -1260,7 +1259,6 @@ static void memory_region_finalize(Object *obj) assert(memory_region_transaction_depth == 0); mr->destructor(mr); memory_region_clear_coalescing(mr); - g_free((char *)mr->name); g_free(mr->ioeventfds); } @@ -1310,7 +1308,7 @@ uint64_t memory_region_size(MemoryRegion *mr) const char *memory_region_name(const MemoryRegion *mr) { - return mr->name; + return object_get_canonical_path_component(OBJECT(mr)); } bool memory_region_is_ram(MemoryRegion *mr)