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

[Paolo Bonzini]

On 26/10/2016 16:17, Eduardo Habkost wrote:
> 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).

Yes, it makes sense.

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