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

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>

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

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

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

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