[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h" |
Date: |
Fri, 29 Jun 2018 11:49:51 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
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).
>
- [Qemu-trivial] [PATCH v5 00/46] Use the IEC binary prefix definitions, Philippe Mathieu-Daudé, 2018/06/25
- [Qemu-trivial] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Richard Henderson, 2018/06/27
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Igor Mammedov, 2018/06/27
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/27
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/28
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/29
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h",
Philippe Mathieu-Daudé <=
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Daniel P . Berrangé, 2018/06/29
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/29
[Qemu-trivial] [PATCH v5 02/46] vdi: Use definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
[Qemu-trivial] [PATCH v5 03/46] x86/cpu: Use definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
[Qemu-trivial] [PATCH v5 07/46] hw/ivshmem: Use the IEC binary prefix definitions, Philippe Mathieu-Daudé, 2018/06/25
[Qemu-trivial] [PATCH v5 06/46] hw: Directly use "qemu/units.h" instead of "qemu/cutils.h", Philippe Mathieu-Daudé, 2018/06/25
[Qemu-trivial] [PATCH v5 05/46] hw: Use IEC binary prefix definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25