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

[Eduardo Habkost]

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;
>         ...
>     }
> 

-- 
Eduardo