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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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. :(