| Message ID | 20161024092151.32386-3-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: > If 'size' option of hostmem-file is not given, QEMU will use the file > size of 'mem-path' instead. For an empty file, a non-zero size must be > specified by the option 'size'. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > backends/hostmem-file.c | 10 ++++++---- > exec.c | 8 ++++++++ > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c > index 42efb2f..f94d2f7 100644 > --- a/backends/hostmem-file.c > +++ b/backends/hostmem-file.c > @@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) > { > HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend); > > - if (!backend->size) { > - error_setg(errp, "can't create backend with size 0"); > - return; > - } > if (!fb->mem_path) { > error_setg(errp, "mem-path property not set"); > return; > @@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) > g_free(path); > } > #endif > + if (!errp && !backend->size) { This condition is always false because non-NULL errp is always provided by the only caller (host_memory_backend_memory_complete()). To simplify error checking, I suggest moving the error path to a label at the end of the function, e.g.: static void file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) { Error *local_err = NULL; /* ... */ memory_region_init_ram_from_file(..., &local_err); if (local_err) { goto out; } /* ... */ if (!backend->size) { backend->size = memory_region_size(&backend->mr); } /* ... */ out: error_propagate(errp, local_err); } > + backend->size = memory_region_size(&backend->mr); > + if (!backend->size) { > + error_setg(errp, "can't create backend with size 0"); > + } > + } > } > > static char *get_mem_path(Object *o, Error **errp) > diff --git a/exec.c b/exec.c > index 95983c9..91adc62 100644 > --- a/exec.c > +++ b/exec.c > @@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block, > goto error; > } > > + if (memory) { > + memory = memory ?: file_size; This doesn't make sense to me. You already checked if memory is zero above, and now you are checking if it's zero again. file_size is never going to be used here. > + memory_region_set_size(block->mr, memory); > + memory = HOST_PAGE_ALIGN(memory); > + block->used_length = memory; > + block->max_length = memory; This is fragile: it duplicates the logic that initializes used_length and max_length in qemu_ram_alloc_*(). Maybe it's better to keep the file-size-probing magic inside hostmem-file.c, and always give a non-zero size to memory_region_init_ram_from_file(). > + } > + > if (memory < block->page_size) { > error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " > "or larger than page size 0x%zx", > -- > 2.10.1 >
On 10/25/16 17:50 -0200, Eduardo Habkost wrote: >On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: >> If 'size' option of hostmem-file is not given, QEMU will use the file >> size of 'mem-path' instead. For an empty file, a non-zero size must be >> specified by the option 'size'. >> >> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> >> --- >> backends/hostmem-file.c | 10 ++++++---- >> exec.c | 8 ++++++++ >> 2 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c >> index 42efb2f..f94d2f7 100644 >> --- a/backends/hostmem-file.c >> +++ b/backends/hostmem-file.c >> @@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) >> { >> HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend); >> >> - if (!backend->size) { >> - error_setg(errp, "can't create backend with size 0"); >> - return; >> - } >> if (!fb->mem_path) { >> error_setg(errp, "mem-path property not set"); >> return; >> @@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) >> g_free(path); >> } >> #endif >> + if (!errp && !backend->size) { > >This condition is always false because non-NULL errp is always >provided by the only caller (host_memory_backend_memory_complete()). > Oops, I meant !*errp. Anyway, I'll change to the way you suggested below. >To simplify error checking, I suggest moving the error path to a >label at the end of the function, e.g.: > >static void file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) >{ > Error *local_err = NULL; > /* ... */ > memory_region_init_ram_from_file(..., &local_err); > if (local_err) { > goto out; > } > /* ... */ > if (!backend->size) { > backend->size = memory_region_size(&backend->mr); > } > /* ... */ >out: > error_propagate(errp, local_err); >} > >> + backend->size = memory_region_size(&backend->mr); >> + if (!backend->size) { >> + error_setg(errp, "can't create backend with size 0"); >> + } >> + } >> } >> >> static char *get_mem_path(Object *o, Error **errp) >> diff --git a/exec.c b/exec.c >> index 95983c9..91adc62 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block, >> goto error; >> } >> >> + if (memory) { >> + memory = memory ?: file_size; > >This doesn't make sense to me. You already checked if memory is >zero above, and now you are checking if it's zero again. >file_size is never going to be used here. > >> + memory_region_set_size(block->mr, memory); >> + memory = HOST_PAGE_ALIGN(memory); >> + block->used_length = memory; >> + block->max_length = memory; > >This is fragile: it duplicates the logic that initializes >used_length and max_length in qemu_ram_alloc_*(). > >Maybe it's better to keep the file-size-probing magic inside >hostmem-file.c, and always give a non-zero size to >memory_region_init_ram_from_file(). > Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file(). Thanks, Haozhong >> + } >> + >> if (memory < block->page_size) { >> error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " >> "or larger than page size 0x%zx", >> -- >> 2.10.1 >> > >-- >Eduardo
On 10/26/16 13:56 +0800, Haozhong Zhang wrote: >On 10/25/16 17:50 -0200, Eduardo Habkost wrote: >>On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: >>>+ if (memory) { >>>+ memory = memory ?: file_size; >> >>This doesn't make sense to me. You already checked if memory is >>zero above, and now you are checking if it's zero again. >>file_size is never going to be used here. >> >>>+ memory_region_set_size(block->mr, memory); >>>+ memory = HOST_PAGE_ALIGN(memory); >>>+ block->used_length = memory; >>>+ block->max_length = memory; >> >>This is fragile: it duplicates the logic that initializes >>used_length and max_length in qemu_ram_alloc_*(). >> >>Maybe it's better to keep the file-size-probing magic inside >>hostmem-file.c, and always give a non-zero size to >>memory_region_init_ram_from_file(). >> > >Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file(). > Maybe no. Two reasons: 1) Patch 1 has some code related to file size and I'd like to put all logic related to file size in the same function. 2) file_ram_alloc() has the logic to open/create/close the backend file and handle errors in this process, which also needs to move to qemu_ram_alloc_from_file() if file-size-probing is moved. Instead, I'd 1) keep all logic related to file size in file_ram_alloc() 2) let file_ram_alloc() return the value of 'memory', e.g. static void *file_ram_alloc(RAMBlock *block, - ram_addr_t *memory, + ram_addr_t memory, const char *path, Error **errp) { ... // other usages of 'memory' are changed as well - memory = ROUND_UP(*memory, block->page_size); + *memory = ROUND_UP(*memory, block->page_size); ... } 3) in qemu_ram_alloc_from_file(), move setting block->used_length/max_length after calling file_ram_alloc(), e.g. RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, bool share, const char *mem_path, Error **errp) { ... size = HOST_PAGE_ALIGN(size); new_block = g_malloc0(sizeof(*new_block)); new_block->mr = mr; - new_block->used_length = size; - new_block->max_length = size; new_block->flags = share ? RAM_SHARED : 0; - new_block->host = file_ram_alloc(new_block, size, + new_block->host = file_ram_alloc(new_block, &size, mem_path, errp); if (!new_block->host) { g_free(new_block); return NULL; } + new_block->used_length = size; + new_block->max_length = size; ... }
On Wed, Oct 26, 2016 at 03:49:29PM +0800, Haozhong Zhang wrote: > On 10/26/16 13:56 +0800, Haozhong Zhang wrote: > > On 10/25/16 17:50 -0200, Eduardo Habkost wrote: > > > On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: > > > > + if (memory) { > > > > + memory = memory ?: file_size; > > > > > > This doesn't make sense to me. You already checked if memory is > > > zero above, and now you are checking if it's zero again. > > > file_size is never going to be used here. > > > > > > > + memory_region_set_size(block->mr, memory); > > > > + memory = HOST_PAGE_ALIGN(memory); > > > > + block->used_length = memory; > > > > + block->max_length = memory; > > > > > > This is fragile: it duplicates the logic that initializes > > > used_length and max_length in qemu_ram_alloc_*(). > > > > > > Maybe it's better to keep the file-size-probing magic inside > > > hostmem-file.c, and always give a non-zero size to > > > memory_region_init_ram_from_file(). > > > > > > > Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file(). > > > > Maybe no. Two reasons: > 1) Patch 1 has some code related to file size and I'd like to put all > logic related to file size in the same function. > 2) file_ram_alloc() has the logic to open/create/close the backend file > and handle errors in this process, which also needs to move to > qemu_ram_alloc_from_file() if file-size-probing is moved. I'd argue to move the whole file creation/opening logic to hostmem-file.c. But it wouldn't be a small amount of work, so your points make sense. The plan below could work, but I would like it to get feedback from the Memory API maintainer (Paolo). > > Instead, I'd > 1) keep all logic related to file size in file_ram_alloc() > > 2) let file_ram_alloc() return the value of 'memory', e.g. > static void *file_ram_alloc(RAMBlock *block, > - ram_addr_t *memory, > + ram_addr_t memory, > const char *path, > Error **errp) > { > ... > // other usages of 'memory' are changed as well > - memory = ROUND_UP(*memory, block->page_size); + *memory = > ROUND_UP(*memory, block->page_size); > ... > } > 3) in qemu_ram_alloc_from_file(), move setting > block->used_length/max_length after calling file_ram_alloc(), > e.g. > RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, > bool share, const char *mem_path, > Error **errp) > { > ... > size = HOST_PAGE_ALIGN(size); > new_block = g_malloc0(sizeof(*new_block)); > new_block->mr = mr; > - new_block->used_length = size; > - new_block->max_length = size; > new_block->flags = share ? RAM_SHARED : 0; > - new_block->host = file_ram_alloc(new_block, size, > + new_block->host = file_ram_alloc(new_block, &size, > mem_path, errp); > if (!new_block->host) { > g_free(new_block); > return NULL; > } > + new_block->used_length = size; > + new_block->max_length = size; > ... > } >
On 26/10/2016 16:17, Eduardo Habkost wrote: > On Wed, Oct 26, 2016 at 03:49:29PM +0800, Haozhong Zhang wrote: >> On 10/26/16 13:56 +0800, Haozhong Zhang wrote: >>> On 10/25/16 17:50 -0200, Eduardo Habkost wrote: >>>> On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: >>>>> + if (memory) { >>>>> + memory = memory ?: file_size; >>>> >>>> This doesn't make sense to me. You already checked if memory is >>>> zero above, and now you are checking if it's zero again. >>>> file_size is never going to be used here. >>>> >>>>> + memory_region_set_size(block->mr, memory); >>>>> + memory = HOST_PAGE_ALIGN(memory); >>>>> + block->used_length = memory; >>>>> + block->max_length = memory; >>>> >>>> This is fragile: it duplicates the logic that initializes >>>> used_length and max_length in qemu_ram_alloc_*(). >>>> >>>> Maybe it's better to keep the file-size-probing magic inside >>>> hostmem-file.c, and always give a non-zero size to >>>> memory_region_init_ram_from_file(). >>>> >>> >>> Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file(). >>> >> >> Maybe no. Two reasons: >> 1) Patch 1 has some code related to file size and I'd like to put all >> logic related to file size in the same function. >> 2) file_ram_alloc() has the logic to open/create/close the backend file >> and handle errors in this process, which also needs to move to >> qemu_ram_alloc_from_file() if file-size-probing is moved. > > I'd argue to move the whole file creation/opening logic to > hostmem-file.c. But it wouldn't be a small amount of work, so > your points make sense. > > The plan below could work, but I would like it to get feedback > from the Memory API maintainer (Paolo). Yes, it makes sense. Paolo >> >> Instead, I'd >> 1) keep all logic related to file size in file_ram_alloc() >> >> 2) let file_ram_alloc() return the value of 'memory', e.g. >> static void *file_ram_alloc(RAMBlock *block, >> - ram_addr_t *memory, >> + ram_addr_t memory, >> const char *path, >> Error **errp) >> { >> ... >> // other usages of 'memory' are changed as well >> - memory = ROUND_UP(*memory, block->page_size); + *memory = >> ROUND_UP(*memory, block->page_size); >> ... >> } >> 3) in qemu_ram_alloc_from_file(), move setting >> block->used_length/max_length after calling file_ram_alloc(), >> e.g. >> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, >> bool share, const char *mem_path, >> Error **errp) >> { >> ... >> size = HOST_PAGE_ALIGN(size); >> new_block = g_malloc0(sizeof(*new_block)); >> new_block->mr = mr; >> - new_block->used_length = size; >> - new_block->max_length = size; >> new_block->flags = share ? RAM_SHARED : 0; >> - new_block->host = file_ram_alloc(new_block, size, >> + new_block->host = file_ram_alloc(new_block, &size, >> mem_path, errp); >> if (!new_block->host) { >> g_free(new_block); >> return NULL; >> } >> + new_block->used_length = size; >> + new_block->max_length = size; >> ... >> } >> >
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c index 42efb2f..f94d2f7 100644 --- a/backends/hostmem-file.c +++ b/backends/hostmem-file.c @@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) { HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend); - if (!backend->size) { - error_setg(errp, "can't create backend with size 0"); - return; - } if (!fb->mem_path) { error_setg(errp, "mem-path property not set"); return; @@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) g_free(path); } #endif + if (!errp && !backend->size) { + backend->size = memory_region_size(&backend->mr); + if (!backend->size) { + error_setg(errp, "can't create backend with size 0"); + } + } } static char *get_mem_path(Object *o, Error **errp) diff --git a/exec.c b/exec.c index 95983c9..91adc62 100644 --- a/exec.c +++ b/exec.c @@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block, goto error; } + if (memory) { + memory = memory ?: file_size; + memory_region_set_size(block->mr, memory); + memory = HOST_PAGE_ALIGN(memory); + block->used_length = memory; + block->max_length = memory; + } + if (memory < block->page_size) { error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " "or larger than page size 0x%zx",
If 'size' option of hostmem-file is not given, QEMU will use the file size of 'mem-path' instead. For an empty file, a non-zero size must be specified by the option 'size'. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- backends/hostmem-file.c | 10 ++++++---- exec.c | 8 ++++++++ 2 files changed, 14 insertions(+), 4 deletions(-)