Re: [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>>> ---
>>>   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é <address@hidden>

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 :)