| Message ID | 20200503184324.12506-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | LUKS: encryption slot management using amend interface | expand |
Patchew URL: https://patchew.org/QEMU/20200503184324.12506-1-mlevitsk@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200503184324.12506-1-mlevitsk@redhat.com
Subject: [PATCH v3 00/14] LUKS: encryption slot management using amend interface
Type: series
=== 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 ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5d8eee3 iotests: add tests for blockdev-amend
73b4eb1 block/qcow2: implement blockdev-amend
b636b4e block/crypto: implement blockdev-amend
989ac21 block/core: add generic infrastructure for x-blockdev-amend qmp command
905e766 iotests: qemu-img tests for luks key management
63c741f iotests: filter few more luks specific create options
7f3246a block/qcow2: extend qemu-img amend interface with crypto options
2e71af1 block/crypto: implement the encryption key management
e23e157 block/crypto: rename two functions
1d744a7 block/amend: refactor qcow2 amend options
9dfeafe block/amend: separate amend and create options for qemu-img
6abf4af block/amend: add 'force' option
70e39d2 qcrypto/luks: implement encryption key management
3e1975e qcrypto/core: add generic infrastructure for crypto options amendment
=== OUTPUT BEGIN ===
1/14 Checking commit 3e1975eea93f (qcrypto/core: add generic infrastructure for crypto options amendment)
2/14 Checking commit 70e39d22b302 (qcrypto/luks: implement encryption key management)
3/14 Checking commit 6abf4afc9174 (block/amend: add 'force' option)
4/14 Checking commit 9dfeafe0f584 (block/amend: separate amend and create options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: block/qcow2.c:5498:
+#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/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/14 Checking commit 1d744a7b0346 (block/amend: refactor qcow2 amend options)
6/14 Checking commit e23e15761eaf (block/crypto: rename two functions)
7/14 Checking commit 2e71af15aca9 (block/crypto: implement the encryption key management)
8/14 Checking commit 7f3246a67d36 (block/qcow2: extend qemu-img amend interface with crypto options)
9/14 Checking commit 63c741ff4dd1 (iotests: filter few more luks specific create options)
10/14 Checking commit 905e766ae0a7 (iotests: qemu-img tests for luks key management)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 432 lines checked
Patch 10/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/14 Checking commit 989ac217ef73 (block/core: add generic infrastructure for x-blockdev-amend qmp command)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31:
new file mode 100644
total: 0 errors, 1 warnings, 216 lines checked
Patch 11/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/14 Checking commit b636b4e630bd (block/crypto: implement blockdev-amend)
13/14 Checking commit 73b4eb12a431 (block/qcow2: implement blockdev-amend)
14/14 Checking commit 5d8eee36ada5 (iotests: add tests for blockdev-amend)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 589 lines checked
Patch 14/14 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/20200503184324.12506-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Sun, May 03, 2020 at 09:43:10PM +0300, Maxim Levitsky wrote: > Hi! > Here is the updated series of my patches, incorporating all the feedback I received. > > This implements the API interface that we agreed upon except that I merged the > LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise > I need nested unions which are not supported currently by QAPI parser. > This didn't change the API and thus once support for nested unions is there, > it can always be implemented in backward compatible way. > > I hope that this series will finally be considered for merging, since I am somewhat running > out of time to finish this task. > > Patches are strictly divided by topic to 3 groups, and each group depends on former groups. > > * Patches 1,2 implement qcrypto generic amend interface, including definition > of structs used in crypto.json and implement this in luks crypto driver > Nothing is exposed to the user at this stage > > * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management > for luks and for qcow2, and add a bunch of iotests to cover that. > > * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it > to luks and qcow2 driver to implement qmp based encryption slot management also using > the code from patches 1,2, and also add a bunch of iotests to cover this. > > Tested with -raw,-qcow2 and -luks iotests and 'make check' > > V3: rebased, addressed most of the review feedback. > For now I kept the slot bitmap code since I am not sure that replacing it will be better. I'm still of the opinion that the bitmaps code should be replaced. With this current design we are iterating over the slot of keys slots 4 times, doing different checks/logic in each iteration. This is really not nice for understanding how the code works. IMHO we should be iterating at most twice - once to validate the requested configuration, and once to apply the requested configuration. Even if there si duplication of logic in between the delete/add key codepaths, I think it will be better as the logic for each operation will be in one place. Regards, Daniel
On Mon, 2020-05-04 at 11:19 +0100, Daniel P. Berrangé wrote: > On Sun, May 03, 2020 at 09:43:10PM +0300, Maxim Levitsky wrote: > > Hi! > > Here is the updated series of my patches, incorporating all the feedback I received. > > > > This implements the API interface that we agreed upon except that I merged the > > LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise > > I need nested unions which are not supported currently by QAPI parser. > > This didn't change the API and thus once support for nested unions is there, > > it can always be implemented in backward compatible way. > > > > I hope that this series will finally be considered for merging, since I am somewhat running > > out of time to finish this task. > > > > Patches are strictly divided by topic to 3 groups, and each group depends on former groups. > > > > * Patches 1,2 implement qcrypto generic amend interface, including definition > > of structs used in crypto.json and implement this in luks crypto driver > > Nothing is exposed to the user at this stage > > > > * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management > > for luks and for qcow2, and add a bunch of iotests to cover that. > > > > * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it > > to luks and qcow2 driver to implement qmp based encryption slot management also using > > the code from patches 1,2, and also add a bunch of iotests to cover this. > > > > Tested with -raw,-qcow2 and -luks iotests and 'make check' > > > > V3: rebased, addressed most of the review feedback. > > For now I kept the slot bitmap code since I am not sure that replacing it will be better. > > I'm still of the opinion that the bitmaps code should be replaced. > With this current design we are iterating over the slot of keys slots > 4 times, doing different checks/logic in each iteration. This is really > not nice for understanding how the code works. IMHO we should be iterating > at most twice - once to validate the requested configuration, and once > to apply the requested configuration. Even if there si duplication of > logic in between the delete/add key codepaths, I think it will be better > as the logic for each operation will be in one place. > > > Regards, > Daniel All right then, I also agree kind of that these iterations aren't that nice. I just wanted to do this in the same way the spec defines it, which is currently kind of independent of how key add/remove operation. I'll send updated code soon. Best regards, Maxim Levitsky
Hi! Here is the updated series of my patches, incorporating all the feedback I received. This implements the API interface that we agreed upon except that I merged the LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise I need nested unions which are not supported currently by QAPI parser. This didn't change the API and thus once support for nested unions is there, it can always be implemented in backward compatible way. I hope that this series will finally be considered for merging, since I am somewhat running out of time to finish this task. Patches are strictly divided by topic to 3 groups, and each group depends on former groups. * Patches 1,2 implement qcrypto generic amend interface, including definition of structs used in crypto.json and implement this in luks crypto driver Nothing is exposed to the user at this stage * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management for luks and for qcow2, and add a bunch of iotests to cover that. * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it to luks and qcow2 driver to implement qmp based encryption slot management also using the code from patches 1,2, and also add a bunch of iotests to cover this. Tested with -raw,-qcow2 and -luks iotests and 'make check' V3: rebased, addressed most of the review feedback. For now I kept the slot bitmap code since I am not sure that replacing it will be better. Best regards, Maxim Levitsky clone of "luks-keymgmnt-v2" Maxim Levitsky (14): qcrypto/core: add generic infrastructure for crypto options amendment qcrypto/luks: implement encryption key management block/amend: add 'force' option block/amend: separate amend and create options for qemu-img block/amend: refactor qcow2 amend options block/crypto: rename two functions block/crypto: implement the encryption key management block/qcow2: extend qemu-img amend interface with crypto options iotests: filter few more luks specific create options iotests: qemu-img tests for luks key management block/core: add generic infrastructure for x-blockdev-amend qmp command block/crypto: implement blockdev-amend block/qcow2: implement blockdev-amend iotests: add tests for blockdev-amend block.c | 4 +- block/Makefile.objs | 2 +- block/amend.c | 108 ++++++++ block/crypto.c | 203 ++++++++++++++-- block/crypto.h | 37 +++ block/qcow2.c | 306 +++++++++++++---------- crypto/block-luks.c | 406 ++++++++++++++++++++++++++++++- crypto/block.c | 29 +++ crypto/blockpriv.h | 8 + docs/tools/qemu-img.rst | 5 +- include/block/block.h | 1 + include/block/block_int.h | 24 +- include/crypto/block.h | 22 ++ qapi/block-core.json | 68 ++++++ qapi/crypto.json | 75 +++++- qapi/job.json | 4 +- qemu-img-cmds.hx | 4 +- qemu-img.c | 44 +++- tests/qemu-iotests/049.out | 102 ++++---- tests/qemu-iotests/061.out | 12 +- tests/qemu-iotests/079.out | 18 +- tests/qemu-iotests/082.out | 176 ++++---------- tests/qemu-iotests/085.out | 38 +-- tests/qemu-iotests/087.out | 6 +- tests/qemu-iotests/115.out | 2 +- tests/qemu-iotests/121.out | 4 +- tests/qemu-iotests/125.out | 192 +++++++-------- tests/qemu-iotests/134.out | 2 +- tests/qemu-iotests/144.out | 4 +- tests/qemu-iotests/158.out | 4 +- tests/qemu-iotests/182.out | 2 +- tests/qemu-iotests/185.out | 8 +- tests/qemu-iotests/188.out | 2 +- tests/qemu-iotests/189.out | 4 +- tests/qemu-iotests/198.out | 4 +- tests/qemu-iotests/243.out | 16 +- tests/qemu-iotests/250.out | 2 +- tests/qemu-iotests/255.out | 8 +- tests/qemu-iotests/263.out | 4 +- tests/qemu-iotests/274.out | 46 ++-- tests/qemu-iotests/280.out | 2 +- tests/qemu-iotests/284.out | 6 +- tests/qemu-iotests/300 | 207 ++++++++++++++++ tests/qemu-iotests/300.out | 99 ++++++++ tests/qemu-iotests/301 | 90 +++++++ tests/qemu-iotests/301.out | 30 +++ tests/qemu-iotests/302 | 278 +++++++++++++++++++++ tests/qemu-iotests/302.out | 40 +++ tests/qemu-iotests/303 | 233 ++++++++++++++++++ tests/qemu-iotests/303.out | 33 +++ tests/qemu-iotests/common.filter | 6 +- tests/qemu-iotests/group | 5 + 52 files changed, 2503 insertions(+), 532 deletions(-) create mode 100644 block/amend.c create mode 100755 tests/qemu-iotests/300 create mode 100644 tests/qemu-iotests/300.out create mode 100755 tests/qemu-iotests/301 create mode 100644 tests/qemu-iotests/301.out create mode 100755 tests/qemu-iotests/302 create mode 100644 tests/qemu-iotests/302.out create mode 100755 tests/qemu-iotests/303 create mode 100644 tests/qemu-iotests/303.out