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