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