block/qapi.c | 49 +++++++++++-------------------------------------- 1 file changed, 11 insertions(+), 38 deletions(-)
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 <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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
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 <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
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 <rjones@redhat.com>
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
On 3/30/19 11:40 AM, Richard W.M. Jones wrote: > 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 <rjones@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > 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 <rjones@redhat.com> Thanks for testing. I'll need to squash in a few iotests fixes; for example: --- /home/eblake/qemu/tests/qemu-iotests/104.out 2018-11-16 15:48:11.988526768 -0600 +++ /home/eblake/qemu/tests/qemu-iotests/104.out.bad 2019-03-30 15:47:30.487118666 -0500 @@ -4,9 +4,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 image: TEST_DIR/t.IMGFMT file format: IMGFMT -virtual size: 1.0K (1024 bytes) +virtual size: 1 KiB (1024 bytes) Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1234 image: TEST_DIR/t.IMGFMT file format: IMGFMT -virtual size: 1.5K (1536 bytes) +virtual size: 1.5 KiB (1536 bytes) ***done So it looks like I'll be sending another version for the sake of the testsuite, but with no changes to the actual code. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Sat 30 Mar 2019 04:07:02 PM CET, 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 <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
30.03.2019 18:07, 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 <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
Am 30.03.2019 um 16:07 hat Eric Blake geschrieben: > 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 <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Thanks, applied to the block-next branch for 4.1. Kevin
On 4/1/19 9:21 AM, Kevin Wolf wrote: > Am 30.03.2019 um 16:07 hat Eric Blake geschrieben: >> 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 <rjones@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Thanks, applied to the block-next branch for 4.1. You'll need v2. Coming up shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Am 30.03.2019 um 16:07 hat Eric Blake geschrieben: > 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 <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> This is quite obviously a bug fix for some cases. This suggests that we want it in 4.0. It is also an output change for other cases, like going from "8M" to "8 MiB". We probably can't tell for sure whether some tools expect the spelling "8M" (even if this is supposed to be the human interface and tools should be using JSON) or feed the change back to qemu-img or qemu-io (which accept "8M", but not "8 MiB" as sizes in most places). This suggests that we shouldn't make this change as late as -rc2. So what is the conclusion? Kevin
On Mon, Apr 01, 2019 at 03:04:16PM +0200, Kevin Wolf wrote: > Am 30.03.2019 um 16:07 hat Eric Blake geschrieben: > > 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 <rjones@redhat.com> > > Signed-off-by: Eric Blake <eblake@redhat.com> > > This is quite obviously a bug fix for some cases. This suggests that we > want it in 4.0. > > It is also an output change for other cases, like going from "8M" to > "8 MiB". We probably can't tell for sure whether some tools expect the > spelling "8M" (even if this is supposed to be the human interface and > tools should be using JSON) or feed the change back to qemu-img or > qemu-io (which accept "8M", but not "8 MiB" as sizes in most places). > This suggests that we shouldn't make this change as late as -rc2. If it breaks our own tests, then it is possible to break other tools too. > So what is the conclusion? The safe option is to do the minimal fix for the existing code and look at the refactoring in the next dev cycle. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 4/1/19 8:16 AM, Daniel P. Berrangé wrote: > On Mon, Apr 01, 2019 at 03:04:16PM +0200, Kevin Wolf wrote: >> Am 30.03.2019 um 16:07 hat Eric Blake geschrieben: >>> 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 >>> >> This is quite obviously a bug fix for some cases. This suggests that we >> want it in 4.0. >> >> It is also an output change for other cases, like going from "8M" to >> "8 MiB". We probably can't tell for sure whether some tools expect the >> spelling "8M" (even if this is supposed to be the human interface and >> tools should be using JSON) or feed the change back to qemu-img or >> qemu-io (which accept "8M", but not "8 MiB" as sizes in most places). >> This suggests that we shouldn't make this change as late as -rc2. > > If it breaks our own tests, then it is possible to break other tools > too. > >> So what is the conclusion? > > The safe option is to do the minimal fix for the existing code and look at > the refactoring in the next dev cycle. It's not worth fixing the second implementation to match the first in terms of support for sizes > T with no overflow, but with the existing format of 8.0M vs. 8 MiB. I'm happy to defer this patch to 4.1; the bug is not new to 4.0, so living with it for one more release doesn't hurt. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.