| Message ID | 1489119213-977-1-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <peterx@redhat.com> 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). 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(&backend->mr)) { > - gchar *path; > - backend->force_prealloc = mem_prealloc; > - path = object_get_canonical_path(OBJECT(backend)); > - memory_region_init_ram_from_file(&backend->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(&backend->mr, OBJECT(backend), > + path, backend->size, fb->share, > + fb->mem_path, errp); > + g_free(path); > #endif > } > >
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 <peterx@redhat.com> > > 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
On 10 March 2017 at 09:59, Peter Xu <peterx@redhat.com> wrote: > 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 <peterx@redhat.com> >> >> 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(&backend->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
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
On Fri, Mar 10, 2017 at 10:17:30AM +0100, Peter Maydell wrote: > On 10 March 2017 at 09:59, Peter Xu <peterx@redhat.com> wrote: > > 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 <peterx@redhat.com> > >> > >> 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(&backend->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
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 Xu <peterx@redhat.com> wrote: >>> 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 <peterx@redhat.com> >>>> >>>> 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(&backend->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
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(&backend->mr)) { - gchar *path; - backend->force_prealloc = mem_prealloc; - path = object_get_canonical_path(OBJECT(backend)); - memory_region_init_ram_from_file(&backend->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(&backend->mr, OBJECT(backend), + path, backend->size, fb->share, + fb->mem_path, errp); + g_free(path); #endif }
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 <peterx@redhat.com> --- backends/hostmem-file.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)