| Message ID | 1446184587-142784-12-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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 > >
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
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
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.
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
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
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
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)),
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(-)