| Message ID | 20170425153858.25660-3-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/25/2017 10:38 AM, Daniel P. Berrange wrote: > When integrating the crypto support with qcow/qcow2, we don't > want to use the bare LUKS option names "hash-alg", "key-secret", > etc. We want to namespace them "luks-hash-alg", "luks-key-secret" > so that they don't clash with any general qcow options at a later > date. > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > block/crypto.c | 16 ++++++++-------- > block/crypto.h | 40 ++++++++++++++++++++-------------------- > 2 files changed, 28 insertions(+), 28 deletions(-) > > diff --git a/block/crypto.c b/block/crypto.c > index 8205bd8..7edcc49 100644 > --- a/block/crypto.c > +++ b/block/crypto.c > @@ -129,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = { > .name = "crypto", > .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head), > .desc = { > - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, > + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), Is this still needed, given your cover letter said you reworked things to use a nested struct? I'm still not convinced we need the complexity of two different prefixes if we can instead reuse a common structure.
On Wed, Apr 26, 2017 at 08:28:04AM -0500, Eric Blake wrote: > On 04/25/2017 10:38 AM, Daniel P. Berrange wrote: > > When integrating the crypto support with qcow/qcow2, we don't > > want to use the bare LUKS option names "hash-alg", "key-secret", > > etc. We want to namespace them "luks-hash-alg", "luks-key-secret" > > so that they don't clash with any general qcow options at a later > > date. > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > block/crypto.c | 16 ++++++++-------- > > block/crypto.h | 40 ++++++++++++++++++++-------------------- > > 2 files changed, 28 insertions(+), 28 deletions(-) > > > > diff --git a/block/crypto.c b/block/crypto.c > > index 8205bd8..7edcc49 100644 > > --- a/block/crypto.c > > +++ b/block/crypto.c > > @@ -129,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = { > > .name = "crypto", > > .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head), > > .desc = { > > - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, > > + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), > > Is this still needed, given your cover letter said you reworked things > to use a nested struct? I'm still not convinced we need the complexity > of two different prefixes if we can instead reuse a common structure. Yes, we still need this at the QemuOpts level. We have the general purpose luks driver that has opts directly in the top level QAPI block driver options, vs the qcow2 integration, which now has the encryption options in a nested struct/union, rather than having an option prefix in the QAPI member names. At the QemuOpts level, this mean that the option names have changed from being 'luks-key-secret', 'aes-key-secret', to be "encrypt.key-secret" So this change is about letting us provide the "encrypt." prefix for the QemuOpts. Regards, Daniel
On 04/26/2017 08:50 AM, Daniel P. Berrange wrote: > On Wed, Apr 26, 2017 at 08:28:04AM -0500, Eric Blake wrote: >> On 04/25/2017 10:38 AM, Daniel P. Berrange wrote: >>> When integrating the crypto support with qcow/qcow2, we don't >>> want to use the bare LUKS option names "hash-alg", "key-secret", >>> etc. We want to namespace them "luks-hash-alg", "luks-key-secret" >>> so that they don't clash with any general qcow options at a later >>> date. >> Is this still needed, given your cover letter said you reworked things >> to use a nested struct? I'm still not convinced we need the complexity >> of two different prefixes if we can instead reuse a common structure. > > Yes, we still need this at the QemuOpts level. We have the general > purpose luks driver that has opts directly in the top level QAPI block > driver options, vs the qcow2 integration, which now has the encryption > options in a nested struct/union, rather than having an option prefix > in the QAPI member names. Fair enough. > > At the QemuOpts level, this mean that the option names have changed > from being 'luks-key-secret', 'aes-key-secret', to be "encrypt.key-secret" But you'll want to update the commit message to match your new planned names ;)
On Wed, Apr 26, 2017 at 09:12:06AM -0500, Eric Blake wrote: > On 04/26/2017 08:50 AM, Daniel P. Berrange wrote: > > On Wed, Apr 26, 2017 at 08:28:04AM -0500, Eric Blake wrote: > >> On 04/25/2017 10:38 AM, Daniel P. Berrange wrote: > >>> When integrating the crypto support with qcow/qcow2, we don't > >>> want to use the bare LUKS option names "hash-alg", "key-secret", > >>> etc. We want to namespace them "luks-hash-alg", "luks-key-secret" > >>> so that they don't clash with any general qcow options at a later > >>> date. > > >> Is this still needed, given your cover letter said you reworked things > >> to use a nested struct? I'm still not convinced we need the complexity > >> of two different prefixes if we can instead reuse a common structure. > > > > Yes, we still need this at the QemuOpts level. We have the general > > purpose luks driver that has opts directly in the top level QAPI block > > driver options, vs the qcow2 integration, which now has the encryption > > options in a nested struct/union, rather than having an option prefix > > in the QAPI member names. > > Fair enough. > > > > > At the QemuOpts level, this mean that the option names have changed > > from being 'luks-key-secret', 'aes-key-secret', to be "encrypt.key-secret" > > But you'll want to update the commit message to match your new planned > names ;) Hah, opps :-) Regards, Daniel
diff --git a/block/crypto.c b/block/crypto.c index 8205bd8..7edcc49 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -129,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = { .name = "crypto", .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head), .desc = { - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), { /* end of list */ } }, }; @@ -144,13 +144,13 @@ static QemuOptsList block_crypto_create_opts_luks = { .type = QEMU_OPT_SIZE, .help = "Virtual disk size" }, - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, - BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG, - BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE, - BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG, - BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG, - BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG, - BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""), { /* end of list */ } }, }; diff --git a/block/crypto.h b/block/crypto.h index a4ea28a..78c1f50 100644 --- a/block/crypto.h +++ b/block/crypto.h @@ -29,51 +29,51 @@ #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg" #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time" -#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix) \ { \ - .name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, \ .type = QEMU_OPT_STRING, \ .help = "ID of the secret that provides the keyslot passphrase", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix) \ { \ - .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG, \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG, \ .type = QEMU_OPT_STRING, \ .help = "Name of encryption cipher algorithm", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE \ - { \ - .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE, \ - .type = QEMU_OPT_STRING, \ - .help = "Name of encryption cipher mode", \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE, \ + .type = QEMU_OPT_STRING, \ + .help = "Name of encryption cipher mode", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG \ - { \ - .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, \ - .type = QEMU_OPT_STRING, \ - .help = "Name of IV generator algorithm", \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, \ + .type = QEMU_OPT_STRING, \ + .help = "Name of IV generator algorithm", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(prefix) \ { \ - .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG, \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG, \ .type = QEMU_OPT_STRING, \ .help = "Name of IV generator hash algorithm", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(prefix) \ { \ - .name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG, \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_HASH_ALG, \ .type = QEMU_OPT_STRING, \ .help = "Name of encryption hash algorithm", \ } -#define BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME \ +#define BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(prefix) \ { \ - .name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME, \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_ITER_TIME, \ .type = QEMU_OPT_NUMBER, \ .help = "Time to spend in PBKDF in milliseconds", \ }