Re: [Qemu-devel] [PATCH 12/13] qemu-img: implement key management
On Thu, 2019-08-22 at 16:42 +0200, Max Reitz wrote: > On 22.08.19 13:32, Daniel P. Berrangé wrote: > > On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote: > > > On 14.08.19 22:22, Maxim Levitsky wrote: > > > > Signed-off-by: Maxim Levitsky > > > > --- > > > > block/crypto.c | 16 ++ > > > > block/crypto.h | 3 + > > > > qemu-img-cmds.hx | 13 + > > > > qemu-img.c | 140 +++ > > > > 4 files changed, 172 insertions(+) > > > > > > Yes, this seems a bit weird. Putting it under amend seems like the > > > natural thing if that works; if not, I think it should be a single > > > qemu-img subcommand instead of two. > > > > I'm not convinced by overloading two distinct operations on to one > > sub-command - doesn't seem to give an obvious benefit to overload > > them & IME experiance overloading results in harder to understand > > commands due to having distinct args to each command. > > Because it suits the qemu-img interface we currently have. For example, > we have a single subcommand for internal snapshot management (“qemu-img > snapshot”), so I think it makes sense to have a single subcommand for > encrypted image management. I personally don't care, other that I do thing that the best here is to use the amend interface. > > Max > Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 12/13] qemu-img: implement key management
On 22.08.19 13:32, Daniel P. Berrangé wrote: > On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote: >> On 14.08.19 22:22, Maxim Levitsky wrote: >>> Signed-off-by: Maxim Levitsky >>> --- >>> block/crypto.c | 16 ++ >>> block/crypto.h | 3 + >>> qemu-img-cmds.hx | 13 + >>> qemu-img.c | 140 +++ >>> 4 files changed, 172 insertions(+) >> >> Yes, this seems a bit weird. Putting it under amend seems like the >> natural thing if that works; if not, I think it should be a single >> qemu-img subcommand instead of two. > > I'm not convinced by overloading two distinct operations on to one > sub-command - doesn't seem to give an obvious benefit to overload > them & IME experiance overloading results in harder to understand > commands due to having distinct args to each command. Because it suits the qemu-img interface we currently have. For example, we have a single subcommand for internal snapshot management (“qemu-img snapshot”), so I think it makes sense to have a single subcommand for encrypted image management. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 12/13] qemu-img: implement key management
On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote: > On 14.08.19 22:22, Maxim Levitsky wrote: > > Signed-off-by: Maxim Levitsky > > --- > > block/crypto.c | 16 ++ > > block/crypto.h | 3 + > > qemu-img-cmds.hx | 13 + > > qemu-img.c | 140 +++ > > 4 files changed, 172 insertions(+) > > Yes, this seems a bit weird. Putting it under amend seems like the > natural thing if that works; if not, I think it should be a single > qemu-img subcommand instead of two. I'm not convinced by overloading two distinct operations on to one sub-command - doesn't seem to give an obvious benefit to overload them & IME experiance overloading results in harder to understand commands due to having distinct args to each command. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 12/13] qemu-img: implement key management
On Tue, 2019-08-20 at 20:29 +0200, Max Reitz wrote: > On 14.08.19 22:22, Maxim Levitsky wrote: > > Signed-off-by: Maxim Levitsky > > --- > > block/crypto.c | 16 ++ > > block/crypto.h | 3 + > > qemu-img-cmds.hx | 13 + > > qemu-img.c | 140 +++ > > 4 files changed, 172 insertions(+) > > Yes, this seems a bit weird. Putting it under amend seems like the > natural thing if that works; if not, I think it should be a single > qemu-img subcommand instead of two. > > Max > Agree, thats why RFC. Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 12/13] qemu-img: implement key management
On 14.08.19 22:22, Maxim Levitsky wrote: > Signed-off-by: Maxim Levitsky > --- > block/crypto.c | 16 ++ > block/crypto.h | 3 + > qemu-img-cmds.hx | 13 + > qemu-img.c | 140 +++ > 4 files changed, 172 insertions(+) Yes, this seems a bit weird. Putting it under amend seems like the natural thing if that works; if not, I think it should be a single qemu-img subcommand instead of two. Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH 12/13] qemu-img: implement key management
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 16 ++
block/crypto.h | 3 +
qemu-img-cmds.hx | 13 +
qemu-img.c | 140 +++
4 files changed, 172 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index 415b6db041..2fcdf9dd39 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -203,6 +203,22 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)
return ret;
}
+QCryptoEncryptionSetupOptions *
+block_crypto_setup_opts_init(QDict *opts, Error **errp)
+{
+Visitor *v;
+QCryptoEncryptionSetupOptions *ret;
+
+v = qobject_input_visitor_new_flat_confused(opts, errp);
+if (!v) {
+return NULL;
+}
+
+visit_type_QCryptoEncryptionSetupOptions(v, NULL, , errp);
+
+visit_free(v);
+return ret;
+}
static int block_crypto_open_generic(QCryptoBlockFormat format,
QemuOptsList *opts_spec,
diff --git a/block/crypto.h b/block/crypto.h
index b935695e79..ece4d64aef 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -94,4 +94,7 @@ block_crypto_create_opts_init(QDict *opts, Error **errp);
QCryptoBlockOpenOptions *
block_crypto_open_opts_init(QDict *opts, Error **errp);
+QCryptoEncryptionSetupOptions *
+block_crypto_setup_opts_init(QDict *opts, Error **errp);
+
#endif /* BLOCK_CRYPTO_H */
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1c93e6d185..7816a0adfb 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -19,6 +19,18 @@ STEXI
@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}]
[-t @var{cache}] -o @var{options} @var{filename}
ETEXI
+DEF("add_encryption_key", img_add_encryption_key,
+"add_encryption_key [--object objectdef] [--image-opts] [--force] -U
--keydef key_definition filename")
+STEXI
+@item add_encryption_key [--object @var{objectdef}] [--image-opts] [--force]
-U --keydef @var{key_definition} @var{filename}
+ETEXI
+
+DEF("erase_encryption_key", img_erase_encryption_key,
+"erase_encryption_key [--object objectdef] [--image-opts] [--force] -U
--keydef key_definition filename")
+STEXI
+@item erase_encryption_key [--object @var{objectdef}] [--image-opts] [--force]
-U --keydef @var{key_definition} @var{filename}
+ETEXI
+
DEF("bench", img_bench,
"bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval]
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S
step_size] [-t cache] [-w] [-U] filename")
STEXI
@@ -97,6 +109,7 @@ STEXI
@item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}]
[--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ |
-]@var{size}
ETEXI
+
STEXI
@end table
ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 79983772de..bc6cd60df1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -47,6 +47,7 @@
#include "block/blockjob.h"
#include "block/qapi.h"
#include "crypto/init.h"
+#include "block/crypto.h"
#include "trace/control.h"
#define QEMU_IMG_VERSION "qemu-img version " QEMU_FULL_VERSION \
@@ -70,6 +71,8 @@ enum {
OPTION_PREALLOCATION = 265,
OPTION_SHRINK = 266,
OPTION_SALVAGE = 267,
+OPTION_FORCE = 268,
+OPTION_KEYDEF = 269,
};
typedef enum OutputFormat {
@@ -223,6 +226,14 @@ static QemuOptsList qemu_source_opts = {
},
};
+static QemuOptsList keydef_opts = {
+.name = "encryption_opts",
+.head = QTAILQ_HEAD_INITIALIZER(keydef_opts.head),
+.desc = {
+{ }
+},
+};
+
static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
{
int ret = 0;
@@ -4997,6 +5008,135 @@ out:
return ret;
}
+
+static QemuOptsList keydef_options_list = {
+.name = "encryption",
+.head = QTAILQ_HEAD_INITIALIZER(keydef_options_list.head),
+.desc = {
+{ }
+},
+};
+
+static int setup_encryption(int argc, char **argv,
+enum BlkSetupEncryptionAction action)
+{
+static const struct option long_options[] = {
+{"help", no_argument, 0, 'h'},
+{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"object", required_argument, 0, OPTION_OBJECT},
+{"force", no_argument, 0, OPTION_FORCE},
+{"force-share", no_argument, 0, 'U'},
+{"keydef", required_argument, 0, OPTION_KEYDEF},
+{0, 0, 0, 0}
+};
+
+BlockBackend *blk = NULL;
+const char *filename = NULL;
+bool force_share = false;
+QemuOpts *keydef_opts = NULL;
+bool image_opts = false;
+Error *local_err = NULL;
+QDict *keydef_dict = NULL;
+QCryptoEncryptionSetupOptions *qcrypto_options = NULL;
+bool force = false;
+
+int ret = 1;
+int c;
+
+while ((c = getopt_long(argc, argv, "hU:", long_options, NULL)) != -1) {
+switch (c) {
+case '?':
+case 'h':
+help();
+break;
+case 'U':
+force_share = true;
+break;
+
+case OPTION_KEYDEF:
+if (keydef_opts) {
+error_report("Only
