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

[Eduardo Habkost <ehabkost@redhat.com>]

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.

> 
> >
> >   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?

> >>+    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.

-- 
Eduardo