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

[Leonid Bloch]

Hi,

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() */

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
>