Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Eduardo Habkost]

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 <address@hidden>
> ---
>  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
> 

-- 
Eduardo