Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On 10/03/2017 10:36, Peter Xu wrote: > On Fri, Mar 10, 2017 at 10:17:30AM +0100, Peter Maydell wrote: >> On 10 March 2017 at 09:59, Peter Xuwrote: >>> On Fri, Mar 10, 2017 at 09:33:57AM +0100, Paolo Bonzini wrote: On 10/03/2017 05:13, Peter Xu wrote: > Trying to get memory region size of an uninitialized memory region is > probably not a good idea. Let's just do the alloc no matter what. > > Signed-off-by: Peter Xu What is the effect of the bug? The idea was to do the initialization once only (memory_region_size ought to be 0 when the MR is uninitialized; now it is ugly but it made more sense when MemoryRegion was just a C struct and not a QOM object). >>> >>> It's not really a bug. I just saw it, thought it was something not >>> quite right, so posted a patch. >> >> We could reasonably abstract out the test into a function >> bool backend_mr_initialized(HostMemoryBackend *backend) >> { >> /* We forbid zero-length in file_backend_memory_alloc, >> * so zero always means "we haven't allocated the backend >> * MR yet". >> */ >> return memory_region_size(>mr) != 0; >> } >> >> and use it in file_backend_memory_alloc(), set_mem_path() >> and file_memory_backend_set_share(). That would make the intent >> clearer here I think. > > Agree, maybe use it in hostmem.c as well where capable? > > (Paolo: I wasn't planning to add anything there, but if any of you > like me to do this cleanup, I would be glad to :) Yes, please (to name things more consistently it should be host_memory_backend_initialized). Paolo
Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On Fri, Mar 10, 2017 at 10:17:30AM +0100, Peter Maydell wrote: > On 10 March 2017 at 09:59, Peter Xuwrote: > > On Fri, Mar 10, 2017 at 09:33:57AM +0100, Paolo Bonzini wrote: > >> > >> > >> On 10/03/2017 05:13, Peter Xu wrote: > >> > Trying to get memory region size of an uninitialized memory region is > >> > probably not a good idea. Let's just do the alloc no matter what. > >> > > >> > Signed-off-by: Peter Xu > >> > >> What is the effect of the bug? The idea was to do the initialization > >> once only (memory_region_size ought to be 0 when the MR is > >> uninitialized; now it is ugly but it made more sense when MemoryRegion > >> was just a C struct and not a QOM object). > > > > It's not really a bug. I just saw it, thought it was something not > > quite right, so posted a patch. > > We could reasonably abstract out the test into a function > bool backend_mr_initialized(HostMemoryBackend *backend) > { > /* We forbid zero-length in file_backend_memory_alloc, > * so zero always means "we haven't allocated the backend > * MR yet". > */ > return memory_region_size(>mr) != 0; > } > > and use it in file_backend_memory_alloc(), set_mem_path() > and file_memory_backend_set_share(). That would make the intent > clearer here I think. Agree, maybe use it in hostmem.c as well where capable? (Paolo: I wasn't planning to add anything there, but if any of you like me to do this cleanup, I would be glad to :) -- peterx
Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On 10/03/2017 09:59, Peter Xu wrote: >> What is the effect of the bug? The idea was to do the initialization >> once only (memory_region_size ought to be 0 when the MR is >> uninitialized; now it is ugly but it made more sense when MemoryRegion >> was just a C struct and not a QOM object). > It's not really a bug. I just saw it, thought it was something not > quite right, so posted a patch. Since it's intended, then please > ignore this patch, and sorry for the noise... :) It's intended but ugly. :) Peter's idea is a good one if you want to implement something along those lines. Paolo
Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On 10 March 2017 at 09:59, Peter Xuwrote: > On Fri, Mar 10, 2017 at 09:33:57AM +0100, Paolo Bonzini wrote: >> >> >> On 10/03/2017 05:13, Peter Xu wrote: >> > Trying to get memory region size of an uninitialized memory region is >> > probably not a good idea. Let's just do the alloc no matter what. >> > >> > Signed-off-by: Peter Xu >> >> What is the effect of the bug? The idea was to do the initialization >> once only (memory_region_size ought to be 0 when the MR is >> uninitialized; now it is ugly but it made more sense when MemoryRegion >> was just a C struct and not a QOM object). > > It's not really a bug. I just saw it, thought it was something not > quite right, so posted a patch. We could reasonably abstract out the test into a function bool backend_mr_initialized(HostMemoryBackend *backend) { /* We forbid zero-length in file_backend_memory_alloc, * so zero always means "we haven't allocated the backend * MR yet". */ return memory_region_size(>mr) != 0; } and use it in file_backend_memory_alloc(), set_mem_path() and file_memory_backend_set_share(). That would make the intent clearer here I think. thanks -- PMM
Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On Fri, Mar 10, 2017 at 09:33:57AM +0100, Paolo Bonzini wrote: > > > On 10/03/2017 05:13, Peter Xu wrote: > > Trying to get memory region size of an uninitialized memory region is > > probably not a good idea. Let's just do the alloc no matter what. > > > > Signed-off-by: Peter Xu> > What is the effect of the bug? The idea was to do the initialization > once only (memory_region_size ought to be 0 when the MR is > uninitialized; now it is ugly but it made more sense when MemoryRegion > was just a C struct and not a QOM object). It's not really a bug. I just saw it, thought it was something not quite right, so posted a patch. Since it's intended, then please ignore this patch, and sorry for the noise... :) (Considering it's a QOM object, it'll better explain itself if we were using something like object_initialized() here for the check, but since we don't have that, I think fetching size is okay as well) Thanks, -- peterx
Re: [Qemu-devel] [PATCH] hostmem: fix reference to uninit mr
On 10/03/2017 05:13, Peter Xu wrote: > Trying to get memory region size of an uninitialized memory region is > probably not a good idea. Let's just do the alloc no matter what. > > Signed-off-by: Peter XuWhat is the effect of the bug? The idea was to do the initialization once only (memory_region_size ought to be 0 when the MR is uninitialized; now it is ugly but it made more sense when MemoryRegion was just a C struct and not a QOM object). Paolo > --- > backends/hostmem-file.c | 17 +++-- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c > index 42efb2f..a61d1bd 100644 > --- a/backends/hostmem-file.c > +++ b/backends/hostmem-file.c > @@ -39,6 +39,7 @@ static void > file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) > { > HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend); > +gchar *path; > > if (!backend->size) { > error_setg(errp, "can't create backend with size 0"); > @@ -51,16 +52,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, > Error **errp) > #ifndef CONFIG_LINUX > error_setg(errp, "-mem-path not supported on this host"); > #else > -if (!memory_region_size(>mr)) { > -gchar *path; > -backend->force_prealloc = mem_prealloc; > -path = object_get_canonical_path(OBJECT(backend)); > -memory_region_init_ram_from_file(>mr, OBJECT(backend), > - path, > - backend->size, fb->share, > - fb->mem_path, errp); > -g_free(path); > -} > +backend->force_prealloc = mem_prealloc; > +path = object_get_canonical_path(OBJECT(backend)); > +memory_region_init_ram_from_file(>mr, OBJECT(backend), > + path, backend->size, fb->share, > + fb->mem_path, errp); > +g_free(path); > #endif > } > >