| Message ID | 1446455617-129562-12-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02.11.2015 12:13, Xiao Guangrong wrote: > It is used to get the size of the specified file, also qemu_fd_getlength() > is introduced to unify the code with raw_getlength() in block/raw-posix.c > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > block/raw-posix.c | 7 +------ > include/qemu/osdep.h | 2 ++ > util/osdep.c | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 918c756..734e6dd 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1592,18 +1592,13 @@ 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; > } > > - size = lseek(s->fd, 0, SEEK_END); > - if (size < 0) { > - return -errno; > - } > - return size; > + return qemu_fd_getlength(s->fd); > } > #endif > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index dbc17dc..ca4c3fa 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -303,4 +303,6 @@ int qemu_read_password(char *buf, int buf_size); > pid_t qemu_fork(Error **errp); > > size_t qemu_file_get_page_size(const char *mem_path, Error **errp); > +int64_t qemu_fd_getlength(int fd); > +size_t qemu_file_getlength(const char *file, Error **errp); > #endif > diff --git a/util/osdep.c b/util/osdep.c > index 0092bb6..5a61e19 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -428,3 +428,34 @@ writev(int fd, const struct iovec *iov, int iov_cnt) > return readv_writev(fd, iov, iov_cnt, true); > } > #endif > + > +int64_t qemu_fd_getlength(int fd) > +{ > + int64_t size; > + > + size = lseek(fd, 0, SEEK_END); > + if (size < 0) { > + return -errno; > + } > + return size; > +} > + > +size_t qemu_file_getlength(const char *file, Error **errp) > +{ > + int64_t size; > + int fd = qemu_open(file, O_RDONLY); > + > + if (fd < 0) { > + error_setg_file_open(errp, errno, file); > + return 0; > + } > + > + size = qemu_fd_getlength(fd); > + if (size < 0) { > + error_setg_errno(errp, -size, "can't get size of file %s", file); > + size = 0; > + } > + > + qemu_close(fd); > + return size; > +} Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: [...] > +size_t qemu_file_getlength(const char *file, Error **errp) > +{ > + int64_t size; [...] > + return size; Can you guarantee that SIZE_MAX >= INT64_MAX on all platforms supported by QEMU?
On 11/04/2015 07:21 AM, Eduardo Habkost wrote: > On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: > [...] >> +size_t qemu_file_getlength(const char *file, Error **errp) >> +{ >> + int64_t size; > [...] >> + return size; > > Can you guarantee that SIZE_MAX >= INT64_MAX on all platforms supported > by QEMU? > Actually, this function is abstracted from the common function, raw_getlength(), in raw-posix.c whose return value is int64_t. And i think int64_t is large enough for block devices. -- 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 Wed, Nov 04, 2015 at 11:17:09AM +0800, Xiao Guangrong wrote: > > > On 11/04/2015 07:21 AM, Eduardo Habkost wrote: > >On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: > >[...] > >>+size_t qemu_file_getlength(const char *file, Error **errp) > >>+{ > >>+ int64_t size; > >[...] > >>+ return size; > > > >Can you guarantee that SIZE_MAX >= INT64_MAX on all platforms supported > >by QEMU? > > > > Actually, this function is abstracted from the common function, raw_getlength(), > in raw-posix.c whose return value is int64_t. > > And i think int64_t is large enough for block devices. int64_t should be enough, but I don't know if size_t is large enough on all platforms. I believe it's going to be either one of those cases: * If you are absolutely sure SIZE_MAX >= INT64_MAX on all platforms, please explain why (and maybe add a QEMU_BUILD_BUG_ON?). (I don't think this will be the case) * If SIZE_MAX < INT64_MAX is possible but you believe size <= SIZE_MAX is always true here, please explain why (and maybe add an assert()). * Otherwise, we need to set an appropriate error if size > SIZE_MAX or change the type of qemu_file_getlength(). What about making it uint64_t?
On 11/04/2015 10:44 PM, Eduardo Habkost wrote: > On Wed, Nov 04, 2015 at 11:17:09AM +0800, Xiao Guangrong wrote: >> >> >> On 11/04/2015 07:21 AM, Eduardo Habkost wrote: >>> On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: >>> [...] >>>> +size_t qemu_file_getlength(const char *file, Error **errp) >>>> +{ >>>> + int64_t size; >>> [...] >>>> + return size; >>> >>> Can you guarantee that SIZE_MAX >= INT64_MAX on all platforms supported >>> by QEMU? >>> >> >> Actually, this function is abstracted from the common function, raw_getlength(), >> in raw-posix.c whose return value is int64_t. >> >> And i think int64_t is large enough for block devices. > > int64_t should be enough, but I don't know if size_t is large enough on > all platforms. > > I believe it's going to be either one of those cases: > > * If you are absolutely sure SIZE_MAX >= INT64_MAX on all platforms, > please explain why (and maybe add a QEMU_BUILD_BUG_ON?). (I don't > think this will be the case) > * If SIZE_MAX < INT64_MAX is possible but you believe > size <= SIZE_MAX is always true here, please explain why (and maybe > add an assert()). > * Otherwise, we need to set an appropriate error if size > SIZE_MAX > or change the type of qemu_file_getlength(). What about making it > uint64_t? > It sounds better, I will change the return value from size_t to uint64_t. Thank you for pointing it out, Eduardo! -- 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
As this patch affects raw_getlength(), CCing the raw block driver maintainer and the qemu-block mailing list. On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: > It is used to get the size of the specified file, also qemu_fd_getlength() > is introduced to unify the code with raw_getlength() in block/raw-posix.c > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > block/raw-posix.c | 7 +------ > include/qemu/osdep.h | 2 ++ > util/osdep.c | 31 +++++++++++++++++++++++++++++++ I know I was the one who suggested osdep.c, but maybe oslib-posix.c is a more appropriate place for the new function? > 3 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 918c756..734e6dd 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1592,18 +1592,13 @@ 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; > } > > - size = lseek(s->fd, 0, SEEK_END); > - if (size < 0) { > - return -errno; > - } > - return size; > + return qemu_fd_getlength(s->fd); > } > #endif > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index dbc17dc..ca4c3fa 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -303,4 +303,6 @@ int qemu_read_password(char *buf, int buf_size); > pid_t qemu_fork(Error **errp); > > size_t qemu_file_get_page_size(const char *mem_path, Error **errp); > +int64_t qemu_fd_getlength(int fd); > +size_t qemu_file_getlength(const char *file, Error **errp); > #endif > diff --git a/util/osdep.c b/util/osdep.c > index 0092bb6..5a61e19 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -428,3 +428,34 @@ writev(int fd, const struct iovec *iov, int iov_cnt) > return readv_writev(fd, iov, iov_cnt, true); > } > #endif > + > +int64_t qemu_fd_getlength(int fd) > +{ > + int64_t size; > + > + size = lseek(fd, 0, SEEK_END); > + if (size < 0) { > + return -errno; > + } > + return size; > +} > + > +size_t qemu_file_getlength(const char *file, Error **errp) > +{ > + int64_t size; > + int fd = qemu_open(file, O_RDONLY); > + > + if (fd < 0) { > + error_setg_file_open(errp, errno, file); > + return 0; > + } > + > + size = qemu_fd_getlength(fd); > + if (size < 0) { > + error_setg_errno(errp, -size, "can't get size of file %s", file); > + size = 0; > + } > + > + qemu_close(fd); > + return size; > +} > -- > 1.8.3.1 >
On 11/06/2015 11:50 PM, Eduardo Habkost wrote: > As this patch affects raw_getlength(), CCing the raw block driver > maintainer and the qemu-block mailing list. Eduardo, thanks for your reminder. I will keep CCing Kevin and qemu-block mail list for future version. > > On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: >> It is used to get the size of the specified file, also qemu_fd_getlength() >> is introduced to unify the code with raw_getlength() in block/raw-posix.c >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> block/raw-posix.c | 7 +------ >> include/qemu/osdep.h | 2 ++ >> util/osdep.c | 31 +++++++++++++++++++++++++++++++ > > I know I was the one who suggested osdep.c, but maybe oslib-posix.c is a > more appropriate place for the new function? > Since the function we introduced here can work on both windows and posix, so i thing osdep.c is the right place. Otherwise we should implement it for multiple platforms. -- 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 Mon, Nov 09, 2015 at 12:44:55PM +0800, Xiao Guangrong wrote: > On 11/06/2015 11:50 PM, Eduardo Habkost wrote: > >As this patch affects raw_getlength(), CCing the raw block driver > >maintainer and the qemu-block mailing list. > > Eduardo, thanks for your reminder. I will keep CCing Kevin and qemu-block mail > list for future version. > > > > >On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote: > >>It is used to get the size of the specified file, also qemu_fd_getlength() > >>is introduced to unify the code with raw_getlength() in block/raw-posix.c > >> > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >>--- > >> block/raw-posix.c | 7 +------ > >> include/qemu/osdep.h | 2 ++ > >> util/osdep.c | 31 +++++++++++++++++++++++++++++++ > > > >I know I was the one who suggested osdep.c, but maybe oslib-posix.c is a > >more appropriate place for the new function? > > > > Since the function we introduced here can work on both windows and posix, so > i thing osdep.c is the right place. Otherwise we should implement it for multiple > platforms. I didn't notice it was going to be used by a platform-independent qemu_file_getlength() function in addition to the posix-specific raw_getlength(). Have you tested it on Windows, though? If you didn't test it on Windows, what about keeping qemu_file_getlength() available only on posix, by now? The only users are raw-posix.c and hostmem-file.c, currently. If in the future somebody need it on Windows, they can decide between moving the SEEK_END code to osdep.c (after testing it), or moving the existing raw-win32.c:raw_getlength() code to a oslib-win32.c:qemu_file_getlength() function.
diff --git a/block/raw-posix.c b/block/raw-posix.c index 918c756..734e6dd 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1592,18 +1592,13 @@ 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; } - size = lseek(s->fd, 0, SEEK_END); - if (size < 0) { - return -errno; - } - return size; + return qemu_fd_getlength(s->fd); } #endif diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index dbc17dc..ca4c3fa 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -303,4 +303,6 @@ int qemu_read_password(char *buf, int buf_size); pid_t qemu_fork(Error **errp); size_t qemu_file_get_page_size(const char *mem_path, Error **errp); +int64_t qemu_fd_getlength(int fd); +size_t qemu_file_getlength(const char *file, Error **errp); #endif diff --git a/util/osdep.c b/util/osdep.c index 0092bb6..5a61e19 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -428,3 +428,34 @@ writev(int fd, const struct iovec *iov, int iov_cnt) return readv_writev(fd, iov, iov_cnt, true); } #endif + +int64_t qemu_fd_getlength(int fd) +{ + int64_t size; + + size = lseek(fd, 0, SEEK_END); + if (size < 0) { + return -errno; + } + return size; +} + +size_t qemu_file_getlength(const char *file, Error **errp) +{ + int64_t size; + int fd = qemu_open(file, O_RDONLY); + + if (fd < 0) { + error_setg_file_open(errp, errno, file); + return 0; + } + + size = qemu_fd_getlength(fd); + if (size < 0) { + error_setg_errno(errp, -size, "can't get size of file %s", file); + size = 0; + } + + qemu_close(fd); + return size; +}
It is used to get the size of the specified file, also qemu_fd_getlength() is introduced to unify the code with raw_getlength() in block/raw-posix.c Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- block/raw-posix.c | 7 +------ include/qemu/osdep.h | 2 ++ util/osdep.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 6 deletions(-)