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