Re: [PATCH v6 11/33] hostmem-file: use whole file size if possible

From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
To: Eduardo Habkost <ehabkost@redhat.com>
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
Subject: Re: [PATCH v6 11/33] hostmem-file: use whole file size if possible
Date: Sun, 1 Nov 2015 00:59:53 +0800	[thread overview]
Message-ID: <5634F389.5070008@linux.intel.com> (raw)
In-Reply-To: <20151031134530.GW4180@thinpad.lan.raisama.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 <guangrong.xiao@linux.intel.com>
>>>> ---
>>>>   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 <sys/ioctl.h>
>>>> +#include <linux/fs.h>
>>>
>>> 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 <sys/ioctl.h>
>>>                         ^
>>> 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.