Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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 > >>--- > >> 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. -- Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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 --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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
> ---
> 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
>
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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?
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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?
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 11/35] util: introduce qemu_file_getlength()
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
---
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
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 11/35] util: introduce qemu_file_getlength()
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
---
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;
+}
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
