[Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Eric Blake]

Patches speak louder than words.  This is my counter-proposal to
Leonid's thread on how best to respresent the S_*iB macros in units.h,
where my proposal is that we don't need them at all. (hence my subject
line, even though it is completely unrelated to the series)

True, my diffstat is even bigger, but I think it is more maintainable
in the long run (if calling QemuOpts maintainable is even appropriate).

Eric Blake (6):
  qemu-option: Allow integer defaults
  block: Take advantage of QemuOpt default integers
  Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE"
  qemu: Prefer '(x * MiB)' over 'S_xiB'
  Revert "include: Add a comment to explain the origin of sizes' lookup
    table"
  Revert "include: Add a lookup table of sizes"

 block/qcow2.h         | 10 +++---
 include/qemu/option.h | 12 +++++++
 include/qemu/units.h  | 73 -------------------------------------------
 block/parallels.c     |  2 +-
 block/qcow2.c         |  2 +-
 block/qed.c           |  2 +-
 block/vdi.c           |  6 ++--
 block/vhdx.c          |  3 +-
 util/qemu-option.c    | 69 ++++++++++++++++++++++++++++++++++------
 9 files changed, 84 insertions(+), 95 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Eric Blake]

Set the framework up for declaring integer options with an integer
default, instead of our current insane approach of requiring the
default value to be given as a string (which then has to be reparsed
at every use that wants a number).  git grep '[^.]def_value_str' says
that we have done a good job of NOT abusing the internal field
outside of the implementation in qemu-option.c; therefore, it is not
too hard to audit that all code can either handle the new integer
defaults correctly or abort because a caller violated constraints.
Sadly, we DO have a constraint that qemu_opt_get() should not be
called on an option that has an integer type, because we have no
where to stash a cached const char * result; but callers that want
an integer should be using qemu_opt_get_number() and friends
anyways.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/option.h | 12 ++++++++
 util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab39..46b80d5a6e1 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
     const char *name;
     enum QemuOptType type;
     const char *help;
+    /*
+     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
+     * to a default value or leave NULL for no default.
+     *
+     * For other types: Initialize at most non-zero def_value_int or a
+     * parseable def_value_str for a default (must use a string for an
+     * explicit default of 0, although an implicit default generally
+     * works).  If setting def_value_int, calling qemu_opt_get() on
+     * that option will abort(); instead, call qemu_opt_get_del() or a
+     * typed getter.
+     */
+    uint64_t def_value_int;
     const char *def_value_str;
 } QemuOptDesc;

diff --git a/util/qemu-option.c b/util/qemu-option.c
index de42e2a406a..06c4e8102a8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -321,7 +321,8 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
     opt = qemu_opt_find(opts, name);
     if (!opt) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
+        if (desc) {
+            assert(!desc->def_value_int);
             return desc->def_value_str;
         }
     }
@@ -364,8 +365,22 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
     opt = qemu_opt_find(opts, name);
     if (!opt) {
         desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            str = g_strdup(desc->def_value_str);
+        if (desc) {
+            if (desc->def_value_str) {
+                str = g_strdup(desc->def_value_str);
+            } else if (desc->def_value_int) {
+                switch (desc->type) {
+                case QEMU_OPT_BOOL:
+                    str = g_strdup("on");
+                    break;
+                case QEMU_OPT_NUMBER:
+                case QEMU_OPT_SIZE:
+                    str = g_strdup_printf("%" PRId64, desc->def_value_int);
+                    break;
+                default:
+                    abort();
+                }
+            }
         }
         return str;
     }
@@ -400,8 +415,15 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return true;
+            }
+            if (desc->def_value_str) {
+                parse_option_bool(name, desc->def_value_str, &ret,
+                                  &error_abort);
+            }
         }
         return ret;
     }
@@ -436,8 +458,15 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_number(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return desc->def_value_int;
+            }
+            if (desc->def_value_str) {
+                parse_option_number(name, desc->def_value_str, &ret,
+                                    &error_abort);
+            }
         }
         return ret;
     }
@@ -473,8 +502,15 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_size(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return desc->def_value_int;
+            }
+            if (desc->def_value_str) {
+                parse_option_size(name, desc->def_value_str, &ret,
+                                  &error_abort);
+            }
         }
         return ret;
     }
@@ -787,9 +823,23 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
     }
     for (; desc && desc->name; desc++) {
         const char *value;
+        char *tmp = NULL;
         opt = qemu_opt_find(opts, desc->name);

         value = opt ? opt->str : desc->def_value_str;
+        if (!value && desc->def_value_int) {
+            switch (desc->type) {
+            case QEMU_OPT_BOOL:
+                value = tmp = g_strdup("on");
+                break;
+            case QEMU_OPT_NUMBER:
+            case QEMU_OPT_SIZE:
+                value = tmp = g_strdup_printf("%" PRId64, desc->def_value_int);
+                break;
+            default:
+                abort();
+            }
+        }
         if (!value) {
             continue;
         }
@@ -803,6 +853,7 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
             printf("%s%s=%s", sep, desc->name, value);
         }
         sep = separator;
+        g_free(tmp);
     }
 }

-- 
2.20.1

[Qemu-devel] [PATCH v3 2/6] block: Take advantage of QemuOpt default integers

[Eric Blake]

Instead of defining an integer to a default string value (where we
have to be careful how we spelled the integer because of the use of
stringify), populate a default integer value instead.

Drop a useless stringify(0); a missing default is just as easy to
interpret as 0 as an explicit string 0.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/parallels.c | 2 +-
 block/qcow2.c     | 2 +-
 block/qed.c       | 2 +-
 block/vdi.c       | 2 +-
 block/vhdx.c      | 3 +--
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index cc9445879d7..a7593310332 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -96,7 +96,7 @@ static QemuOptsList parallels_create_opts = {
             .name = BLOCK_OPT_CLUSTER_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "Parallels image cluster size",
-            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
+            .def_value_int = DEFAULT_CLUSTER_SIZE,
         },
         { /* end of list */ }
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 4897abae5e8..c9b76a9ea93 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4892,7 +4892,7 @@ static QemuOptsList qcow2_create_opts = {
             .name = BLOCK_OPT_CLUSTER_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "qcow2 cluster size",
-            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)
+            .def_value_int = DEFAULT_CLUSTER_SIZE,
         },
         {
             .name = BLOCK_OPT_PREALLOC,
diff --git a/block/qed.c b/block/qed.c
index 9377c0b7ad8..9db36c39066 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1651,7 +1651,7 @@ static QemuOptsList qed_create_opts = {
             .name = BLOCK_OPT_CLUSTER_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "Cluster size (in bytes)",
-            .def_value_str = stringify(QED_DEFAULT_CLUSTER_SIZE)
+            .def_value_int = QED_DEFAULT_CLUSTER_SIZE,
         },
         {
             .name = BLOCK_OPT_TABLE_SIZE,
diff --git a/block/vdi.c b/block/vdi.c
index 2380daa583e..65659c9b179 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -985,7 +985,7 @@ static QemuOptsList vdi_create_opts = {
             .name = BLOCK_OPT_CLUSTER_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "VDI cluster (block) size",
-            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)
+            .def_value_int = DEFAULT_CLUSTER_SIZE,
         },
 #endif
 #if defined(CONFIG_VDI_STATIC_IMAGE)
diff --git a/block/vhdx.c b/block/vhdx.c
index b785aef4b7b..54bf6805fc6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2085,13 +2085,12 @@ static QemuOptsList vhdx_create_opts = {
        {
            .name = VHDX_BLOCK_OPT_LOG_SIZE,
            .type = QEMU_OPT_SIZE,
-           .def_value_str = stringify(DEFAULT_LOG_SIZE),
+           .def_value_int = DEFAULT_LOG_SIZE,
            .help = "Log size; min 1MB."
        },
        {
            .name = VHDX_BLOCK_OPT_BLOCK_SIZE,
            .type = QEMU_OPT_SIZE,
-           .def_value_str = stringify(0),
            .help = "Block Size; min 1MB, max 256MB. " \
                    "0 means auto-calculate based on image size."
        },
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/6] Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE"

[Eric Blake]

This reverts commit 3dd5b8f4718c6ca1eadb16dd67a8cad76455ddb0.

Now that we can express QemuOpts values as an integer, we don't have
to be careful about how we spell our macro.
---
 block/vdi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 65659c9b179..180a45b70f6 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -85,7 +85,7 @@
 #define BLOCK_OPT_STATIC "static"

 #define SECTOR_SIZE 512
-#define DEFAULT_CLUSTER_SIZE S_1MiB
+#define DEFAULT_CLUSTER_SIZE (1 * MiB)

 #if defined(CONFIG_VDI_DEBUG)
 #define VDI_DEBUG 1
@@ -432,7 +432,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
         error_setg(errp, "unsupported VDI image (block size %" PRIu32
-                         " is not %" PRIu32 ")",
+                         " is not %" PRIu64 ")",
                    header.block_size, DEFAULT_CLUSTER_SIZE);
         ret = -ENOTSUP;
         goto fail;
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/6] qemu: Prefer '(x * MiB)' over 'S_xiB'

[Eric Blake]

Now that QemuOpts can accept true integer defaults, we no longer
have to be careful about the spelling of our default macros because
we are no longer applying stringify() to any of the macros.
Although it is slightly more verbose to call out an integer times
a scale, it avoids the need to have a list of S_*iB macros that
were used in commit b6a95c6d.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index a98d24500b2..d8db6622774 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 (64 * KiB)

 #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
 #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/6] Revert "include: Add a comment to explain the origin of sizes' lookup table"

[Eric Blake]

This reverts commit 1240ac558d348f6c7a5752b1a57c1da58e4efe3e.

The table itself is about to be reverted, now that it has no clients.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/units.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/include/qemu/units.h b/include/qemu/units.h
index 1c959d182e8..68a77586504 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,24 +17,6 @@
 #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
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/6] Revert "include: Add a lookup table of sizes"

[Eric Blake]

This reverts commit 540b8492618ebbe98e7462bd7d31361b8cb10a05.

Now that QemuOpts can accept default values as integers, there
are no more clients of the S_*iB macros, and no longer a reason
to have a list of macros that contain a specific spelling of
special values merely for the sake of stringify(), when it is
a lot nicer to just compute integers in the first place using
ordinary arithmetic and our smaller list of xiB scaling macros.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/units.h | 55 --------------------------------------------
 1 file changed, 55 deletions(-)

diff --git a/include/qemu/units.h b/include/qemu/units.h
index 68a77586504..692db3fbb26 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,59 +17,4 @@
 #define PiB     (INT64_C(1) << 50)
 #define EiB     (INT64_C(1) << 60)

-#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
-- 
2.20.1

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On 1/10/19 1:18 PM, Eric Blake wrote:
> Patches speak louder than words.  This is my counter-proposal to
> Leonid's thread on how best to respresent the S_*iB macros in units.h,
> where my proposal is that we don't need them at all. (hence my subject
> line, even though it is completely unrelated to the series)
> 
> True, my diffstat is even bigger, but I think it is more maintainable
> in the long run (if calling QemuOpts maintainable is even appropriate).

I wasn't brave enough to assert that def_value_str was used only with
QEMU_OPT_STRING, and that all uses of QEMU_OPT_{BOOL,NUMBER,SIZE} with a
default value are either defaulting to 0 or using def_value_int; that's
a bigger auditing task followup that could be given as a BiteSizedTask,
if you like this series.

Of course, if we ever get around to teaching the QAPI generators about
default values (where we could have strongly-typed defaults enforced by
the generator, rather than quacks-like-a-duck defaults in QemuOpts), and
converting more things to QAPI and away from QemuOpts, that's even
better (but an even bigger task).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 3/6] Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

On 1/10/19 1:18 PM, Eric Blake wrote:
> This reverts commit 3dd5b8f4718c6ca1eadb16dd67a8cad76455ddb0.
> 
> Now that we can express QemuOpts values as an integer, we don't have
> to be careful about how we spell our macro.

Signed-off-by: Eric Blake <eblake@redhat.com>

> ---
>  block/vdi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 65659c9b179..180a45b70f6 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -85,7 +85,7 @@
>  #define BLOCK_OPT_STATIC "static"
> 
>  #define SECTOR_SIZE 512
> -#define DEFAULT_CLUSTER_SIZE S_1MiB
> +#define DEFAULT_CLUSTER_SIZE (1 * MiB)
> 
>  #if defined(CONFIG_VDI_DEBUG)
>  #define VDI_DEBUG 1
> @@ -432,7 +432,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
>          error_setg(errp, "unsupported VDI image (block size %" PRIu32
> -                         " is not %" PRIu32 ")",
> +                         " is not %" PRIu64 ")",
>                     header.block_size, DEFAULT_CLUSTER_SIZE);
>          ret = -ENOTSUP;
>          goto fail;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On 1/10/19 1:26 PM, Eric Blake wrote:
> On 1/10/19 1:18 PM, Eric Blake wrote:
>> Patches speak louder than words.  This is my counter-proposal to
>> Leonid's thread on how best to respresent the S_*iB macros in units.h,
>> where my proposal is that we don't need them at all. (hence my subject
>> line, even though it is completely unrelated to the series)
>>
>> True, my diffstat is even bigger, but I think it is more maintainable
>> in the long run (if calling QemuOpts maintainable is even appropriate).
> 
> I wasn't brave enough to assert that def_value_str was used only with
> QEMU_OPT_STRING, and that all uses of QEMU_OPT_{BOOL,NUMBER,SIZE} with a
> default value are either defaulting to 0 or using def_value_int; that's
> a bigger auditing task followup that could be given as a BiteSizedTask,
> if you like this series.

Hmm - I wonder if Coccinelle is powerful enough to find all QemuOptDesc
initializers that .type to something other than QEMU_OPT_STRING and also
set .def_value_str; including initializers that did not use C99
named-member initialization (if we have any of those). (Those are the
candidates to start using .def_value_int)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

On 1/10/19 2:21 PM, Eric Blake wrote:
> On 1/10/19 1:26 PM, Eric Blake wrote:
>> On 1/10/19 1:18 PM, Eric Blake wrote:
>>> Patches speak louder than words.  This is my counter-proposal to
>>> Leonid's thread on how best to respresent the S_*iB macros in units.h,
>>> where my proposal is that we don't need them at all. (hence my subject
>>> line, even though it is completely unrelated to the series)
>>>
>>> True, my diffstat is even bigger, but I think it is more maintainable
>>> in the long run (if calling QemuOpts maintainable is even appropriate).
>>
>> I wasn't brave enough to assert that def_value_str was used only with
>> QEMU_OPT_STRING, and that all uses of QEMU_OPT_{BOOL,NUMBER,SIZE} with a
>> default value are either defaulting to 0 or using def_value_int; that's
>> a bigger auditing task followup that could be given as a BiteSizedTask,
>> if you like this series.
> 
> Hmm - I wonder if Coccinelle is powerful enough to find all QemuOptDesc
> initializers that .type to something other than QEMU_OPT_STRING and also
> set .def_value_str; including initializers that did not use C99
> named-member initialization (if we have any of those). (Those are the
> candidates to start using .def_value_int)

Then again, if we were consistent at C99 initializers, a grep may be
powerful enough:

$ git grep -A3 '\.type *= *QEMU_OPT_\(BOOL\|NUMBER\|SIZE\)' | grep -B3
def_value_str
--
block/parallels.c:            .type = QEMU_OPT_SIZE,
block/parallels.c-            .help = "Preallocation size on image
expansion",
block/parallels.c-            .def_value_str = "128M",
--
--
block/qcow2.c:            .type = QEMU_OPT_BOOL,
block/qcow2.c-            .help = "Postpone refcount updates",
block/qcow2.c-            .def_value_str = "off"
--
--
block/qcow2.c:            .type = QEMU_OPT_NUMBER,
block/qcow2.c-            .help = "Width of a reference count entry in
bits",
block/qcow2.c-            .def_value_str = "16"
--
--
block/vdi.c:            .type = QEMU_OPT_BOOL,
block/vdi.c-            .help = "VDI static (pre-allocated) image",
block/vdi.c-            .def_value_str = "off"
--
--
block/vmdk.c:            .type = QEMU_OPT_BOOL,
block/vmdk.c-            .help = "VMDK version 6 image",
block/vmdk.c-            .def_value_str = "off"
--
--
block/vxhs.c:            .type = QEMU_OPT_NUMBER,
block/vxhs.c-            .help = "port number on which VxHSD is
listening (default 9999)",
block/vxhs.c-            .def_value_str = "9999"


and where the def_value_str = "off" defaults can probably be deleted.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[Kevin Wolf]

Am 10.01.2019 um 20:18 hat Eric Blake geschrieben:
> Patches speak louder than words.  This is my counter-proposal to
> Leonid's thread on how best to respresent the S_*iB macros in units.h,
> where my proposal is that we don't need them at all. (hence my subject
> line, even though it is completely unrelated to the series)
> 
> True, my diffstat is even bigger, but I think it is more maintainable
> in the long run (if calling QemuOpts maintainable is even appropriate).

We're wasting too much time on this stuff. But anyway, I consider this
part of Markus' maintainership area, and if he likes to merge it, I
don't mind. I did not actually review this in detail.

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/6] block: Take advantage of QemuOpt default integers

[Kevin Wolf]

Am 10.01.2019 um 20:18 hat Eric Blake geschrieben:
> Instead of defining an integer to a default string value (where we
> have to be careful how we spelled the integer because of the use of
> stringify), populate a default integer value instead.
> 
> Drop a useless stringify(0); a missing default is just as easy to
> interpret as 0 as an explicit string 0.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

> diff --git a/block/vhdx.c b/block/vhdx.c
> index b785aef4b7b..54bf6805fc6 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2085,13 +2085,12 @@ static QemuOptsList vhdx_create_opts = {
>         {
>             .name = VHDX_BLOCK_OPT_LOG_SIZE,
>             .type = QEMU_OPT_SIZE,
> -           .def_value_str = stringify(DEFAULT_LOG_SIZE),
> +           .def_value_int = DEFAULT_LOG_SIZE,
>             .help = "Log size; min 1MB."
>         },
>         {
>             .name = VHDX_BLOCK_OPT_BLOCK_SIZE,
>             .type = QEMU_OPT_SIZE,
> -           .def_value_str = stringify(0),
>             .help = "Block Size; min 1MB, max 256MB. " \
>                     "0 means auto-calculate based on image size."
>         },

Before the patch:
$ ./qemu-img create -f vhdx /tmp/test.vhdx 64M
Formatting '/tmp/test.vhdx', fmt=vhdx size=67108864 log_size=1048576 block_size=0

After the patch:
$ ./qemu-img create -f vhdx /tmp/test.vhdx 64M
Formatting '/tmp/test.vhdx', fmt=vhdx size=67108864 log_size=1048576

Intentional?

Kevin

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Leonid Bloch]

On 1/10/19 9:18 PM, Eric Blake wrote:
> Set the framework up for declaring integer options with an integer
> default, instead of our current insane approach of requiring the
> default value to be given as a string (which then has to be reparsed
> at every use that wants a number).  git grep '[^.]def_value_str' says
> that we have done a good job of NOT abusing the internal field
> outside of the implementation in qemu-option.c; therefore, it is not
> too hard to audit that all code can either handle the new integer
> defaults correctly or abort because a caller violated constraints.
> Sadly, we DO have a constraint that qemu_opt_get() should not be
> called on an option that has an integer type, because we have no
> where to stash a cached const char * result; but callers that want
> an integer should be using qemu_opt_get_number() and friends
> anyways.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/qemu/option.h | 12 ++++++++
>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>   2 files changed, 72 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 844587cab39..46b80d5a6e1 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>       const char *name;
>       enum QemuOptType type;
>       const char *help;
> +    /*
> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
> +     * to a default value or leave NULL for no default.
> +     *
> +     * For other types: Initialize at most non-zero def_value_int or a
> +     * parseable def_value_str for a default (must use a string for an
> +     * explicit default of 0, although an implicit default generally
> +     * works).  If setting def_value_int, calling qemu_opt_get() on
> +     * that option will abort(); instead, call qemu_opt_get_del() or a
> +     * typed getter.
> +     */
> +    uint64_t def_value_int;
>       const char *def_value_str;
>   } QemuOptDesc;
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index de42e2a406a..06c4e8102a8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -321,7 +321,8 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>       opt = qemu_opt_find(opts, name);
>       if (!opt) {
>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> +        if (desc) {
> +            assert(!desc->def_value_int);
>               return desc->def_value_str;
>           }
>       }
> @@ -364,8 +365,22 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
>       opt = qemu_opt_find(opts, name);
>       if (!opt) {
>           desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> -            str = g_strdup(desc->def_value_str);
> +        if (desc) {
> +            if (desc->def_value_str) {
> +                str = g_strdup(desc->def_value_str);
> +            } else if (desc->def_value_int) {
> +                switch (desc->type) {
> +                case QEMU_OPT_BOOL:
> +                    str = g_strdup("on");
> +                    break;
> +                case QEMU_OPT_NUMBER:
> +                case QEMU_OPT_SIZE:
> +                    str = g_strdup_printf("%" PRId64, desc->def_value_int);
> +                    break;
> +                default:
> +                    abort();
> +                }
> +            }
>           }
>           return str;
>       }
> @@ -400,8 +415,15 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,
>       opt = qemu_opt_find(opts, name);
>       if (opt == NULL) {
>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> -            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return true;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_bool(name, desc->def_value_str, &ret,
> +                                  &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -436,8 +458,15 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name,
>       opt = qemu_opt_find(opts, name);
>       if (opt == NULL) {
>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> -            parse_option_number(name, desc->def_value_str, &ret, &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return desc->def_value_int;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_number(name, desc->def_value_str, &ret,
> +                                    &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -473,8 +502,15 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name,
>       opt = qemu_opt_find(opts, name);
>       if (opt == NULL) {
>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> -            parse_option_size(name, desc->def_value_str, &ret, &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return desc->def_value_int;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_size(name, desc->def_value_str, &ret,
> +                                  &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -787,9 +823,23 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
>       }
>       for (; desc && desc->name; desc++) {
>           const char *value;
> +        char *tmp = NULL;
>           opt = qemu_opt_find(opts, desc->name);
> 
>           value = opt ? opt->str : desc->def_value_str;
> +        if (!value && desc->def_value_int) {
> +            switch (desc->type) {
> +            case QEMU_OPT_BOOL:
> +                value = tmp = g_strdup("on");
> +                break;
> +            case QEMU_OPT_NUMBER:
> +            case QEMU_OPT_SIZE:
> +                value = tmp = g_strdup_printf("%" PRId64, desc->def_value_int);
> +                break;
> +            default:
> +                abort();
> +            }
> +        }
>           if (!value) {
>               continue;
>           }
> @@ -803,6 +853,7 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
>               printf("%s%s=%s", sep, desc->name, value);
>           }
>           sep = separator;
> +        g_free(tmp);
>       }
>   }
> 

If I understand correctly, the number will still be compiled into the 
binary as an expression string, but will be printed correctly during 
runtime?
If so, it still doesn't feel right to put expressions in binaries, but I 
agree that it's more elegant than my solution with the lookup table. I'd 
say the table would be more elegant if there would be more cases in 
which it's needed, but that's not the case.

The solution which Markus described as "attractively stupid" also could 
work, as there are exactly 2 places (which I know of) where it's really 
needed. Also, absolutely no changes is an option, as was mentioned before.

Leonid.

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5199 bytes --]

On 1/11/19 8:14 AM, Leonid Bloch wrote:
> On 1/10/19 9:18 PM, Eric Blake wrote:
>> Set the framework up for declaring integer options with an integer
>> default, instead of our current insane approach of requiring the
>> default value to be given as a string (which then has to be reparsed
>> at every use that wants a number).  git grep '[^.]def_value_str' says
>> that we have done a good job of NOT abusing the internal field
>> outside of the implementation in qemu-option.c; therefore, it is not
>> too hard to audit that all code can either handle the new integer
>> defaults correctly or abort because a caller violated constraints.
>> Sadly, we DO have a constraint that qemu_opt_get() should not be
>> called on an option that has an integer type, because we have no
>> where to stash a cached const char * result; but callers that want
>> an integer should be using qemu_opt_get_number() and friends
>> anyways.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/qemu/option.h | 12 ++++++++
>>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 72 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 844587cab39..46b80d5a6e1 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>>       const char *name;
>>       enum QemuOptType type;
>>       const char *help;
>> +    /*
>> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
>> +     * to a default value or leave NULL for no default.
>> +     *
>> +     * For other types: Initialize at most non-zero def_value_int or a
>> +     * parseable def_value_str for a default (must use a string for an
>> +     * explicit default of 0, although an implicit default generally
>> +     * works).  If setting def_value_int, calling qemu_opt_get() on
>> +     * that option will abort(); instead, call qemu_opt_get_del() or a
>> +     * typed getter.
>> +     */
>> +    uint64_t def_value_int;
>>       const char *def_value_str;
>>   } QemuOptDesc;
>>

> 
> If I understand correctly, the number will still be compiled into the 
> binary as an expression string, but will be printed correctly during 
> runtime?

Anyone that uses .def_value_int will compile into the binary as an
8-byte integer (regardless of how that number was spelled in C, either
as a single constant, or as a constant expression), and NOT as a decimal
string.  String conversions for code that asks for a string will happen
by a runtime printf() (ideally, such code is limited to the case of
printing out information during help output).  Code that is smart enough
to request a number now gets the default value directly, rather than the
old way of having to strtoll() decode a decimal string back into a
number.  No one should ever be using .def_value_str = stringify(macro),
when they can instead just use .def_value_int = macro (which is what the
next patch fixes).

I split the addition of def_value_int from the use in order to ease review.

> If so, it still doesn't feel right to put expressions in binaries, but I 
> agree that it's more elegant than my solution with the lookup table. I'd 
> say the table would be more elegant if there would be more cases in 
> which it's needed, but that's not the case.
> 
> The solution which Markus described as "attractively stupid" also could 
> work, as there are exactly 2 places (which I know of) where it's really 
> needed. Also, absolutely no changes is an option, as was mentioned before.


Markus' suggestion boils down to forgoing patch 1 and 2, rewriting patch
3 and 4 to use hard-coded strings in the two spots that need them (more
of a maintenance burden - if we ever change the macro's value, we have
to also remember to change the string), but keeping patch 5 and 6 as-is.
 It's a smaller diffstat, but less type-safe, and thus a slightly larger
maintenance burden (more technical debt).  My approach is to get rid of
the technical debt (use more type safety, so that we can catch bugs
sooner, and avoid the risk of duplicated values getting out of sync ifa
future patch changes a macro but not the corresponding string). I also
replied to my thread about how we can make further enhancements by even
more patches to FORCE that all integer-typed defaults use only
def_value_int, all string-typed defaults use only def_value_str, at
which point we could switch to using a union for def_value_str/int once
we are sure our code base properly follows the safe typing rules.  But
of course, doing it right costs more time up front (we're avoiding
technical debt, but the patch is bigger and has to be reviewed).  So now
it boils down to Markus' opinion on whether adding type-safe defaults to
QemuOpts is worth it for less technical debt, or if it is adding too
much burden to the fact that we already know that QemuOpts is a hack and
we want to get rid of it in favor of QAPI.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 2/6] block: Take advantage of QemuOpt default integers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2080 bytes --]

On 1/11/19 4:38 AM, Kevin Wolf wrote:
> Am 10.01.2019 um 20:18 hat Eric Blake geschrieben:
>> Instead of defining an integer to a default string value (where we
>> have to be careful how we spelled the integer because of the use of
>> stringify), populate a default integer value instead.
>>
>> Drop a useless stringify(0); a missing default is just as easy to
>> interpret as 0 as an explicit string 0.
>>

>>         {
>>             .name = VHDX_BLOCK_OPT_BLOCK_SIZE,
>>             .type = QEMU_OPT_SIZE,
>> -           .def_value_str = stringify(0),
>>             .help = "Block Size; min 1MB, max 256MB. " \
>>                     "0 means auto-calculate based on image size."
>>         },
> 
> Before the patch:
> $ ./qemu-img create -f vhdx /tmp/test.vhdx 64M
> Formatting '/tmp/test.vhdx', fmt=vhdx size=67108864 log_size=1048576 block_size=0
> 
> After the patch:
> $ ./qemu-img create -f vhdx /tmp/test.vhdx 64M
> Formatting '/tmp/test.vhdx', fmt=vhdx size=67108864 log_size=1048576
> 
> Intentional?

Yes. Well, sort of.  A default of 0 is automatic, so right now, our help
output omits all variables with an implicit default of 0.  We have no
way to tell the difference between an explicit integer default of 0 and
an implicit one (we did with strings, but the point of the series is to
store default integers as integers rather than as strings), so we could
work around that by adding even more to QemuOpts to record a bool of
whether an int of 0 should be listed as an explicit zero in our help
output.  Or we could state that listing a default of zero doesn't help
anyone.  Or we could list ALL variable defaults (even the ones that
default to implicit 0) rather than the current code that special-cases
output to omit variables left at their implicit default, but that may
change output in the other direction (more output where we are now
silent - although that may not be a bad thing).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Leonid Bloch]

On 1/11/19 6:23 PM, Eric Blake wrote:
> On 1/11/19 8:14 AM, Leonid Bloch wrote:
>> On 1/10/19 9:18 PM, Eric Blake wrote:
>>> Set the framework up for declaring integer options with an integer
>>> default, instead of our current insane approach of requiring the
>>> default value to be given as a string (which then has to be reparsed
>>> at every use that wants a number).  git grep '[^.]def_value_str' says
>>> that we have done a good job of NOT abusing the internal field
>>> outside of the implementation in qemu-option.c; therefore, it is not
>>> too hard to audit that all code can either handle the new integer
>>> defaults correctly or abort because a caller violated constraints.
>>> Sadly, we DO have a constraint that qemu_opt_get() should not be
>>> called on an option that has an integer type, because we have no
>>> where to stash a cached const char * result; but callers that want
>>> an integer should be using qemu_opt_get_number() and friends
>>> anyways.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>    include/qemu/option.h | 12 ++++++++
>>>    util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>>>    2 files changed, 72 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>>> index 844587cab39..46b80d5a6e1 100644
>>> --- a/include/qemu/option.h
>>> +++ b/include/qemu/option.h
>>> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>>>        const char *name;
>>>        enum QemuOptType type;
>>>        const char *help;
>>> +    /*
>>> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
>>> +     * to a default value or leave NULL for no default.
>>> +     *
>>> +     * For other types: Initialize at most non-zero def_value_int or a
>>> +     * parseable def_value_str for a default (must use a string for an
>>> +     * explicit default of 0, although an implicit default generally
>>> +     * works).  If setting def_value_int, calling qemu_opt_get() on
>>> +     * that option will abort(); instead, call qemu_opt_get_del() or a
>>> +     * typed getter.
>>> +     */
>>> +    uint64_t def_value_int;
>>>        const char *def_value_str;
>>>    } QemuOptDesc;
>>>
> 
>>
>> If I understand correctly, the number will still be compiled into the
>> binary as an expression string, but will be printed correctly during
>> runtime?
> 
> Anyone that uses .def_value_int will compile into the binary as an
> 8-byte integer (regardless of how that number was spelled in C, either
> as a single constant, or as a constant expression), and NOT as a decimal
> string.  String conversions for code that asks for a string will happen
> by a runtime printf() (ideally, such code is limited to the case of
> printing out information during help output).  Code that is smart enough
> to request a number now gets the default value directly, rather than the
> old way of having to strtoll() decode a decimal string back into a
> number.  No one should ever be using .def_value_str = stringify(macro),
> when they can instead just use .def_value_int = macro (which is what the
> next patch fixes).

Yes, you're right. Thanks for the explanation!

Re: [Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190110191901.5082-1-eblake@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190110191901.5082-1-eblake@redhat.com
Subject: [Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5e1a667 Revert "include: Add a lookup table of sizes"
d4c45db Revert "include: Add a comment to explain the origin of sizes' lookup table"
f1f2208 qemu: Prefer '(x * MiB)' over 'S_xiB'
79e5023 Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE"
b07329d block: Take advantage of QemuOpt default integers
b0a186c qemu-option: Allow integer defaults

=== OUTPUT BEGIN ===
1/6 Checking commit b0a186cb9fc4 (qemu-option: Allow integer defaults)
2/6 Checking commit b07329d73ed1 (block: Take advantage of QemuOpt default integers)
3/6 Checking commit 79e5023e2a1f (Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE")
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 16 lines checked

Patch 3/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/6 Checking commit f1f22081d0c0 (qemu: Prefer '(x * MiB)' over 'S_xiB')
5/6 Checking commit d4c45db902f5 (Revert "include: Add a comment to explain the origin of sizes' lookup table")
6/6 Checking commit 5e1a667a4e06 (Revert "include: Add a lookup table of sizes")
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190110191901.5082-1-eblake@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com