[v6,11/33] hostmem-file: use whole file size if possible
diff mbox

Message ID 1446184587-142784-12-git-send-email-guangrong.xiao@linux.intel.com
State New
Headers show

Commit Message

Xiao Guangrong Oct. 30, 2015, 5:56 a.m. UTC
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(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 30, 2015, 3:27 p.m. UTC | #1
On 30.10.2015 08:56, 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>
> +
>   #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;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    if (size == -1) {
> +        size = 0;
> +    }
> +exit:
> +    close(fd);
> +    return size;
> +}
> +
>   static void
>   file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>   {
>       HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>   
> -    if (!backend->size) {
> -        error_setg(errp, "can't create backend with size 0");
> -        return;
> -    }
>       if (!fb->mem_path) {
>           error_setg(errp, "mem-path property not set");
>           return;
>       }
>   
> +    if (!backend->size) {
> +        /*
> +         * use the whole file size if @size is not specified.
> +         */
> +        backend->size = get_file_size(fb->mem_path);
> +    }
> +
> +    if (!backend->size) {
> +        error_setg(errp, "failed to get file size for %s, can't create "
> +                         "backend on it", mem_path);
> +        return;
> +    }
> +
>       backend->force_prealloc = mem_prealloc;
>       memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>                                    object_get_canonical_path(OBJECT(backend)),

You can use error_setg_errno to get an error descriptioin for free, as 
all possible errors from get_file_size comes with errno. Just zero size 
should be separated for this, for example by usiing int64_t as return 
type and -1 for an error.

Sorry for this nit-picking)
Eduardo Habkost Oct. 30, 2015, 5:30 p.m. UTC | #2
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:

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

> +
>  static void
>  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  {
>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>  
> -    if (!backend->size) {
> -        error_setg(errp, "can't create backend with size 0");
> -        return;
> -    }
>      if (!fb->mem_path) {
>          error_setg(errp, "mem-path property not set");
>          return;
>      }
>  
> +    if (!backend->size) {
> +        /*
> +         * use the whole file size if @size is not specified.
> +         */
> +        backend->size = get_file_size(fb->mem_path);
> +    }
> +
> +    if (!backend->size) {
> +        error_setg(errp, "failed to get file size for %s, can't create "
> +                         "backend on it", mem_path);
> +        return;
> +    }
> +
>      backend->force_prealloc = mem_prealloc;
>      memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>                                   object_get_canonical_path(OBJECT(backend)),
> -- 
> 1.8.3.1
> 
>
Xiao Guangrong Oct. 31, 2015, 7:59 a.m. UTC | #3
On 10/30/2015 11:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 30.10.2015 08:56, 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>
>> +
>>   #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;
>> +    }
>> +
>> +    size = lseek(fd, 0, SEEK_END);
>> +    if (size == -1) {
>> +        size = 0;
>> +    }
>> +exit:
>> +    close(fd);
>> +    return size;
>> +}
>> +
>>   static void
>>   file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>   {
>>       HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>> -    if (!backend->size) {
>> -        error_setg(errp, "can't create backend with size 0");
>> -        return;
>> -    }
>>       if (!fb->mem_path) {
>>           error_setg(errp, "mem-path property not set");
>>           return;
>>       }
>> +    if (!backend->size) {
>> +        /*
>> +         * use the whole file size if @size is not specified.
>> +         */
>> +        backend->size = get_file_size(fb->mem_path);
>> +    }
>> +
>> +    if (!backend->size) {
>> +        error_setg(errp, "failed to get file size for %s, can't create "
>> +                         "backend on it", mem_path);
>> +        return;
>> +    }
>> +
>>       backend->force_prealloc = mem_prealloc;
>>       memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>>                                    object_get_canonical_path(OBJECT(backend)),
>
> You can use error_setg_errno to get an error descriptioin for free, as all possible errors from
> get_file_size comes with errno. Just zero size should be separated for this, for example by usiing
> int64_t as return type and -1 for an error.
>

Okay.

> Sorry for this nit-picking)

Your comments/suggestion is valuable to me. :)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong Oct. 31, 2015, 8:46 a.m. UTC | #4
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...

>
>    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;
>> +    }
>> +
>> +    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. :(

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Habkost Oct. 31, 2015, 1:45 p.m. UTC | #5
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.
Xiao Guangrong Oct. 31, 2015, 4:59 p.m. UTC | #6
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.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Nov. 9, 2015, 10:17 a.m. UTC | #7
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>

Better split these simplifications off from the series.

> ---
>  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>
> +
>  #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)) {

You must test S_ISDIR too.

> +        goto exit;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    if (size == -1) {
> +        size = 0;
> +    }
> +exit:
> +    close(fd);
> +    return size;
> +}
> +
>  static void
>  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  {
>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>  
> -    if (!backend->size) {
> -        error_setg(errp, "can't create backend with size 0");
> -        return;
> -    }
>      if (!fb->mem_path) {
>          error_setg(errp, "mem-path property not set");
>          return;
>      }
>  
> +    if (!backend->size) {
> +        /*
> +         * use the whole file size if @size is not specified.
> +         */
> +        backend->size = get_file_size(fb->mem_path);
> +    }
> +
> +    if (!backend->size) {
> +        error_setg(errp, "failed to get file size for %s, can't create "
> +                         "backend on it", mem_path);
> +        return;
> +    }
> +
>      backend->force_prealloc = mem_prealloc;
>      memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>                                   object_get_canonical_path(OBJECT(backend)),
> -- 
> 1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong Nov. 9, 2015, 10:49 a.m. UTC | #8
On 11/09/2015 06:17 PM, Michael S. Tsirkin 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>
>
> Better split these simplifications off from the series.

Yep, i am also little tired to respin this long series, will drop this
one.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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>
+
 #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;
+    }
+
+    size = lseek(fd, 0, SEEK_END);
+    if (size == -1) {
+        size = 0;
+    }
+exit:
+    close(fd);
+    return size;
+}
+
 static void
 file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
 
-    if (!backend->size) {
-        error_setg(errp, "can't create backend with size 0");
-        return;
-    }
     if (!fb->mem_path) {
         error_setg(errp, "mem-path property not set");
         return;
     }
 
+    if (!backend->size) {
+        /*
+         * use the whole file size if @size is not specified.
+         */
+        backend->size = get_file_size(fb->mem_path);
+    }
+
+    if (!backend->size) {
+        error_setg(errp, "failed to get file size for %s, can't create "
+                         "backend on it", mem_path);
+        return;
+    }
+
     backend->force_prealloc = mem_prealloc;
     memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  object_get_canonical_path(OBJECT(backend)),