Patchwork [v2,2/7] cutils: Add bytes_to_str() to format byte values

login
register
mail settings
Submitter Stefan Hajnoczi
Date Oct. 8, 2010, 3:48 p.m.
Message ID <1286552914-27014-3-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/67257/
State New
Headers show

Comments

Stefan Hajnoczi - Oct. 8, 2010, 3:48 p.m.
From: Anthony Liguori <aliguori@us.ibm.com>

This common function converts byte counts to human-readable strings with
proper units.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 cutils.c      |   15 +++++++++++++++
 qemu-common.h |    1 +
 2 files changed, 16 insertions(+), 0 deletions(-)
Kevin Wolf - Oct. 11, 2010, 11:09 a.m.
Am 08.10.2010 17:48, schrieb Stefan Hajnoczi:
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> This common function converts byte counts to human-readable strings with
> proper units.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

I wonder how many similar implementations we have in the tree. I know at
least of cvtstr() in cmd.c, which is used by qemu-io, but I would be
surprised if it was the only one.

Adding such a function to cutils.c sounds like a good idea, but we
should probably try to change everything to use this common function.

Kevin
Markus Armbruster - Oct. 13, 2010, 9:15 a.m.
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:

> From: Anthony Liguori <aliguori@us.ibm.com>
>
> This common function converts byte counts to human-readable strings with
> proper units.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  cutils.c      |   15 +++++++++++++++
>  qemu-common.h |    1 +
>  2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 6c32198..5041203 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -301,3 +301,18 @@ int get_bits_from_size(size_t size)
>      return __builtin_ctzl(size);
>  #endif
>  }
> +
> +void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size)

Why is the size argument uint64_t and not size_t?

The name bytes_to_str() suggests you're formatting a sequence of bytes.
What about sztostr()?  Matches Jes's strtosz().

> +{
> +    if (size < (1ULL << 10)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " byte(s)", size);
> +    } else if (size < (1ULL << 20)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " KB(s)", size >> 10);
> +    } else if (size < (1ULL << 30)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " MB(s)", size >> 20);
> +    } else if (size < (1ULL << 40)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " GB(s)", size >> 30);
> +    } else {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " TB(s)", size >> 40);
> +    }

Sure you want to truncate rather than round?

The "(s)" sure are ugly.  We don't usually add plural-s after a unit: we
write ten milliseconds as 10ms, not 10ms(s).

Suggest to return the length of the resulting string, as returned by
snprintf().

> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index e0ca398..80ae834 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -154,6 +154,7 @@ int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
>  int get_bits_from_size(size_t size);
> +void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
Kevin Wolf - Oct. 13, 2010, 9:28 a.m.
Am 13.10.2010 11:15, schrieb Markus Armbruster:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:
> 
>> From: Anthony Liguori <aliguori@us.ibm.com>
>>
>> This common function converts byte counts to human-readable strings with
>> proper units.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  cutils.c      |   15 +++++++++++++++
>>  qemu-common.h |    1 +
>>  2 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/cutils.c b/cutils.c
>> index 6c32198..5041203 100644
>> --- a/cutils.c
>> +++ b/cutils.c
>> @@ -301,3 +301,18 @@ int get_bits_from_size(size_t size)
>>      return __builtin_ctzl(size);
>>  #endif
>>  }
>> +
>> +void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size)
> 
> Why is the size argument uint64_t and not size_t?

size_t would be rather small for disk images on 32 bit hosts.

> The name bytes_to_str() suggests you're formatting a sequence of bytes.
> What about sztostr()?  Matches Jes's strtosz().
> 
>> +{
>> +    if (size < (1ULL << 10)) {
>> +        snprintf(buffer, buffer_len, "%" PRIu64 " byte(s)", size);
>> +    } else if (size < (1ULL << 20)) {
>> +        snprintf(buffer, buffer_len, "%" PRIu64 " KB(s)", size >> 10);
>> +    } else if (size < (1ULL << 30)) {
>> +        snprintf(buffer, buffer_len, "%" PRIu64 " MB(s)", size >> 20);
>> +    } else if (size < (1ULL << 40)) {
>> +        snprintf(buffer, buffer_len, "%" PRIu64 " GB(s)", size >> 30);
>> +    } else {
>> +        snprintf(buffer, buffer_len, "%" PRIu64 " TB(s)", size >> 40);
>> +    }
> 
> Sure you want to truncate rather than round?
> 
> The "(s)" sure are ugly.  We don't usually add plural-s after a unit: we
> write ten milliseconds as 10ms, not 10ms(s).

I suggest taking the output format from cvtstr in cmd.c, so that qemu-io
output stays the same when switching to a common function (it says "10
KiB" and "1 bytes").

Kevin
Avi Kivity - Oct. 13, 2010, 10:25 a.m.
On 10/08/2010 05:48 PM, Stefan Hajnoczi wrote:
> From: Anthony Liguori<aliguori@us.ibm.com>
>
> This common function converts byte counts to human-readable strings with
> proper units.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
> ---
>   cutils.c      |   15 +++++++++++++++
>   qemu-common.h |    1 +
>   2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 6c32198..5041203 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -301,3 +301,18 @@ int get_bits_from_size(size_t size)
>       return __builtin_ctzl(size);
>   #endif
>   }
> +
> +void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size)
> +{
> +    if (size<  (1ULL<<  10)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " byte(s)", size);
> +    } else if (size<  (1ULL<<  20)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " KB(s)", size>>  10);
> +    } else if (size<  (1ULL<<  30)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " MB(s)", size>>  20);
> +    } else if (size<  (1ULL<<  40)) {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " GB(s)", size>>  30);
> +    } else {
> +        snprintf(buffer, buffer_len, "%" PRIu64 " TB(s)", size>>  40);
> +    }
> +}

This will show 1.5GB as 1GB.  Need either floating point with a couple 
of digits of precision, or show 1.5GB as 1500MB.

It also misuses SI prefixes. 1 GB means 10^9 bytes, not 2^30 bytes (with 
a common exception for RAM which is usually packaged in 1.125 times some 
power of two).
Stefan Hajnoczi - Oct. 13, 2010, 10:58 a.m.
On Wed, Oct 13, 2010 at 11:28:42AM +0200, Kevin Wolf wrote:
> Am 13.10.2010 11:15, schrieb Markus Armbruster:
> > Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:
> > 
> >> From: Anthony Liguori <aliguori@us.ibm.com>
> >>
> >> This common function converts byte counts to human-readable strings with
> >> proper units.
> >>
> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> >> ---
> >>  cutils.c      |   15 +++++++++++++++
> >>  qemu-common.h |    1 +
> >>  2 files changed, 16 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/cutils.c b/cutils.c
> >> index 6c32198..5041203 100644
> >> --- a/cutils.c
> >> +++ b/cutils.c
> >> @@ -301,3 +301,18 @@ int get_bits_from_size(size_t size)
> >>      return __builtin_ctzl(size);
> >>  #endif
> >>  }
> >> +
> >> +void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size)
> > 
> > Why is the size argument uint64_t and not size_t?
> 
> size_t would be rather small for disk images on 32 bit hosts.
> 
> > The name bytes_to_str() suggests you're formatting a sequence of bytes.
> > What about sztostr()?  Matches Jes's strtosz().
> > 
> >> +{
> >> +    if (size < (1ULL << 10)) {
> >> +        snprintf(buffer, buffer_len, "%" PRIu64 " byte(s)", size);
> >> +    } else if (size < (1ULL << 20)) {
> >> +        snprintf(buffer, buffer_len, "%" PRIu64 " KB(s)", size >> 10);
> >> +    } else if (size < (1ULL << 30)) {
> >> +        snprintf(buffer, buffer_len, "%" PRIu64 " MB(s)", size >> 20);
> >> +    } else if (size < (1ULL << 40)) {
> >> +        snprintf(buffer, buffer_len, "%" PRIu64 " GB(s)", size >> 30);
> >> +    } else {
> >> +        snprintf(buffer, buffer_len, "%" PRIu64 " TB(s)", size >> 40);
> >> +    }
> > 
> > Sure you want to truncate rather than round?
> > 
> > The "(s)" sure are ugly.  We don't usually add plural-s after a unit: we
> > write ten milliseconds as 10ms, not 10ms(s).
> 
> I suggest taking the output format from cvtstr in cmd.c, so that qemu-io
> output stays the same when switching to a common function (it says "10
> KiB" and "1 bytes").

I have a patch to replace bytes_to_str() with cvtstr().  We should
probably rename cvtstr() to sztostr() as suggested because that name is
more descriptive.

Stefan

Patch

diff --git a/cutils.c b/cutils.c
index 6c32198..5041203 100644
--- a/cutils.c
+++ b/cutils.c
@@ -301,3 +301,18 @@  int get_bits_from_size(size_t size)
     return __builtin_ctzl(size);
 #endif
 }
+
+void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size)
+{
+    if (size < (1ULL << 10)) {
+        snprintf(buffer, buffer_len, "%" PRIu64 " byte(s)", size);
+    } else if (size < (1ULL << 20)) {
+        snprintf(buffer, buffer_len, "%" PRIu64 " KB(s)", size >> 10);
+    } else if (size < (1ULL << 30)) {
+        snprintf(buffer, buffer_len, "%" PRIu64 " MB(s)", size >> 20);
+    } else if (size < (1ULL << 40)) {
+        snprintf(buffer, buffer_len, "%" PRIu64 " GB(s)", size >> 30);
+    } else {
+        snprintf(buffer, buffer_len, "%" PRIu64 " TB(s)", size >> 40);
+    }
+}
diff --git a/qemu-common.h b/qemu-common.h
index e0ca398..80ae834 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -154,6 +154,7 @@  int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
 int get_bits_from_size(size_t size);
+void bytes_to_str(char *buffer, size_t buffer_len, uint64_t size);
 
 /* path.c */
 void init_paths(const char *prefix);