From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52056) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzIxy-0000Wa-7X for qemu-devel@nongnu.org; Wed, 26 Oct 2016 03:49:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzIxu-00031A-Vp for qemu-devel@nongnu.org; Wed, 26 Oct 2016 03:49:42 -0400 Received: from mga01.intel.com ([192.55.52.88]:41878) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bzIxu-00030K-Na for qemu-devel@nongnu.org; Wed, 26 Oct 2016 03:49:38 -0400 Date: Wed, 26 Oct 2016 15:49:29 +0800 From: Haozhong Zhang Message-ID: <20161026074929.rd4qn5tdrzfgpuf4@hz-desktop> References: <20161024092151.32386-1-haozhong.zhang@intel.com> <20161024092151.32386-3-haozhong.zhang@intel.com> <20161025195028.GX5057@thinpad.lan.raisama.net> <20161026055648.cjsvoyhq65qrpmnm@hz-desktop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20161026055648.cjsvoyhq65qrpmnm@hz-desktop> Subject: Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Igor Mammedov , Paolo Bonzini , Peter Crosthwaite , Richard Henderson , Xiao Guangrong , Haozhong Zhang 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; ... }