Re: [Qemu-devel] [PATCH v1 02/15] block: add ability to set a prefix for opt names

2017-01-24 Thread Daniel P. Berrange
On Mon, Jan 16, 2017 at 08:31:55PM +0100, Max Reitz wrote:
> On 03.01.2017 19:27, 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.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/crypto.c | 110 
> > +
> >  block/crypto.h |  42 +++---
> >  2 files changed, 118 insertions(+), 34 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index d281de6..1037c70 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> 
> [...]
> 
> > +static int block_crypto_copy_value(void *opaque, const char *name,
> > +   const char *value, Error **errp)
> > +{
> > +struct BlockCryptoCopyData *data = opaque;
> > +
> > +if (g_str_has_prefix(name, data->prefix)) {
> > +Error *local_err = NULL;
> > +const char *newname = name + strlen(data->prefix);
> 
> strstart() would be shorter:
> 
> const char *newname;
> 
> if (strstart(name, data->prefix, &newname)) {
> /* ... */
> }

Ah, didn't know that function existed.

> 
> > +
> > +qemu_opt_set(data->opts, newname, value, &local_err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +return 1;
> 
> I'd prefer -1, because 0/1 looks more like false/true to me, which in
> turn looks like failure/success.

Yes, that makes more sense.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|



Re: [Qemu-devel] [PATCH v1 02/15] block: add ability to set a prefix for opt names

2017-01-16 Thread Max Reitz
On 03.01.2017 19:27, 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.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/crypto.c | 110 
> +
>  block/crypto.h |  42 +++---
>  2 files changed, 118 insertions(+), 34 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index d281de6..1037c70 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c

[...]

> +static int block_crypto_copy_value(void *opaque, const char *name,
> +   const char *value, Error **errp)
> +{
> +struct BlockCryptoCopyData *data = opaque;
> +
> +if (g_str_has_prefix(name, data->prefix)) {
> +Error *local_err = NULL;
> +const char *newname = name + strlen(data->prefix);

strstart() would be shorter:

const char *newname;

if (strstart(name, data->prefix, &newname)) {
/* ... */
}

> +
> +qemu_opt_set(data->opts, newname, value, &local_err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return 1;

I'd prefer -1, because 0/1 looks more like false/true to me, which in
turn looks like failure/success.

Both optional suggestions, so either way:

Reviewed-by: Max Reitz 

> +}
> +}
> +
> +return 0;
> +}

[...]




signature.asc
Description: OpenPGP digital signature


[Qemu-devel] [PATCH v1 02/15] block: add ability to set a prefix for opt names

2017-01-03 Thread Daniel P. Berrange
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.

Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 110 +
 block/crypto.h |  42 +++---
 2 files changed, 118 insertions(+), 34 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index d281de6..1037c70 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -128,7 +128,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 */ }
 },
 };
@@ -143,31 +143,101 @@ 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 */ }
 },
 };
 
+static QemuOptsList empty_opts = {
+.name = "crypto-empty",
+.merge_lists = false,
+.head = QTAILQ_HEAD_INITIALIZER(empty_opts.head),
+.desc = {
+/* no elements => accept any params */
+{ /* end of list */ }
+},
+};
+
+
+struct BlockCryptoCopyData {
+QemuOpts *opts;
+const char *prefix;
+};
+
+static int block_crypto_copy_value(void *opaque, const char *name,
+   const char *value, Error **errp)
+{
+struct BlockCryptoCopyData *data = opaque;
+
+if (g_str_has_prefix(name, data->prefix)) {
+Error *local_err = NULL;
+const char *newname = name + strlen(data->prefix);
+
+qemu_opt_set(data->opts, newname, value, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return 1;
+}
+}
+
+return 0;
+}
+
+/*
+ * Create a copy of @opts containing only the fields with
+ * a prefix of @prefix, stripping the prefix in the returned
+ * opts
+ */
+static QemuOpts *
+block_crypto_copy_opts(QemuOpts *opts,
+   const char *prefix,
+   Error **errp)
+{
+struct BlockCryptoCopyData data = {
+.opts = qemu_opts_create(&empty_opts, NULL, false, errp),
+.prefix = prefix
+};
+if (!data.opts) {
+return NULL;
+}
+
+if (qemu_opt_foreach(opts, block_crypto_copy_value, &data, errp) != 0) {
+qemu_opts_del(data.opts);
+return NULL;
+}
+
+return data.opts;
+}
 
 QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QCryptoBlockFormat format,
 QemuOpts *opts,
+const char *prefix,
 Error **errp)
 {
-Visitor *v;
+Visitor *v = NULL;
 QCryptoBlockOpenOptions *ret = NULL;
 Error *local_err = NULL;
+QemuOpts *newopts = NULL;
 
 ret = g_new0(QCryptoBlockOpenOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+if (prefix != NULL) {
+newopts = block_crypto_copy_opts(opts, prefix, &local_err);
+if (local_err) {
+goto out;
+}
+v = opts_visitor_new(newopts);
+} else {
+v = opts_visitor_new(opts);
+}
 
 visit_start_struct(v, NULL, NULL, 0, &local_err);
 if (local_err) {
@@ -196,6 +266,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 qapi_free_QCryptoBlockOpenOptions(ret);
 ret = NULL;
 }
+qemu_opts_del(newopts);
 visit_free(v);
 return ret;
 }
@@ -204,16 +275,26 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 QCryptoBlockCreateOptions *
 block_crypto_create_opts_init(QCryptoBlockFormat format,
   QemuOpts *opts,
+  const char *prefix,
   Error **errp)
 {
-Visitor *v;
+Visitor *v = NULL;
 QCryptoBlockCreateOptions *ret = NULL;
 Error *local_err = NULL;
+QemuOpts *newopts = NULL;
 
 ret = g_new0(QCryptoBlockCreateOptions, 1);
 ret->format = format;
 
-v = opts_visitor_new(opts);
+if (prefix != NULL) {
+newopts =