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

[Haozhong Zhang]

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 <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()).

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