[v7,11/35] util: introduce qemu_file_getlength()
diff mbox

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

Commit Message

Xiao Guangrong Nov. 2, 2015, 9:13 a.m. UTC
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(-)

Comments

Vladimir Sementsov-Ogievskiy Nov. 2, 2015, 3:26 p.m. UTC | #1
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>
Eduardo Habkost Nov. 3, 2015, 11:21 p.m. UTC | #2
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?
Xiao Guangrong Nov. 4, 2015, 3:17 a.m. UTC | #3
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
Eduardo Habkost Nov. 4, 2015, 2:44 p.m. UTC | #4
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?
Xiao Guangrong Nov. 4, 2015, 2:44 p.m. UTC | #5
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
Eduardo Habkost Nov. 6, 2015, 3:50 p.m. UTC | #6
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
>
Xiao Guangrong Nov. 9, 2015, 4:44 a.m. UTC | #7
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
Eduardo Habkost Nov. 9, 2015, 7:21 p.m. UTC | #8
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.

Patch
diff mbox

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