Re: [Qemu-devel] [PATCH v2 2/7] cutils: Add bytes_to_str() to format byte values

[Kevin Wolf]

Am 13.10.2010 11:15, schrieb Markus Armbruster:
> Stefan Hajnoczi <address@hidden> writes:
> 
>> From: Anthony Liguori <address@hidden>
>>
>> This common function converts byte counts to human-readable strings with
>> proper units.
>>
>> Signed-off-by: Anthony Liguori <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  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