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

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 10/31/16 16:18 -0200, Eduardo Habkost wrote:
>On Fri, Oct 28, 2016 at 10:06:40AM +0800, Haozhong Zhang wrote:
>[...]
>> > > 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?
>> >
>>
>> Qemu currently sets the memory region size to the file size, and block
>> length to the aligned file size, so the code here can be changed as below:
>>
>>            memory_region_set_size(block->mr, mem_size);
>>            mem_size = HOST_PAGE_ALIGN(mem_size);
>>            *memory = mem_size;
>>
>> The second line is necessary because Qemu currently passes the aligned
>> file size to file_ram_alloc().
>
>That would duplicate the existing HOST_PAGE_ALIGN logic from
>qemu_ram_alloc_from_file(), won't it?
>
>I believe that's yet another reason to check file size before
>initializing the memory region, instead of initializing it first,
>and fixing up its size later.
>

Agree. In the next version of this patch 3, I'll move file operations
(open/close/get size) from file_ram_alloc() to qemu_ram_alloc_from_file().

Thanks,
Haozhong