Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 12:23:00PM +0800, Haozhong Zhang wrote:
> If 'size' option is not given, Qemu will use the file size of 'mem-path'
> instead. If an empty file, a non-existing file or a directory is specified
> by option 'mem-path', a non-zero option 'size' is still needed.
> 
> Signed-off-by: Haozhong Zhang <address@hidden>
> ---
>  backends/hostmem-file.c | 28 ++++++++++++++++++++--------
>  exec.c                  | 33 ++++++++++++++++++++-------------
>  2 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 42efb2f..6ee4352 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -39,17 +39,14 @@ static void
>  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  {
>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
> +    Error *local_err = NULL;
>  
> -    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;
> +        error_setg(&local_err, "mem-path property not set");
> +        goto out;
>      }
>  #ifndef CONFIG_LINUX
> -    error_setg(errp, "-mem-path not supported on this host");
> +    error_setg(&local_err, "-mem-path not supported on this host");
>  #else
>      if (!memory_region_size(&backend->mr)) {
>          gchar *path;
> @@ -58,10 +55,25 @@ file_backend_memory_alloc(HostMemoryBackend *backend, 
> Error **errp)
>          memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>                                   path,
>                                   backend->size, fb->share,
> -                                 fb->mem_path, errp);
> +                                 fb->mem_path, &local_err);
>          g_free(path);
> +
> +        if (local_err) {
> +            goto out;
> +        }
> +
> +        if (!backend->size) {
> +            backend->size = memory_region_size(&backend->mr);
> +        }
>      }
>  #endif
> +
> +    if (!backend->size) {
> +        error_setg(&local_err, "can't create backend with size 0");
> +    }

You need to move this check before the #endif line, as an error
is already unconditionally set in local_err in the !CONFIG_LINUX
path, and a second error_setg() call would trigger an assert()
inside error_setv().

> +
> + out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static char *get_mem_path(Object *o, Error **errp)
> diff --git a/exec.c b/exec.c
> index 264a25f..89065bd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
>  }
>  
>  static void *file_ram_alloc(RAMBlock *block,
> -                            ram_addr_t memory,
> +                            ram_addr_t *memory,
>                              const char *path,
>                              Error **errp)
>  {
> @@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      void *area = MAP_FAILED;
>      int fd = -1;
>      int64_t file_size;
> +    ram_addr_t mem_size = *memory;
>  
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>          error_setg(errp,
> @@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
>  
>      file_size = get_file_size(fd);
>  
> -    if (memory < block->page_size) {
> +    if (!mem_size && file_size > 0) {
> +        mem_size = file_size;

Maybe we should set *memory here and not below?

> +        memory_region_set_size(block->mr, mem_size);

This could be simplified (and made safer) if the memory region
got initialized by memory_region_init_ram_from_file() after we
already mapped/allocated the file (so we avoid surprises in case
other code does something weird because of the temporarily
zero-sized MemoryRegion). But it would probably be an intrusive
change, so I guess changing the memory size here is OK. Paolo,
what do you think?

> +    }
> +
> +    if (mem_size < block->page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>                     "or larger than page size 0x%zx",
> -                   memory, block->page_size);
> +                   mem_size, block->page_size);
>          goto error;
>      }
>  
> -    if (file_size > 0 && file_size < memory) {
> +    if (file_size > 0 && file_size < mem_size) {
>          error_setg(errp, "backing store %s size %"PRId64
>                     " does not match 'size' option %"PRIu64,
> -                   path, file_size, memory);
> +                   path, file_size, mem_size);
>          goto error;
>      }
>  
> -    memory = ROUND_UP(memory, block->page_size);
> +    mem_size = ROUND_UP(mem_size, block->page_size);
> +    *memory = mem_size;

Why exactly did you choose to set *memory to the rounded-up size
and not file_size? Changing *memory to the rounded-up value would
be additional behavior that is not described in the commit
message. I believe we should change *memory only if *memory == 0,
and avoid touching it otherwise.

>  
>      /*
>       * ftruncate is not supported by hugetlbfs in older
> @@ -1339,11 +1346,11 @@ static void *file_ram_alloc(RAMBlock *block,
>       * those labels. Therefore, extending the non-empty backend file
>       * is disabled as well.
>       */
> -    if (!file_size && ftruncate(fd, memory)) {
> +    if (!file_size && ftruncate(fd, mem_size)) {
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, block->mr->align,
> +    area = qemu_ram_mmap(fd, mem_size, block->mr->align,
>                           block->flags & RAM_SHARED);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
> @@ -1352,7 +1359,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  
>      if (mem_prealloc) {
> -        os_mem_prealloc(fd, area, memory, errp);
> +        os_mem_prealloc(fd, area, mem_size, errp);
>          if (errp && *errp) {
>              goto error;
>          }
> @@ -1363,7 +1370,7 @@ static void *file_ram_alloc(RAMBlock *block,
>  
>  error:
>      if (area != MAP_FAILED) {
> -        qemu_ram_munmap(area, memory);
> +        qemu_ram_munmap(area, mem_size);
>      }
>      if (unlink_on_error) {
>          unlink(path);
> @@ -1690,15 +1697,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
>      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;
>  
>      ram_block_add(new_block, &local_err);
>      if (local_err) {
> -- 
> 2.10.1
> 

-- 
Eduardo