Re: [PATCH 00/13] LUKS: encryption slot management using amend interface

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2020-01-14 at 13:17 -0800, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200114193350.10830-1-mlevitsk@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [PATCH 00/13] LUKS: encryption slot management using amend interface
> Type: series
> Message-id: 20200114193350.10830-1-mlevitsk@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> 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 ===
> 
> Switched to a new branch 'test'
> c97e00f iotests: add tests for blockdev-amend
> 005f7d8 block/qcow2: implement blockdev-amend
> fcfaaaa block/crypto: implement blockdev-amend
> 97610b5 block: add generic infrastructure for x-blockdev-amend qmp command
> b93e775 qemu-iotests: qemu-img tests for luks key management
> 9730684 iotests: filter few more luks specific create options
> f98c145 qcow2: extend qemu-img amend interface with crypto options
> dd5bc1c block/crypto: implement the encryption key management
> ad24636 block/crypto: rename two functions
> 88f372b block: amend: separate amend and create options for qemu-img
> e9720f3 block: amend: add 'force' option
> d96c666 qcrypto-luks: implement encryption key management
> c41fba3 qcrypto: add generic infrastructure for crypto options amendment
> 
> === OUTPUT BEGIN ===
> 1/13 Checking commit c41fba3b83a1 (qcrypto: add generic infrastructure for crypto options amendment)
> 2/13 Checking commit d96c6663e39d (qcrypto-luks: implement encryption key management)
> 3/13 Checking commit e9720f380038 (block: amend: add 'force' option)
> 4/13 Checking commit 88f372b238fc (block: amend: separate amend and create options for qemu-img)
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #30: FILE: block/qcow2.c:5448:
> +#define QCOW_COMMON_OPTIONS                                         \
> +    {                                                               \
> +        .name = BLOCK_OPT_SIZE,                                     \
> +        .type = QEMU_OPT_SIZE,                                      \
> +        .help = "Virtual disk size"                                 \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_COMPAT_LEVEL,                             \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Compatibility level (v2 [0.10] or v3 [1.1])"       \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_BACKING_FILE,                             \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "File name of a base image"                         \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_BACKING_FMT,                              \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Image format of the base image"                    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_DATA_FILE,                                \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "File name of an external data file"                \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_DATA_FILE_RAW,                            \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "The external data file must stay valid "           \
> +                "as a raw image"                                    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_ENCRYPT,                                  \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "Encrypt the image with format 'aes'. (Deprecated " \
> +                "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_ENCRYPT_FORMAT,                           \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Encrypt the image, format choices: 'aes', 'luks'", \
> +    },                                                              \
> +    BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",                     \
> +        "ID of secret providing qcow AES key or LUKS passphrase"),  \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."),               \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."),              \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."),                \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."),           \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."),                 \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."),                \
> +    {                                                               \
> +        .name = BLOCK_OPT_CLUSTER_SIZE,                             \
> +        .type = QEMU_OPT_SIZE,                                      \
> +        .help = "qcow2 cluster size",                               \
> +        .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)            \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_PREALLOC,                                 \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Preallocation mode (allowed values: off, "         \
> +                "metadata, falloc, full)"                           \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_LAZY_REFCOUNTS,                           \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "Postpone refcount updates",                        \
> +        .def_value_str = "off"                                      \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_REFCOUNT_BITS,                            \
> +        .type = QEMU_OPT_NUMBER,                                    \
> +        .help = "Width of a reference count entry in bits",         \
> +        .def_value_str = "16"                                       \
> +    }                                                               \
> +
> 
> total: 1 errors, 0 warnings, 231 lines checked
> 
> Patch 4/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 5/13 Checking commit ad24636acdc5 (block/crypto: rename two functions)
> 6/13 Checking commit dd5bc1cc0be5 (block/crypto: implement the encryption key management)
> ERROR: Macros with complex values should be enclosed in parenthesis
> #253: FILE: block/crypto.h:116:
> +#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE(prefix)  \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT(prefix),            \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_OLD_SECRET(prefix),         \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_NEW_SECRET(prefix),         \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(prefix)           \
> +
> 
> total: 1 errors, 0 warnings, 213 lines checked
> 
> Patch 6/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 7/13 Checking commit f98c145c0ebc (qcow2: extend qemu-img amend interface with crypto options)
> ERROR: "foo* bar" should be "foo *bar"
> #44: FILE: block/qcow2.c:4648:
> +    QDict* crypto_opts_dict;
> 
> total: 1 errors, 0 warnings, 165 lines checked
> 
> Patch 7/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 8/13 Checking commit 97306845f3fb (iotests: filter few more luks specific create options)
> 9/13 Checking commit b93e77524180 (qemu-iotests: qemu-img tests for luks key management)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #14: 
> new file mode 100755
> 
> total: 0 errors, 1 warnings, 432 lines checked
> 
> Patch 9/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 10/13 Checking commit 97610b546fea (block: add generic infrastructure for x-blockdev-amend qmp command)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #30: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 215 lines checked
> 
> Patch 10/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 11/13 Checking commit fcfaaaa88585 (block/crypto: implement blockdev-amend)
> 12/13 Checking commit 005f7d83f052 (block/qcow2: implement blockdev-amend)
> 13/13 Checking commit c97e00f6078b (iotests: add tests for blockdev-amend)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #14: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 598 lines checked
> 
> Patch 13/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20200114193350.10830-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.

This time I did throughout manual check of all patches, since checkpatch.pl doesn't catch most
of coding style mistakes I automatically do, but for commit f98c145c0ebc, I somehow missed this.

The warnings about macro style are known to me, I tried to do this similar to rest of the luks code.

Best regards,
	Maxim Levitsky


> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com