From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54314) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsZbz-0002Wf-GM for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:06:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsZbu-00047y-G3 for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:06:39 -0400 Received: from mga14.intel.com ([192.55.52.115]:30586) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsZbu-00047o-5F for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:06:34 -0400 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-12-git-send-email-guangrong.xiao@linux.intel.com> <20151030173042.GC7865@thinpad.lan.raisama.net> <56347FCD.7080609@linux.intel.com> <20151031134530.GW4180@thinpad.lan.raisama.net> From: Xiao Guangrong Message-ID: <5634F389.5070008@linux.intel.com> Date: Sun, 1 Nov 2015 00:59:53 +0800 MIME-Version: 1.0 In-Reply-To: <20151031134530.GW4180@thinpad.lan.raisama.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 11/33] hostmem-file: use whole file size if possible List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 10/31/2015 09:45 PM, Eduardo Habkost wrote: > On Sat, Oct 31, 2015 at 04:46:05PM +0800, Xiao Guangrong wrote: >> On 10/31/2015 01:30 AM, Eduardo Habkost wrote: >>> On Fri, Oct 30, 2015 at 01:56:05PM +0800, Xiao Guangrong wrote: >>>> Use the whole file size if @size is not specified which is useful >>>> if we want to directly pass a file to guest >>>> >>>> Signed-off-by: Xiao Guangrong >>>> --- >>>> backends/hostmem-file.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 44 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c >>>> index 9097a57..e1bc9ff 100644 >>>> --- a/backends/hostmem-file.c >>>> +++ b/backends/hostmem-file.c >>>> @@ -9,6 +9,9 @@ >>>> * This work is licensed under the terms of the GNU GPL, version 2 or later. >>>> * See the COPYING file in the top-level directory. >>>> */ >>>> +#include >>>> +#include >>> >>> This code needs to build on other platforms too. e.g. using mingw32: >> >> Err... You did it on Windows? It's surprised that the file is only built >> on Linux: >> common-obj-$(CONFIG_LINUX) += hostmem-file.o >> >> How it can happen... > > I did it using mingw32. I don't remember what I did, but I probably tried > something stupid to test just the build of hostmem-file.o and didn't notice it > was conditional on CONFIG_LINUX. Sorry for the noise. > No problem. :) >> >>> >>> CC backends/hostmem-file.o >>> /home/ehabkost/rh/proj/virt/qemu/backends/hostmem-file.c:12:23: fatal error: sys/ioctl.h: No such file or directory >>> #include >>> ^ >>> compilation terminated. >>> /home/ehabkost/rh/proj/virt/qemu/rules.mak:57: recipe for target 'backends/hostmem-file.o' failed >>> make: *** [backends/hostmem-file.o] Error 1 >>> >>> >>>> + >>>> #include "qemu-common.h" >>>> #include "sysemu/hostmem.h" >>>> #include "sysemu/sysemu.h" >>>> @@ -33,20 +36,57 @@ struct HostMemoryBackendFile { >>>> char *mem_path; >>>> }; >>>> >>>> +static uint64_t get_file_size(const char *file) >>>> +{ >>>> + struct stat stat_buf; >>>> + uint64_t size = 0; >>>> + int fd; >>>> + >>>> + fd = open(file, O_RDONLY); >>>> + if (fd < 0) { >>>> + return 0; >>>> + } >>>> + >>>> + if (stat(file, &stat_buf) < 0) { >>>> + goto exit; >>>> + } >>>> + >>>> + if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, &size)) { >>>> + goto exit; >>>> + } >>>> + > > I have another question: if our block device code at raw-posix.c doesn't need > the Linux-specific BLKGETSIZE64 call, why exactly do we need it in > hostmem-file.c? In which cases it would break without BLKGETSIZE64? I guess the function at raw-posix.c did not realize it will work on raw block device directly. > >>>> + size = lseek(fd, 0, SEEK_END); >>>> + if (size == -1) { >>>> + size = 0; >>>> + } >>>> +exit: >>>> + close(fd); >>>> + return size; >>>> +} >>> >>> This code seems to duplicate what block/raw-posix.c:raw_getlength() does >>> (except for the BLKGETSIZE64 part). Have you considered using the same >>> code for both? >>> >>> We can probably move all the raw-posix.c raw_getlength(BlockDriverState >>> *bs) code to fd_getlength(int fd) functions (on osdep.c?), and just >>> implement raw-posix.c:raw_getlength(s) as fd_getlength(s->fd). >>> >> >> Actually, Paolo has the same suggestion before... but >> >> | The function you pointed out is really complex - it mixed 9 platforms and each >> | platform has its own specific implementation. It is hard for us to verify the >> | change. >> | >> | I'd prefer to make it for Linux specific first then share it to other platforms >> | if it's needed in the future. >> >> I do not know if it's really worth doing it. :( > > If hostmem-file.c is Linux-specific we don't need to move or reuse the code for > all the other 9 platforms right now, that's true. But now you are adding a new > arch-specific function that does exactly the same thing in a different file to > the mix. What if somebody want to make hostmem-file.c work in another platform > in the future, and begin to duplicate the same #ifdef mess from raw-posix.c? > > I was considering this: > > 1) Move the arch-independent raw_getlength() code to fd_getlength() (at > osdep.c, maybe?), as: > > int64_t fd_getlength(int fd) > { > int64_t size; > > size = lseek(s->fd, 0, SEEK_END); > if (size < 0) { > return -errno; > } > return size; > } > > > 2) Change the arch-independent version of raw_getlength() to: > > [...] > #else > static int64_t raw_getlength(BlockDriverState *bs) > { > BDRVRawState *s = bs->opaque; > int ret; > int64_t size; > > ret = fd_open(bs); > if (ret < 0) { > return ret; > } > > return fd_getlength(s->fd); > } > #endif > > > 3) Implement get_file_size() using fd_getlength(): > > uint64_t get_file_size(const char *file, Error **errp) > { > struct stat stat_buf; > int64_t size = 0; > int fd; > > fd = open(file, O_RDONLY); > if (fd < 0) { > error_setg_errno(errp, errno, "can't open file %s", file); > return 0; > } > > size = fd_getlength(fd); > if (size < 0) { > error_setg_errno(errp, -size, "can't get size of file %s", file); > size = 0; > } > > exit: > close(fd); > return size; > } > > > 4) In case BLKGETSIZE64 is really necesary, add a Linux-specific block to > fd_getlength(): > > int64_t fd_getlength(int fd) > { > int64_t size; > > #ifdef CONFIG_LINUX > struct stat stat_buf; > if (fstat(fd, &stat_buf) < 0) { > return -errno; > } > > if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, &size)) { > if (size < 0) { > return -EOVERFLOW; > } > return size; > } > #endif > > size = lseek(fd, 0, SEEK_END); > if (size < 0) { > return -errno; > } > > return size; > } > > People working on other platforms will be able to move arch-specific code to > fd_getlength() later, if they want to. > Fair enough to me and really smart. Will do it in next version.