Re: [Qemu-devel] [PATCH] qemu-img: Saner printing of large file sizes

[Richard W.M. Jones]

On Sat, Mar 30, 2019 at 10:07:02AM -0500, Eric Blake wrote:
> Disk sizes close to INT64_MAX cause overflow, for some pretty
> ridiculous output:
> 
>   $ ./nbdkit -U - memory size=$((2**63 - 512)) --run 'qemu-img info $nbd'
>   image: nbd+unix://?socket=/tmp/nbdkitHSAzNz/socket
>   file format: raw
>   virtual size: -8388607T (9223372036854775296 bytes)
>   disk size: unavailable
> 
> But there's no reason to have two separate implementations of integer
> to human-readable abbreviation, where one has overflow and stops at
> 'T', while the other avoids overflow and goes all the way to 'E'. With
> this patch, the output now claims 8EiB instead of -8388607T, which
> really is the correct rounding of largest file size supported by qemu
> (we could go 511 bytes larger if we used byte-accurate sizing instead
> of rounding up to the next sector boundary, but that wouldn't change
> the human-readable result).
> 
> Reported-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>

I tested this with a few large and a few more reasonable values and
couldn't see anything wrong, so:

Tested-by: Richard W.M. Jones <address@hidden>

Rich.

>  block/qapi.c | 49 +++++++++++--------------------------------------
>  1 file changed, 11 insertions(+), 38 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 21edab34fca..110d05dc575 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -630,43 +630,14 @@ BlockStatsList *qmp_query_blockstats(bool 
> has_query_nodes,
>      return head;
>  }
> 
> -#define NB_SUFFIXES 4
> -
> -static char *get_human_readable_size(char *buf, int buf_size, int64_t size)
> -{
> -    static const char suffixes[NB_SUFFIXES] = {'K', 'M', 'G', 'T'};
> -    int64_t base;
> -    int i;
> -
> -    if (size <= 999) {
> -        snprintf(buf, buf_size, "%" PRId64, size);
> -    } else {
> -        base = 1024;
> -        for (i = 0; i < NB_SUFFIXES; i++) {
> -            if (size < (10 * base)) {
> -                snprintf(buf, buf_size, "%0.1f%c",
> -                         (double)size / base,
> -                         suffixes[i]);
> -                break;
> -            } else if (size < (1000 * base) || i == (NB_SUFFIXES - 1)) {
> -                snprintf(buf, buf_size, "%" PRId64 "%c",
> -                         ((size + (base >> 1)) / base),
> -                         suffixes[i]);
> -                break;
> -            }
> -            base = base * 1024;
> -        }
> -    }
> -    return buf;
> -}
> -
>  void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
>                          QEMUSnapshotInfo *sn)
>  {
> -    char buf1[128], date_buf[128], clock_buf[128];
> +    char date_buf[128], clock_buf[128];
>      struct tm tm;
>      time_t ti;
>      int64_t secs;
> +    char *sizing = NULL;
> 
>      if (!sn) {
>          func_fprintf(f,
> @@ -684,14 +655,15 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, 
> void *f,
>                   (int)((secs / 60) % 60),
>                   (int)(secs % 60),
>                   (int)((sn->vm_clock_nsec / 1000000) % 1000));
> +        sizing = size_to_str(sn->vm_state_size);
>          func_fprintf(f,
>                       "%-10s%-20s%7s%20s%15s",
>                       sn->id_str, sn->name,
> -                     get_human_readable_size(buf1, sizeof(buf1),
> -                                             sn->vm_state_size),
> +                     sizing,
>                       date_buf,
>                       clock_buf);
>      }
> +    g_free(sizing);
>  }
> 
>  static void dump_qdict(fprintf_function func_fprintf, void *f, int 
> indentation,
> @@ -796,14 +768,13 @@ void bdrv_image_info_specific_dump(fprintf_function 
> func_fprintf, void *f,
>  void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>                            ImageInfo *info)
>  {
> -    char size_buf[128], dsize_buf[128];
> +    char *size_buf, *dsize_buf;
>      if (!info->has_actual_size) {
> -        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
> +        dsize_buf = g_strdup("unavailable");
>      } else {
> -        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
> -                                info->actual_size);
> +        dsize_buf = size_to_str(info->actual_size);
>      }
> -    get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
> +    size_buf = size_to_str(info->virtual_size);
>      func_fprintf(f,
>                   "image: %s\n"
>                   "file format: %s\n"
> @@ -812,6 +783,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, 
> void *f,
>                   info->filename, info->format, size_buf,
>                   info->virtual_size,
>                   dsize_buf);
> +    g_free(size_buf);
> +    g_free(dsize_buf);
> 
>      if (info->has_encrypted && info->encrypted) {
>          func_fprintf(f, "encrypted: yes\n");
> -- 
> 2.20.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html