Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h"

[Eric Blake]

On 06/28/2018 05:53 PM, Philippe Mathieu-Daudé wrote:

> > > > +#ifndef QEMU_UNITS_H
> > > > +#define QEMU_UNITS_H
> > > > +
> > > > +#define KiB     (INT64_C(1) << 10)
> > > > +#define MiB     (INT64_C(1) << 20)
> > > > +#define GiB     (INT64_C(1) << 30)
> > > > +#define TiB     (INT64_C(1) << 40)
> > > > +#define PiB     (INT64_C(1) << 50)
> > > > +#define EiB     (INT64_C(1) << 60)
> > > Shouldn't above use UINT64_C()
> >
> > Since the decision of signed vs. unsigned was intentional based on
> > review on earlier versions, it may be worth a comment in this file that
> > these constants are intentionally signed (in usage patterns, these tend
> > to be multiplied by another value; and while it is easy to go to
> > unsigned by doing '1U * KiB', you can't go in the opposite direction if
> > you want a signed number for '1 * KiB' unless KiB is signed).
>
> OK.

Actually, '1U * KiB' still ends up signed.  Why? Because as written, KiB 
is a 64-bit quantity, but 1U is 32-bit; type promotion says that since a 
64-bit int can represent all 32-bit unsigned values, the result of the 
expression is still signed 64-bit.

To get unsigned KiB, you either have to use '1ULL * KiB', or KiB should 
be changed to be (INT32_C(1) << 10) (a 32-bit constant, rather than a 
64-bit one).

> I'll also change this tests using your suggestion:
>
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> @@ -709,8 +709,7 @@ static void test_opts_parse_size(void)
>                              false, &error_abort);
> -    g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0),
> -                     ==, 16777215 * T_BYTE);
> +    g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215U
> * TiB);
>
> to avoid this error on 32-bit archs:
>
> source/qemu/tests/test-qemu-opts.c: In function 'test_opts_parse_size':
> source/qemu/tests/test-qemu-opts.c:713:71: error: integer overflow in
> expression [-Werror=overflow]
>    g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215 * TiB);
>                                                                         ^

And this compile error is proof of the confusion when we have a signed 
integer overflow (why it only happens on 32-bit arch and not also on 
64-bit arch is subtle - it's because TiB is 'long long' on 32-bit, but 
merely 'long' on 64-bit, which results in a different type after type 
promotion - although I still find it odd that the 64-bit compiler isn't 
warning).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org