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

[Haozhong Zhang]

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