Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On 1/13/19 10:41 AM, Leonid Bloch wrote:
>> On 1/11/19 9:14 PM, Markus Armbruster wrote:
>>> We define 54 macros for the powers of two >= 1024. We use six, in six
>>> macro definitions. Four of them could just as well use the common MiB
>>> macro, so do that. The remaining two can't, because they get passed
>>> to stringify. Replace the macro by the literal number there.
>>> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
>>> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
>>> has been good enough for more than seven years there.
>>>
>>> This effectively reverts commit 540b8492618 and 1240ac558d3.
>>>
>>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>>> ---
>>> block/qcow2.h | 10 +++---
>>> block/vdi.c | 3 +-
>>> include/qemu/units.h | 73 --------------------------------------------
>>> 3 files changed, 7 insertions(+), 79 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index a98d24500b..2380cbfb41 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -50,11 +50,11 @@
>>>
>>> /* 8 MB refcount table is enough for 2 PB images at 64k cluster size
>>> * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
>>> -#define QCOW_MAX_REFTABLE_SIZE S_8MiB
>>> +#define QCOW_MAX_REFTABLE_SIZE (8 * MiB)
>>>
>>> /* 32 MB L1 table is enough for 2 PB images at 64k cluster size
>>> * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
>>> -#define QCOW_MAX_L1_SIZE S_32MiB
>>> +#define QCOW_MAX_L1_SIZE (32 * MiB)
>>>
>>> /* Allow for an average of 1k per snapshot table entry, should be plenty
>>> of
>>> * space for snapshot names and IDs */
>>> @@ -81,15 +81,15 @@
>>> #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
>>>
>>> #ifdef CONFIG_LINUX
>>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
>>> +#define DEFAULT_L2_CACHE_MAX_SIZE (32 * MiB)
>>> #define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
>>> #else
>>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
>>> +#define DEFAULT_L2_CACHE_MAX_SIZE (8 * MiB)
>>> /* Cache clean interval is currently available only on Linux, so must be
>>> 0 */
>>> #define DEFAULT_CACHE_CLEAN_INTERVAL 0
>>> #endif
>>>
>>> -#define DEFAULT_CLUSTER_SIZE S_64KiB
>>> +#define DEFAULT_CLUSTER_SIZE 65536
>>
>> /* Note: can't use 64 * KiB here, because it's passed to stringify() */
>
> Good idea.
>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
I omitted the comment here, because I find 64 * KiB hardly more readable
than 65536. We've done just fine with 65536 for more than seven years.
But if you guys want the comment, you can certainly have it.
>> Otherwise fine with me. The other solutions (including mine) indeed seem
>> overengineered compared to this.
>>
>> Leonid.
>>
>>>
>>> #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
>>> #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
>>> diff --git a/block/vdi.c b/block/vdi.c
>>> index 2380daa583..bf1d19dd68 100644
>>> --- a/block/vdi.c
>>> +++ b/block/vdi.c
>>> @@ -85,7 +85,8 @@
>>> #define BLOCK_OPT_STATIC "static"
>>>
>>> #define SECTOR_SIZE 512
>>> -#define DEFAULT_CLUSTER_SIZE S_1MiB
>>> +#define DEFAULT_CLUSTER_SIZE 1048576
>>> +/* Note: can't use 1 * MiB, because it's passed to stringify() */
>>>
>>> #if defined(CONFIG_VDI_DEBUG)
>>> #define VDI_DEBUG 1
>>> diff --git a/include/qemu/units.h b/include/qemu/units.h
>>> index 1c959d182e..692db3fbb2 100644
>>> --- a/include/qemu/units.h
>>> +++ b/include/qemu/units.h
>>> @@ -17,77 +17,4 @@
>>> #define PiB (INT64_C(1) << 50)
>>> #define EiB (INT64_C(1) << 60)
>>>
>>> -/*
>>> - * The following lookup table is intended to be used when a literal string
>>> of
>>> - * the number of bytes is required (for example if it needs to be
>>> stringified).
>>> - * It can also be used for generic shortcuts of power-of-two sizes.
>>> - * This table is generated using the AWK script below:
>>> - *
>>> - * BEGIN {
>>> - * suffix="KMGTPE";
>>> - * for(i=10; i<64; i++) {
>>> - * val=2**i;
>>> - * s=substr(suffix, int(i/10), 1);
>>> - * n=2**(i%10);
>>> - * pad=21-int(log(n)/log(10));
>>> - * printf("#define S_%d%siB %*d\n", n, s, pad, val);
>>> - * }
>>> - * }
>>> - */
>>> -
>>> -#define S_1KiB 1024
>>> -#define S_2KiB 2048
>>> -#define S_4KiB 4096
>>> -#define S_8KiB 8192
>>> -#define S_16KiB 16384
>>> -#define S_32KiB 32768
>>> -#define S_64KiB 65536
>>> -#define S_128KiB 131072
>>> -#define S_256KiB 262144
>>> -#define S_512KiB 524288
>>> -#define S_1MiB 1048576
>>> -#define S_2MiB 2097152
>>> -#define S_4MiB 4194304
>>> -#define S_8MiB 8388608
>>> -#define S_16MiB 16777216
>>> -#define S_32MiB 33554432
>>> -#define S_64MiB 67108864
>>> -#define S_128MiB 134217728
>>> -#define S_256MiB 268435456
>>> -#define S_512MiB 536870912
>>> -#define S_1GiB 1073741824
>>> -#define S_2GiB 2147483648
>>> -#define S_4GiB 4294967296
>>> -#define S_8GiB 8589934592
>>> -#define S_16GiB 17179869184
>>> -#define S_32GiB 34359738368
>>> -#define S_64GiB 68719476736
>>> -#define S_128GiB 137438953472
>>> -#define S_256GiB 274877906944
>>> -#define S_512GiB 549755813888
>>> -#define S_1TiB 1099511627776
>>> -#define S_2TiB 2199023255552
>>> -#define S_4TiB 4398046511104
>>> -#define S_8TiB 8796093022208
>>> -#define S_16TiB 17592186044416
>>> -#define S_32TiB 35184372088832
>>> -#define S_64TiB 70368744177664
>>> -#define S_128TiB 140737488355328
>>> -#define S_256TiB 281474976710656
>>> -#define S_512TiB 562949953421312
>>> -#define S_1PiB 1125899906842624
>>> -#define S_2PiB 2251799813685248
>>> -#define S_4PiB 4503599627370496
>>> -#define S_8PiB 9007199254740992
>>> -#define S_16PiB 18014398509481984
>>> -#define S_32PiB 36028797018963968
>>> -#define S_64PiB 72057594037927936
>>> -#define S_128PiB 144115188075855872
>>> -#define S_256PiB 288230376151711744
>>> -#define S_512PiB 576460752303423488
>>> -#define S_1EiB 1152921504606846976
>>> -#define S_2EiB 2305843009213693952
>>> -#define S_4EiB 4611686018427387904
>>> -#define S_8EiB 9223372036854775808
>>> -
>>> #endif
>>>
>
> Thanks Markus, Eric and Leonid for this cleanup!
>
> I'm sorry all of you wasted so many ressources here.
It happens :)
