| Message ID | 20180625124238.25339-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use the IEC binary prefix definitions | expand |
On 06/25/2018 05:41 AM, Philippe Mathieu-Daudé wrote: > Loosely based on 076b35b5a56. > > Suggested-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/qemu/units.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 include/qemu/units.h Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, 25 Jun 2018 09:41:53 -0300 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Loosely based on 076b35b5a56. > > Suggested-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/qemu/units.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 include/qemu/units.h > > diff --git a/include/qemu/units.h b/include/qemu/units.h > new file mode 100644 > index 0000000000..692db3fbb2 > --- /dev/null > +++ b/include/qemu/units.h > @@ -0,0 +1,20 @@ > +/* > + * IEC binary prefixes definitions > + * > + * Copyright (C) 2015 Nikunj A Dadhania, IBM Corporation > + * Copyright (C) 2018 Philippe Mathieu-Daudé <f4bug@amsat.org> > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#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() > + > +#endif
On 06/27/2018 06:27 AM, Igor Mammedov wrote: > On Mon, 25 Jun 2018 09:41:53 -0300 > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> Loosely based on 076b35b5a56. >> >> Suggested-by: Stefan Weil <sw@weilnetz.de> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> +#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).
On 06/27/2018 09:26 AM, Eric Blake wrote: > On 06/27/2018 06:27 AM, Igor Mammedov wrote: >> On Mon, 25 Jun 2018 09:41:53 -0300 >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >>> Loosely based on 076b35b5a56. >>> >>> Suggested-by: Stefan Weil <sw@weilnetz.de> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- > >>> +#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. 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); ^
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).
Hi Eric, On 06/29/2018 09:19 AM, Eric Blake wrote: > 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. Are you suggesting this? #define KiB (INT32_C(1) << 10) #define MiB (INT32_C(1) << 20) #define GiB (INT32_C(1) << 30) #define TiB (INT64_C(1) << 40) #define PiB (INT64_C(1) << 50) #define EiB (INT64_C(1) << 60) Now than I reread what Richard reviewed, I guess understand he suggested the same change: http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg03284.html > 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). >
On Fri, Jun 29, 2018 at 11:49:51AM -0300, Philippe Mathieu-Daudé wrote: > Hi Eric, > > On 06/29/2018 09:19 AM, Eric Blake wrote: > > 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. > > Are you suggesting this? > > #define KiB (INT32_C(1) << 10) > #define MiB (INT32_C(1) << 20) > #define GiB (INT32_C(1) << 30) > > #define TiB (INT64_C(1) << 40) > #define PiB (INT64_C(1) << 50) > #define EiB (INT64_C(1) << 60) This feels dangerous to me as now the calculation may or may not be 32-bit or 64-bit depending on which constant you happen to pick. I think this inconsistency is going to be surprising to both devs and reviewers leading to bugs. Did you consider just adding unsigned versions ? eg UKiB, UMiB, UGiB. It is a bit ugly but is has the benefit of being obvious whether you're intending the calculation to be signed vs unsigned. Regards, Daniel
On 06/29/2018 10:02 AM, Daniel P. Berrangé wrote: >> Are you suggesting this? >> >> #define KiB (INT32_C(1) << 10) >> #define MiB (INT32_C(1) << 20) >> #define GiB (INT32_C(1) << 30) >> >> #define TiB (INT64_C(1) << 40) >> #define PiB (INT64_C(1) << 50) >> #define EiB (INT64_C(1) << 60) Maybe, but then '8 * GiB' overflows, rather than giving a 64-bit result. > > This feels dangerous to me as now the calculation may or may not be 32-bit > or 64-bit depending on which constant you happen to pick. I think this > inconsistency is going to be surprising to both devs and reviewers leading > to bugs. Being consistently 64-bit may be annoying, but it's easy enough to reason about, compared to worrying whether an overflow will happen. > > Did you consider just adding unsigned versions ? eg UKiB, UMiB, UGiB. It > is a bit ugly but is has the benefit of being obvious whether you're intending > the calculation to be signed vs unsigned. That might also be worthwhile, although I'd tend towards the name KiBU instead if UKiB (since I'm already used to U as a suffix).
diff --git a/include/qemu/units.h b/include/qemu/units.h new file mode 100644 index 0000000000..692db3fbb2 --- /dev/null +++ b/include/qemu/units.h @@ -0,0 +1,20 @@ +/* + * IEC binary prefixes definitions + * + * Copyright (C) 2015 Nikunj A Dadhania, IBM Corporation + * Copyright (C) 2018 Philippe Mathieu-Daudé <f4bug@amsat.org> + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#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) + +#endif
Loosely based on 076b35b5a56. Suggested-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/qemu/units.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 include/qemu/units.h