[Qemu-devel] [PATCH for-2.9 0/5] rbd: Clean up API and code

[Qemu-devel] [PATCH for-2.9 0/5] rbd: Clean up API and code

[Markus Armbruster]

These API cleanups need to go into 2.9.

Markus Armbruster (5):
  rbd: Clean up runtime_opts
  rbd: Clean up qemu_rbd_create()'s detour through QemuOpts
  rbd: Rewrite the code to extract list-valued options
  rbd: Peel off redundant RbdAuthMethod wrapper struct
  rbd: Reject options server.*.{numeric,to,ipv4,ipv6}

 block/rbd.c          | 198 +++++++++++++++++----------------------------------
 qapi-schema.json     |  21 ++++--
 qapi/block-core.json |  17 +----
 3 files changed, 82 insertions(+), 154 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 1/5] rbd: Clean up runtime_opts

[Markus Armbruster]

runtime_opts is used for three different purposes:

* qemu_rbd_open() uses it to accept options it recognizes, such as
  "pool" and "image".  Other .bdrv_open() methods do it similarly.

* qemu_rbd_open() accepts additional list-valued options
  auth-supported and server, with the help of qemu_rbd_array_opts().
  The list elements are again dictionaries.  qemu_rbd_array_opts()
  uses runtime_opts to accept their members.  Thus, runtime_opts
  contains recognized sub-sub-options "auth", "host", "port" in
  addition to recognized options.  No other block driver does that.

* qemu_rbd_create() uses it to converts the QDict produced by
  qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
  that.  The keys produced by qemu_rbd_parse_filename() are "pool",
  "image", "snapshot", "conf", "user" and "keyvalue-pairs".
  qemu_rbd_open() accepts these, so no additional ones here.

This is a confusing mess.  Dates back to commit 0f9d252.  First step
to clean it up is documenting runtime_opts.desc[]:

* Reorder entries to match the QAPI schema, like we do in other block
  drivers.

* Document why the schema's "server" and "auth-supported" aren't in
  .desc[].

* Document why "keyvalue-pairs", "host", "port" and "auth" are in
  .desc[], but not the schema.

* Delete "filename", because none of the three users actually uses it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ee13f3d..6562fbd 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -365,21 +365,6 @@ static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-            .help = "Specification of the rbd image",
-        },
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
-        {
-            .name = "conf",
-            .type = QEMU_OPT_STRING,
-            .help = "Rados config file location",
-        },
-        {
             .name = "pool",
             .type = QEMU_OPT_STRING,
             .help = "Rados pool name",
@@ -390,6 +375,11 @@ static QemuOptsList runtime_opts = {
             .help = "Image name in the pool",
         },
         {
+            .name = "conf",
+            .type = QEMU_OPT_STRING,
+            .help = "Rados config file location",
+        },
+        {
             .name = "snapshot",
             .type = QEMU_OPT_STRING,
             .help = "Ceph snapshot name",
@@ -400,11 +390,30 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Rados id name",
         },
+        /*
+         * server.* and auth-supported.* extracted manually, see
+         * qemu_rbd_array_opts()
+         */
+        {
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret providing the password",
+        },
+        /*
+         * Legacy keys accepted by qemu_rbd_parse_filename(), not in
+         * the QAPI schema
+         */
         {
             .name = "keyvalue-pairs",
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
+        /*
+         * The remainder aren't option keys, but option sub-sub-keys,
+         * so that qemu_rbd_array_opts() can abuse runtime_opts for
+         * its own purposes
+         * TODO clean this up
+         */
         {
             .name = "host",
             .type = QEMU_OPT_STRING,
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 1/5] rbd: Clean up runtime_opts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> runtime_opts is used for three different purposes:
> 
> * qemu_rbd_open() uses it to accept options it recognizes, such as
>   "pool" and "image".  Other .bdrv_open() methods do it similarly.
> 
> * qemu_rbd_open() accepts additional list-valued options
>   auth-supported and server, with the help of qemu_rbd_array_opts().
>   The list elements are again dictionaries.  qemu_rbd_array_opts()
>   uses runtime_opts to accept their members.  Thus, runtime_opts
>   contains recognized sub-sub-options "auth", "host", "port" in
>   addition to recognized options.  No other block driver does that.
> 
> * qemu_rbd_create() uses it to converts the QDict produced by

s/converts/convert/

>   qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
>   that.  The keys produced by qemu_rbd_parse_filename() are "pool",
>   "image", "snapshot", "conf", "user" and "keyvalue-pairs".
>   qemu_rbd_open() accepts these, so no additional ones here.
> 
> This is a confusing mess.  Dates back to commit 0f9d252.  First step

That commit is unreleased, so we are still free to improve it before 2.9
is finalized.

> to clean it up is documenting runtime_opts.desc[]:
> 
> * Reorder entries to match the QAPI schema, like we do in other block
>   drivers.
> 
> * Document why the schema's "server" and "auth-supported" aren't in
>   .desc[].
> 
> * Document why "keyvalue-pairs", "host", "port" and "auth" are in
>   .desc[], but not the schema.
> 
> * Delete "filename", because none of the three users actually uses it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 1/5] rbd: Clean up runtime_opts

[Kevin Wolf]

Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
> runtime_opts is used for three different purposes:
> 
> * qemu_rbd_open() uses it to accept options it recognizes, such as
>   "pool" and "image".  Other .bdrv_open() methods do it similarly.
> 
> * qemu_rbd_open() accepts additional list-valued options
>   auth-supported and server, with the help of qemu_rbd_array_opts().
>   The list elements are again dictionaries.  qemu_rbd_array_opts()
>   uses runtime_opts to accept their members.  Thus, runtime_opts
>   contains recognized sub-sub-options "auth", "host", "port" in
>   addition to recognized options.  No other block driver does that.
> 
> * qemu_rbd_create() uses it to converts the QDict produced by
>   qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
>   that.  The keys produced by qemu_rbd_parse_filename() are "pool",
>   "image", "snapshot", "conf", "user" and "keyvalue-pairs".
>   qemu_rbd_open() accepts these, so no additional ones here.
> 
> This is a confusing mess.  Dates back to commit 0f9d252.  First step
> to clean it up is documenting runtime_opts.desc[]:
> 
> * Reorder entries to match the QAPI schema, like we do in other block
>   drivers.
> 
> * Document why the schema's "server" and "auth-supported" aren't in
>   .desc[].
> 
> * Document why "keyvalue-pairs", "host", "port" and "auth" are in
>   .desc[], but not the schema.
> 
> * Delete "filename", because none of the three users actually uses it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH for-2.9 2/5] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Markus Armbruster]

The conversion from QDict to QemuOpts is pointless.  Simply get the
stuff straight from the QDict.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 6562fbd..59c822a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -442,7 +442,6 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     rados_t cluster;
     rados_ioctx_t io_ctx;
     QDict *options = NULL;
-    QemuOpts *rbd_opts = NULL;
     int ret = 0;
 
     secretid = qemu_opt_get(opts, "password-secret");
@@ -473,19 +472,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
-    rbd_opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(rbd_opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto exit;
-    }
-
-    pool       = qemu_opt_get(rbd_opts, "pool");
-    conf       = qemu_opt_get(rbd_opts, "conf");
-    clientname = qemu_opt_get(rbd_opts, "user");
-    name       = qemu_opt_get(rbd_opts, "image");
-    keypairs   = qemu_opt_get(rbd_opts, "keyvalue-pairs");
+    pool       = qdict_get_str(options, "pool");
+    conf       = qdict_get_str(options, "conf");
+    clientname = qdict_get_str(options, "user");
+    name       = qdict_get_str(options, "image");
+    keypairs   = qdict_get_str(options, "keyvalue-pairs");
 
     ret = rados_create(&cluster, clientname);
     if (ret < 0) {
@@ -536,7 +527,6 @@ shutdown:
 
 exit:
     QDECREF(options);
-    qemu_opts_del(rbd_opts);
     return ret;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 2/5] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> The conversion from QDict to QemuOpts is pointless.  Simply get the
> stuff straight from the QDict.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 2/5] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Kevin Wolf]

Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
> The conversion from QDict to QemuOpts is pointless.  Simply get the
> stuff straight from the QDict.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

We have two list-values options:

* "server" is a list of InetSocketAddress.  We use members "host" and
  "port", and silently ignore the rest.

* "auth-supported" is a list of RbdAuthMethod.  We use its only member
  "auth".

Since qemu_rbd_open() takes options as a flattened QDict, options has
keys of the form server.%d.host, server.%d.port and
auth-supported.%d.auth, where %d counts up from zero.

qemu_rbd_array_opts() extracts these values as follows.  First, it
calls qdict_array_entries() to find the list's length.  For each list
element, it first formats the list's key prefix (e.g. "server.0."),
then creates a new QDict holding the options with that key prefix,
then converts that to a QemuOpts, so it can finally get the member
values from there.

If there's one surefire way to make code using QDict more awkward,
it's creating more of them and mixing in QemuOpts for good measure.

The conversion to QemuOpts abuses runtime_opts, as described in the
commit before previous.

Rewrite to simply get the values straight from the options QDict.
This removes the abuse of runtime_opts, so clean it up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 151 +++++++++++++++++-------------------------------------------
 1 file changed, 42 insertions(+), 109 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 59c822a..8ba0f79 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 
+#include <rbd/librbd.h>
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
@@ -20,8 +21,6 @@
 #include "qemu/cutils.h"
 #include "qapi/qmp/qstring.h"
 
-#include <rbd/librbd.h>
-
 /*
  * When specifying the image filename use:
  *
@@ -408,25 +407,6 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
-        /*
-         * The remainder aren't option keys, but option sub-sub-keys,
-         * so that qemu_rbd_array_opts() can abuse runtime_opts for
-         * its own purposes
-         * TODO clean this up
-         */
-        {
-            .name = "host",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "port",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "auth",
-            .type = QEMU_OPT_STRING,
-            .help = "Supported authentication method, either cephx or none",
-        },
         { /* end of list */ }
     },
 };
@@ -577,91 +557,59 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-#define RBD_MON_HOST          0
-#define RBD_AUTH_SUPPORTED    1
-
-static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
-                                 Error **errp)
+static char *rbd_auth(QDict *options)
 {
-    int num_entries;
-    QemuOpts *opts = NULL;
-    QDict *sub_options;
-    const char *host;
-    const char *port;
-    char *str;
-    char *rados_str = NULL;
-    Error *local_err = NULL;
+    const char **vals = g_new(const char *, qdict_size(options));
+    char keybuf[32];
+    QObject *val;
+    char *rados_str;
     int i;
 
-    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
-
-    num_entries = qdict_array_entries(options, prefix);
+    for (i = 0;; i++) {
+        sprintf(keybuf, "auth-supported.%d.auth", i);
+        val = qdict_get(options, keybuf);
+        if (!val) {
+            break;
+        }
 
-    if (num_entries < 0) {
-        error_setg(errp, "Parse error on RBD QDict array");
-        return NULL;
+        vals[i] = qstring_get_str(qobject_to_qstring(val));
     }
+    vals[i] = NULL;
 
-    for (i = 0; i < num_entries; i++) {
-        char *strbuf = NULL;
-        const char *value;
-        char *rados_str_tmp;
-
-        str = g_strdup_printf("%s%d.", prefix, i);
-        qdict_extract_subqdict(options, &sub_options, str);
-        g_free(str);
-
-        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
-        QDECREF(sub_options);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            g_free(rados_str);
-            rados_str = NULL;
-            goto exit;
-        }
+    rados_str = g_strjoinv(";", (char **)vals);
+    g_free(vals);
+    return rados_str;
+}
 
-        if (type == RBD_MON_HOST) {
-            host = qemu_opt_get(opts, "host");
-            port = qemu_opt_get(opts, "port");
+static char *rbd_mon_host(QDict *options)
+{
+    const char **vals = g_new(const char *, qdict_size(options));
+    char keybuf[32];
+    QObject *val;
+    const char *host, *port;
+    char *rados_str;
+    int i;
 
-            value = host;
-            if (port) {
-                /* check for ipv6 */
-                if (strchr(host, ':')) {
-                    strbuf = g_strdup_printf("[%s]:%s", host, port);
-                } else {
-                    strbuf = g_strdup_printf("%s:%s", host, port);
-                }
-                value = strbuf;
-            } else if (strchr(host, ':')) {
-                strbuf = g_strdup_printf("[%s]", host);
-                value = strbuf;
-            }
-        } else {
-            value = qemu_opt_get(opts, "auth");
+    for (i = 0;; i++) {
+        sprintf(keybuf, "server.%d.host", i);
+        val = qdict_get(options, keybuf);
+        if (!val) {
+            break;
         }
+        host = qstring_get_str(qobject_to_qstring(val));
+        sprintf(keybuf, "server.%d.port", i);
+        port = qdict_get_str(options, keybuf);
 
-
-        /* each iteration in the for loop will build upon the string, and if
-         * rados_str is NULL then it is our first pass */
-        if (rados_str) {
-            /* separate options with ';', as that  is what rados_conf_set()
-             * requires */
-            rados_str_tmp = rados_str;
-            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
-            g_free(rados_str_tmp);
+        if (strchr(host, ':')) {
+            vals[i] = g_strdup_printf("[%s]:%s", host, port);
         } else {
-            rados_str = g_strdup(value);
+            vals[i] = g_strdup_printf("%s:%s", host, port);
         }
-
-        g_free(strbuf);
-        qemu_opts_del(opts);
-        opts = NULL;
     }
+    vals[i] = NULL;
 
-exit:
-    qemu_opts_del(opts);
+    rados_str = g_strjoinv(";", (char **)vals);
+    g_strfreev((char **)vals);
     return rados_str;
 }
 
@@ -685,24 +633,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
-                                         RBD_AUTH_SUPPORTED, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
-    mon_host = qemu_rbd_array_opts(options, "server.",
-                                   RBD_MON_HOST, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
+    auth_supported = rbd_auth(options);
+    mon_host = rbd_mon_host(options);
     secretid = qemu_opt_get(opts, "password-secret");
-
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5158 bytes --]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> We have two list-values options:
> 
> * "server" is a list of InetSocketAddress.  We use members "host" and
>   "port", and silently ignore the rest.
> 
> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>   "auth".
> 
> Since qemu_rbd_open() takes options as a flattened QDict, options has
> keys of the form server.%d.host, server.%d.port and
> auth-supported.%d.auth, where %d counts up from zero.
> 
> qemu_rbd_array_opts() extracts these values as follows.  First, it
> calls qdict_array_entries() to find the list's length.  For each list
> element, it first formats the list's key prefix (e.g. "server.0."),
> then creates a new QDict holding the options with that key prefix,
> then converts that to a QemuOpts, so it can finally get the member
> values from there.
> 
> If there's one surefire way to make code using QDict more awkward,
> it's creating more of them and mixing in QemuOpts for good measure.

No kidding!

> 
> The conversion to QemuOpts abuses runtime_opts, as described in the
> commit before previous.
> 
> Rewrite to simply get the values straight from the options QDict.
> This removes the abuse of runtime_opts, so clean it up.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 151 +++++++++++++++++-------------------------------------------
>  1 file changed, 42 insertions(+), 109 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 59c822a..8ba0f79 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  
> +#include <rbd/librbd.h>
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "block/block_int.h"
> @@ -20,8 +21,6 @@
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/qstring.h"
>  
> -#include <rbd/librbd.h>
> -

Not mentioned in the commit message, but also a useful cleanup for
hoisting <includes> prior to "includes".

> +static char *rbd_auth(QDict *options)
>  {
> -    int num_entries;
> -    QemuOpts *opts = NULL;
> -    QDict *sub_options;
> -    const char *host;
> -    const char *port;
> -    char *str;
> -    char *rados_str = NULL;
> -    Error *local_err = NULL;
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    char *rados_str;
>      int i;
>  
> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> -
> -    num_entries = qdict_array_entries(options, prefix);
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "auth-supported.%d.auth", i);

By my count, and including a trailing NUL, this is 21 bytes + the
maximum size of a formatted int to fit in keybuf[32]; 32-bit INT_MIN is
indeed 11 bytes.  Cutting it close there, but I don't see an overflow
(if gcc 7's new -Wformat-truncation spots something, then gcc is too
strict.)

> +static char *rbd_mon_host(QDict *options)
> +{
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    const char *host, *port;
> +    char *rados_str;
> +    int i;
>  
> -            value = host;
> -            if (port) {
> -                /* check for ipv6 */
> -                if (strchr(host, ':')) {
> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
> -                } else {
> -                    strbuf = g_strdup_printf("%s:%s", host, port);

The old code only prints port information if it is present...

> -                }
> -                value = strbuf;
> -            } else if (strchr(host, ':')) {
> -                strbuf = g_strdup_printf("[%s]", host);
> -                value = strbuf;
> -            }
> -        } else {
> -            value = qemu_opt_get(opts, "auth");
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "server.%d.host", i);

Here, you've got more breathing room.

> +        val = qdict_get(options, keybuf);
> +        if (!val) {
> +            break;
>          }
> +        host = qstring_get_str(qobject_to_qstring(val));
> +        sprintf(keybuf, "server.%d.port", i);
> +        port = qdict_get_str(options, keybuf);
>  
> -
> -        /* each iteration in the for loop will build upon the string, and if
> -         * rados_str is NULL then it is our first pass */
> -        if (rados_str) {
> -            /* separate options with ';', as that  is what rados_conf_set()
> -             * requires */
> -            rados_str_tmp = rados_str;
> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
> -            g_free(rados_str_tmp);
> +        if (strchr(host, ':')) {
> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>          } else {
> -            rados_str = g_strdup(value);
> +            vals[i] = g_strdup_printf("%s:%s", host, port);

...but the new code unconditionally prints port information, even when
port == NULL.  Oops.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/23/2017 05:55 AM, Markus Armbruster wrote:
>> We have two list-values options:
>> 
>> * "server" is a list of InetSocketAddress.  We use members "host" and
>>   "port", and silently ignore the rest.
>> 
>> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>>   "auth".
>> 
>> Since qemu_rbd_open() takes options as a flattened QDict, options has
>> keys of the form server.%d.host, server.%d.port and
>> auth-supported.%d.auth, where %d counts up from zero.
>> 
>> qemu_rbd_array_opts() extracts these values as follows.  First, it
>> calls qdict_array_entries() to find the list's length.  For each list
>> element, it first formats the list's key prefix (e.g. "server.0."),
>> then creates a new QDict holding the options with that key prefix,
>> then converts that to a QemuOpts, so it can finally get the member
>> values from there.
>> 
>> If there's one surefire way to make code using QDict more awkward,
>> it's creating more of them and mixing in QemuOpts for good measure.
>
> No kidding!
>
>> 
>> The conversion to QemuOpts abuses runtime_opts, as described in the
>> commit before previous.
>> 
>> Rewrite to simply get the values straight from the options QDict.
>> This removes the abuse of runtime_opts, so clean it up.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/rbd.c | 151 +++++++++++++++++-------------------------------------------
>>  1 file changed, 42 insertions(+), 109 deletions(-)
>> 
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 59c822a..8ba0f79 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -13,6 +13,7 @@
>>  
>>  #include "qemu/osdep.h"
>>  
>> +#include <rbd/librbd.h>
>>  #include "qapi/error.h"
>>  #include "qemu/error-report.h"
>>  #include "block/block_int.h"
>> @@ -20,8 +21,6 @@
>>  #include "qemu/cutils.h"
>>  #include "qapi/qmp/qstring.h"
>>  
>> -#include <rbd/librbd.h>
>> -
>
> Not mentioned in the commit message, but also a useful cleanup for
> hoisting <includes> prior to "includes".
>
>> +static char *rbd_auth(QDict *options)
>>  {
>> -    int num_entries;
>> -    QemuOpts *opts = NULL;
>> -    QDict *sub_options;
>> -    const char *host;
>> -    const char *port;
>> -    char *str;
>> -    char *rados_str = NULL;
>> -    Error *local_err = NULL;
>> +    const char **vals = g_new(const char *, qdict_size(options));
>> +    char keybuf[32];
>> +    QObject *val;
>> +    char *rados_str;
>>      int i;
>>  
>> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
>> -
>> -    num_entries = qdict_array_entries(options, prefix);
>> +    for (i = 0;; i++) {
>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>
> By my count, and including a trailing NUL, this is 21 bytes + the
> maximum size of a formatted int to fit in keybuf[32]; 32-bit INT_MIN is
> indeed 11 bytes.  Cutting it close there, but I don't see an overflow
> (if gcc 7's new -Wformat-truncation spots something, then gcc is too
> strict.)

11 decimal digits take a hell of a list :)

Could double the buffer if it makes anyone sleep better.

>> +static char *rbd_mon_host(QDict *options)
>> +{
>> +    const char **vals = g_new(const char *, qdict_size(options));
>> +    char keybuf[32];
>> +    QObject *val;
>> +    const char *host, *port;
>> +    char *rados_str;
>> +    int i;
>>  
>> -            value = host;
>> -            if (port) {
>> -                /* check for ipv6 */
>> -                if (strchr(host, ':')) {
>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>> -                } else {
>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>
> The old code only prints port information if it is present...
>
>> -                }
>> -                value = strbuf;
>> -            } else if (strchr(host, ':')) {
>> -                strbuf = g_strdup_printf("[%s]", host);
>> -                value = strbuf;
>> -            }
>> -        } else {
>> -            value = qemu_opt_get(opts, "auth");
>> +    for (i = 0;; i++) {
>> +        sprintf(keybuf, "server.%d.host", i);
>
> Here, you've got more breathing room.
>
>> +        val = qdict_get(options, keybuf);
>> +        if (!val) {
>> +            break;
>>          }
>> +        host = qstring_get_str(qobject_to_qstring(val));
>> +        sprintf(keybuf, "server.%d.port", i);
>> +        port = qdict_get_str(options, keybuf);
>>  
>> -
>> -        /* each iteration in the for loop will build upon the string, and if
>> -         * rados_str is NULL then it is our first pass */
>> -        if (rados_str) {
>> -            /* separate options with ';', as that  is what rados_conf_set()
>> -             * requires */
>> -            rados_str_tmp = rados_str;
>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>> -            g_free(rados_str_tmp);
>> +        if (strchr(host, ':')) {
>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>          } else {
>> -            rados_str = g_strdup(value);
>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>
> ...but the new code unconditionally prints port information, even when
> port == NULL.  Oops.

How can port be null?  SocketAddress member port is mandatory...

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

On 03/23/2017 01:27 PM, Markus Armbruster wrote:

>>> -
>>> -    num_entries = qdict_array_entries(options, prefix);
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>>
>> By my count, and including a trailing NUL, this is 21 bytes + the
>> maximum size of a formatted int to fit in keybuf[32]; 32-bit INT_MIN is
>> indeed 11 bytes.  Cutting it close there, but I don't see an overflow
>> (if gcc 7's new -Wformat-truncation spots something, then gcc is too
>> strict.)
> 
> 11 decimal digits take a hell of a list :)
> 
> Could double the buffer if it makes anyone sleep better.

I'm okay with its current size.


>>> -            value = host;
>>> -            if (port) {
>>> -                /* check for ipv6 */
>>> -                if (strchr(host, ':')) {
>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>> -                } else {
>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>
>> The old code only prints port information if it is present...
>>

>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>> +        sprintf(keybuf, "server.%d.port", i);
>>> +        port = qdict_get_str(options, keybuf);
>>>  
>>> -
>>> -        /* each iteration in the for loop will build upon the string, and if
>>> -         * rados_str is NULL then it is our first pass */
>>> -        if (rados_str) {
>>> -            /* separate options with ';', as that  is what rados_conf_set()
>>> -             * requires */
>>> -            rados_str_tmp = rados_str;
>>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>>> -            g_free(rados_str_tmp);
>>> +        if (strchr(host, ':')) {
>>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>>          } else {
>>> -            rados_str = g_strdup(value);
>>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>>
>> ...but the new code unconditionally prints port information, even when
>> port == NULL.  Oops.
> 
> How can port be null?  SocketAddress member port is mandatory...

Indeed. Does that mean the old code had a dead branch? Looks like it!

So, if I'm not overlooking something, that means you've resolved all my
open questions, and can submit the patch as written with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On 03/23/2017 02:18 PM, Eric Blake wrote:

>>>> -            if (port) {
>>>> -                /* check for ipv6 */
>>>> -                if (strchr(host, ':')) {
>>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>>> -                } else {
>>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>>
>>> The old code only prints port information if it is present...
>>>

>>> ...but the new code unconditionally prints port information, even when
>>> port == NULL.  Oops.
>>
>> How can port be null?  SocketAddress member port is mandatory...
> 
> Indeed. Does that mean the old code had a dead branch? Looks like it!

Or else my reading of the old code was wrong.  It looks like port was
optional on the command line, but in the conversion to QemuOpts, a
missing port was treated as the default port.  Now that you are not
going through QemuOpts, you have to make sure that you can still supply
a default port to keep the QAPI happy with a mandatory port.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/23/2017 02:18 PM, Eric Blake wrote:
>
>>>>> -            if (port) {
>>>>> -                /* check for ipv6 */
>>>>> -                if (strchr(host, ':')) {
>>>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>>>> -                } else {
>>>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>>>
>>>> The old code only prints port information if it is present...
>>>>
>
>>>> ...but the new code unconditionally prints port information, even when
>>>> port == NULL.  Oops.
>>>
>>> How can port be null?  SocketAddress member port is mandatory...
>> 
>> Indeed. Does that mean the old code had a dead branch? Looks like it!
>
> Or else my reading of the old code was wrong.  It looks like port was
> optional on the command line, but in the conversion to QemuOpts, a
> missing port was treated as the default port.  Now that you are not
> going through QemuOpts, you have to make sure that you can still supply
> a default port to keep the QAPI happy with a mandatory port.

Who is "you", and what exactly does "you" have to do?  ;)

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Kevin Wolf]

Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
> We have two list-values options:
> 
> * "server" is a list of InetSocketAddress.  We use members "host" and
>   "port", and silently ignore the rest.
> 
> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>   "auth".
> 
> Since qemu_rbd_open() takes options as a flattened QDict, options has
> keys of the form server.%d.host, server.%d.port and
> auth-supported.%d.auth, where %d counts up from zero.
> 
> qemu_rbd_array_opts() extracts these values as follows.  First, it
> calls qdict_array_entries() to find the list's length.  For each list
> element, it first formats the list's key prefix (e.g. "server.0."),
> then creates a new QDict holding the options with that key prefix,
> then converts that to a QemuOpts, so it can finally get the member
> values from there.
> 
> If there's one surefire way to make code using QDict more awkward,
> it's creating more of them and mixing in QemuOpts for good measure.
> 
> The conversion to QemuOpts abuses runtime_opts, as described in the
> commit before previous.
> 
> Rewrite to simply get the values straight from the options QDict.
> This removes the abuse of runtime_opts, so clean it up.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> @@ -577,91 +557,59 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>      qemu_aio_unref(acb);
>  }
>  
> -#define RBD_MON_HOST          0
> -#define RBD_AUTH_SUPPORTED    1
> -
> -static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> -                                 Error **errp)
> +static char *rbd_auth(QDict *options)
>  {
> -    int num_entries;
> -    QemuOpts *opts = NULL;
> -    QDict *sub_options;
> -    const char *host;
> -    const char *port;
> -    char *str;
> -    char *rados_str = NULL;
> -    Error *local_err = NULL;
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    char *rados_str;
>      int i;
>  
> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> -
> -    num_entries = qdict_array_entries(options, prefix);
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "auth-supported.%d.auth", i);
> +        val = qdict_get(options, keybuf);
> +        if (!val) {
> +            break;
> +        }
>  
> -    if (num_entries < 0) {
> -        error_setg(errp, "Parse error on RBD QDict array");
> -        return NULL;
> +        vals[i] = qstring_get_str(qobject_to_qstring(val));
>      }
> +    vals[i] = NULL;

In case of doubt, i is one more than vals can hold. (It segfaulted for
me when options was empty because I passed only options that are removed
before this function is called.)

You also want to remove the options from the QDict, otherwise
bdrv_open_inherit() will complain that the options are unknown.

>  
> -    for (i = 0; i < num_entries; i++) {
> -        char *strbuf = NULL;
> -        const char *value;
> -        char *rados_str_tmp;
> -
> -        str = g_strdup_printf("%s%d.", prefix, i);
> -        qdict_extract_subqdict(options, &sub_options, str);
> -        g_free(str);
> -
> -        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> -        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
> -        QDECREF(sub_options);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            g_free(rados_str);
> -            rados_str = NULL;
> -            goto exit;
> -        }
> +    rados_str = g_strjoinv(";", (char **)vals);
> +    g_free(vals);
> +    return rados_str;
> +}
>  
> -        if (type == RBD_MON_HOST) {
> -            host = qemu_opt_get(opts, "host");
> -            port = qemu_opt_get(opts, "port");
> +static char *rbd_mon_host(QDict *options)
> +{
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    const char *host, *port;
> +    char *rados_str;
> +    int i;
>  
> -            value = host;
> -            if (port) {
> -                /* check for ipv6 */
> -                if (strchr(host, ':')) {
> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
> -                } else {
> -                    strbuf = g_strdup_printf("%s:%s", host, port);
> -                }
> -                value = strbuf;
> -            } else if (strchr(host, ':')) {
> -                strbuf = g_strdup_printf("[%s]", host);
> -                value = strbuf;
> -            }
> -        } else {
> -            value = qemu_opt_get(opts, "auth");
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "server.%d.host", i);
> +        val = qdict_get(options, keybuf);
> +        if (!val) {
> +            break;
>          }
> +        host = qstring_get_str(qobject_to_qstring(val));
> +        sprintf(keybuf, "server.%d.port", i);
> +        port = qdict_get_str(options, keybuf);

This segfaults if the port option isn't given.

> -
> -        /* each iteration in the for loop will build upon the string, and if
> -         * rados_str is NULL then it is our first pass */
> -        if (rados_str) {
> -            /* separate options with ';', as that  is what rados_conf_set()
> -             * requires */
> -            rados_str_tmp = rados_str;
> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
> -            g_free(rados_str_tmp);
> +        if (strchr(host, ':')) {
> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>          } else {
> -            rados_str = g_strdup(value);
> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>          }
> -
> -        g_free(strbuf);
> -        qemu_opts_del(opts);
> -        opts = NULL;
>      }
> +    vals[i] = NULL;

Probably the same buffer overflow as above (but I didn't test that this
one really segfaults).

> -exit:
> -    qemu_opts_del(opts);
> +    rados_str = g_strjoinv(";", (char **)vals);
> +    g_strfreev((char **)vals);
>      return rados_str;
>  }
>  
> @@ -685,24 +633,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
> -                                         RBD_AUTH_SUPPORTED, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        r = -EINVAL;
> -        goto failed_opts;
> -    }
> -
> -    mon_host = qemu_rbd_array_opts(options, "server.",
> -                                   RBD_MON_HOST, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        r = -EINVAL;
> -        goto failed_opts;
> -    }
> -
> +    auth_supported = rbd_auth(options);
> +    mon_host = rbd_mon_host(options);
>      secretid = qemu_opt_get(opts, "password-secret");

Of course, this also changes the behaviour so that additional options in
server.* and auth-supported.* aren't silently ignored any more, but we
complain that they are unknown. I consider this a bonus bug fix, but it
should probably be spelt out in the commit message.

Kevin

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
>> We have two list-values options:
>> 
>> * "server" is a list of InetSocketAddress.  We use members "host" and
>>   "port", and silently ignore the rest.
>> 
>> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>>   "auth".
>> 
>> Since qemu_rbd_open() takes options as a flattened QDict, options has
>> keys of the form server.%d.host, server.%d.port and
>> auth-supported.%d.auth, where %d counts up from zero.
>> 
>> qemu_rbd_array_opts() extracts these values as follows.  First, it
>> calls qdict_array_entries() to find the list's length.  For each list
>> element, it first formats the list's key prefix (e.g. "server.0."),
>> then creates a new QDict holding the options with that key prefix,
>> then converts that to a QemuOpts, so it can finally get the member
>> values from there.
>> 
>> If there's one surefire way to make code using QDict more awkward,
>> it's creating more of them and mixing in QemuOpts for good measure.
>> 
>> The conversion to QemuOpts abuses runtime_opts, as described in the
>> commit before previous.
>> 
>> Rewrite to simply get the values straight from the options QDict.
>> This removes the abuse of runtime_opts, so clean it up.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> @@ -577,91 +557,59 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>>      qemu_aio_unref(acb);
>>  }
>>  
>> -#define RBD_MON_HOST          0
>> -#define RBD_AUTH_SUPPORTED    1
>> -
>> -static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
>> -                                 Error **errp)
>> +static char *rbd_auth(QDict *options)
>>  {
>> -    int num_entries;
>> -    QemuOpts *opts = NULL;
>> -    QDict *sub_options;
>> -    const char *host;
>> -    const char *port;
>> -    char *str;
>> -    char *rados_str = NULL;
>> -    Error *local_err = NULL;
>> +    const char **vals = g_new(const char *, qdict_size(options));
>> +    char keybuf[32];
>> +    QObject *val;
>> +    char *rados_str;
>>      int i;
>>  
>> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
>> -
>> -    num_entries = qdict_array_entries(options, prefix);
>> +    for (i = 0;; i++) {
>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>> +        val = qdict_get(options, keybuf);
>> +        if (!val) {
>> +            break;
>> +        }
>>  
>> -    if (num_entries < 0) {
>> -        error_setg(errp, "Parse error on RBD QDict array");
>> -        return NULL;
>> +        vals[i] = qstring_get_str(qobject_to_qstring(val));
>>      }
>> +    vals[i] = NULL;
>
> In case of doubt, i is one more than vals can hold. (It segfaulted for
> me when options was empty because I passed only options that are removed
> before this function is called.)

Yes, the g_new() above needs one extra slot.

> You also want to remove the options from the QDict, otherwise
> bdrv_open_inherit() will complain that the options are unknown.

Okay.

>>  
>> -    for (i = 0; i < num_entries; i++) {
>> -        char *strbuf = NULL;
>> -        const char *value;
>> -        char *rados_str_tmp;
>> -
>> -        str = g_strdup_printf("%s%d.", prefix, i);
>> -        qdict_extract_subqdict(options, &sub_options, str);
>> -        g_free(str);
>> -
>> -        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> -        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
>> -        QDECREF(sub_options);
>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            g_free(rados_str);
>> -            rados_str = NULL;
>> -            goto exit;
>> -        }
>> +    rados_str = g_strjoinv(";", (char **)vals);
>> +    g_free(vals);
>> +    return rados_str;
>> +}
>>  
>> -        if (type == RBD_MON_HOST) {
>> -            host = qemu_opt_get(opts, "host");
>> -            port = qemu_opt_get(opts, "port");
>> +static char *rbd_mon_host(QDict *options)
>> +{
>> +    const char **vals = g_new(const char *, qdict_size(options));
>> +    char keybuf[32];
>> +    QObject *val;
>> +    const char *host, *port;
>> +    char *rados_str;
>> +    int i;
>>  
>> -            value = host;
>> -            if (port) {
>> -                /* check for ipv6 */
>> -                if (strchr(host, ':')) {
>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>> -                } else {
>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>> -                }
>> -                value = strbuf;
>> -            } else if (strchr(host, ':')) {
>> -                strbuf = g_strdup_printf("[%s]", host);
>> -                value = strbuf;
>> -            }
>> -        } else {
>> -            value = qemu_opt_get(opts, "auth");
>> +    for (i = 0;; i++) {
>> +        sprintf(keybuf, "server.%d.host", i);
>> +        val = qdict_get(options, keybuf);
>> +        if (!val) {
>> +            break;
>>          }
>> +        host = qstring_get_str(qobject_to_qstring(val));
>> +        sprintf(keybuf, "server.%d.port", i);
>> +        port = qdict_get_str(options, keybuf);
>
> This segfaults if the port option isn't given.

@port is mandatory in BlockdevOptionsRbd.  If it's not there here, the
options must have bypassed QAPI.  That would be bad news.  Can you
explain how it can happen?

>> -
>> -        /* each iteration in the for loop will build upon the string, and if
>> -         * rados_str is NULL then it is our first pass */
>> -        if (rados_str) {
>> -            /* separate options with ';', as that  is what rados_conf_set()
>> -             * requires */
>> -            rados_str_tmp = rados_str;
>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>> -            g_free(rados_str_tmp);
>> +        if (strchr(host, ':')) {
>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>          } else {
>> -            rados_str = g_strdup(value);
>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>>          }
>> -
>> -        g_free(strbuf);
>> -        qemu_opts_del(opts);
>> -        opts = NULL;
>>      }
>> +    vals[i] = NULL;
>
> Probably the same buffer overflow as above (but I didn't test that this
> one really segfaults).

Yes, same off-by-one.

>> -exit:
>> -    qemu_opts_del(opts);
>> +    rados_str = g_strjoinv(";", (char **)vals);
>> +    g_strfreev((char **)vals);
>>      return rados_str;
>>  }
>>  
>> @@ -685,24 +633,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>  
>> -    auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
>> -                                         RBD_AUTH_SUPPORTED, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        r = -EINVAL;
>> -        goto failed_opts;
>> -    }
>> -
>> -    mon_host = qemu_rbd_array_opts(options, "server.",
>> -                                   RBD_MON_HOST, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        r = -EINVAL;
>> -        goto failed_opts;
>> -    }
>> -
>> +    auth_supported = rbd_auth(options);
>> +    mon_host = rbd_mon_host(options);
>>      secretid = qemu_opt_get(opts, "password-secret");
>
> Of course, this also changes the behaviour so that additional options in
> server.* and auth-supported.* aren't silently ignored any more, but we
> complain that they are unknown. I consider this a bonus bug fix, but it
> should probably be spelt out in the commit message.

Good point.

Thanks!

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
>>> We have two list-values options:
>>> 
>>> * "server" is a list of InetSocketAddress.  We use members "host" and
>>>   "port", and silently ignore the rest.
>>> 
>>> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>>>   "auth".
>>> 
>>> Since qemu_rbd_open() takes options as a flattened QDict, options has
>>> keys of the form server.%d.host, server.%d.port and
>>> auth-supported.%d.auth, where %d counts up from zero.
>>> 
>>> qemu_rbd_array_opts() extracts these values as follows.  First, it
>>> calls qdict_array_entries() to find the list's length.  For each list
>>> element, it first formats the list's key prefix (e.g. "server.0."),
>>> then creates a new QDict holding the options with that key prefix,
>>> then converts that to a QemuOpts, so it can finally get the member
>>> values from there.
>>> 
>>> If there's one surefire way to make code using QDict more awkward,
>>> it's creating more of them and mixing in QemuOpts for good measure.
>>> 
>>> The conversion to QemuOpts abuses runtime_opts, as described in the
>>> commit before previous.
>>> 
>>> Rewrite to simply get the values straight from the options QDict.
>>> This removes the abuse of runtime_opts, so clean it up.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>>> @@ -577,91 +557,59 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>>>      qemu_aio_unref(acb);
>>>  }
>>>  
>>> -#define RBD_MON_HOST          0
>>> -#define RBD_AUTH_SUPPORTED    1
>>> -
>>> -static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
>>> -                                 Error **errp)
>>> +static char *rbd_auth(QDict *options)
>>>  {
>>> -    int num_entries;
>>> -    QemuOpts *opts = NULL;
>>> -    QDict *sub_options;
>>> -    const char *host;
>>> -    const char *port;
>>> -    char *str;
>>> -    char *rados_str = NULL;
>>> -    Error *local_err = NULL;
>>> +    const char **vals = g_new(const char *, qdict_size(options));
>>> +    char keybuf[32];
>>> +    QObject *val;
>>> +    char *rados_str;
>>>      int i;
>>>  
>>> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
>>> -
>>> -    num_entries = qdict_array_entries(options, prefix);
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>>> +        val = qdict_get(options, keybuf);
>>> +        if (!val) {
>>> +            break;
>>> +        }
>>>  
>>> -    if (num_entries < 0) {
>>> -        error_setg(errp, "Parse error on RBD QDict array");
>>> -        return NULL;
>>> +        vals[i] = qstring_get_str(qobject_to_qstring(val));
>>>      }
>>> +    vals[i] = NULL;
>>
>> In case of doubt, i is one more than vals can hold. (It segfaulted for
>> me when options was empty because I passed only options that are removed
>> before this function is called.)
>
> Yes, the g_new() above needs one extra slot.
>
>> You also want to remove the options from the QDict, otherwise
>> bdrv_open_inherit() will complain that the options are unknown.
>
> Okay.
>
>>>  
>>> -    for (i = 0; i < num_entries; i++) {
>>> -        char *strbuf = NULL;
>>> -        const char *value;
>>> -        char *rados_str_tmp;
>>> -
>>> -        str = g_strdup_printf("%s%d.", prefix, i);
>>> -        qdict_extract_subqdict(options, &sub_options, str);
>>> -        g_free(str);
>>> -
>>> -        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>> -        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
>>> -        QDECREF(sub_options);
>>> -        if (local_err) {
>>> -            error_propagate(errp, local_err);
>>> -            g_free(rados_str);
>>> -            rados_str = NULL;
>>> -            goto exit;
>>> -        }
>>> +    rados_str = g_strjoinv(";", (char **)vals);
>>> +    g_free(vals);
>>> +    return rados_str;
>>> +}
>>>  
>>> -        if (type == RBD_MON_HOST) {
>>> -            host = qemu_opt_get(opts, "host");
>>> -            port = qemu_opt_get(opts, "port");
>>> +static char *rbd_mon_host(QDict *options)
>>> +{
>>> +    const char **vals = g_new(const char *, qdict_size(options));
>>> +    char keybuf[32];
>>> +    QObject *val;
>>> +    const char *host, *port;
>>> +    char *rados_str;
>>> +    int i;
>>>  
>>> -            value = host;
>>> -            if (port) {
>>> -                /* check for ipv6 */
>>> -                if (strchr(host, ':')) {
>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>> -                } else {
>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>> -                }
>>> -                value = strbuf;
>>> -            } else if (strchr(host, ':')) {
>>> -                strbuf = g_strdup_printf("[%s]", host);
>>> -                value = strbuf;
>>> -            }
>>> -        } else {
>>> -            value = qemu_opt_get(opts, "auth");
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "server.%d.host", i);
>>> +        val = qdict_get(options, keybuf);
>>> +        if (!val) {
>>> +            break;
>>>          }
>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>> +        sprintf(keybuf, "server.%d.port", i);
>>> +        port = qdict_get_str(options, keybuf);
>>
>> This segfaults if the port option isn't given.
>
> @port is mandatory in BlockdevOptionsRbd.  If it's not there here, the
> options must have bypassed QAPI.  That would be bad news.  Can you
> explain how it can happen?

Answering myself, please correct mistakes.

There are two ways to create @options:

1. -blockdev and blockdev-add

   These create @options with a QAPI visitor from the command line
   option argument or QMP arguments, respectively.  This checks them
   against the QAPI schema.  Missing @port is rejected.

2. -drive and drive_add

   These appear to create @options manually, without checking against
   the QAPI schema.

   Crash reproducer: -drive driver=rbd,server.0.host=s0

In other words, we have *two* specifications for @options: the QAPI
schema, and the union of all the QemuOptsList that apply.  In case 1, we
check against both (I think).  In case 2, we only check against the
latter.

I understand how we got into this state, but it's not a good state to be
in.  We need to have our options defined in one way and one way only.

For 2.9, we cope with missing @port.

Post 2.9, we should either finish the QAPIfication of block
configuration we started with blockdev-add, or back it out, i.e. make
the QAPI schema accept anything, and rely on the QemuOpts-based
checking.

I want us to finish QAPIfication.

>>> -
>>> -        /* each iteration in the for loop will build upon the string, and if
>>> -         * rados_str is NULL then it is our first pass */
>>> -        if (rados_str) {
>>> -            /* separate options with ';', as that  is what rados_conf_set()
>>> -             * requires */
>>> -            rados_str_tmp = rados_str;
>>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>>> -            g_free(rados_str_tmp);
>>> +        if (strchr(host, ':')) {
>>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>>          } else {
>>> -            rados_str = g_strdup(value);
>>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>>>          }
>>> -
>>> -        g_free(strbuf);
>>> -        qemu_opts_del(opts);
>>> -        opts = NULL;
>>>      }
>>> +    vals[i] = NULL;
>>
>> Probably the same buffer overflow as above (but I didn't test that this
>> one really segfaults).
>
> Yes, same off-by-one.
>
>>> -exit:
>>> -    qemu_opts_del(opts);
>>> +    rados_str = g_strjoinv(";", (char **)vals);
>>> +    g_strfreev((char **)vals);
>>>      return rados_str;
>>>  }
>>>  
>>> @@ -685,24 +633,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>>          return -EINVAL;
>>>      }
>>>  
>>> -    auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
>>> -                                         RBD_AUTH_SUPPORTED, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        r = -EINVAL;
>>> -        goto failed_opts;
>>> -    }
>>> -
>>> -    mon_host = qemu_rbd_array_opts(options, "server.",
>>> -                                   RBD_MON_HOST, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        r = -EINVAL;
>>> -        goto failed_opts;
>>> -    }
>>> -
>>> +    auth_supported = rbd_auth(options);
>>> +    mon_host = rbd_mon_host(options);
>>>      secretid = qemu_opt_get(opts, "password-secret");
>>
>> Of course, this also changes the behaviour so that additional options in
>> server.* and auth-supported.* aren't silently ignored any more, but we
>> complain that they are unknown. I consider this a bonus bug fix, but it
>> should probably be spelt out in the commit message.
>
> Good point.

Note to self: this applies to -drive / drive_add, but not to -blockdev /
blockdev_add, because the QAPI schema kicks in there.  Example:
-drive driver=rbd,server.0.host=s0,server.0.port=p0,server.0.foo=bar

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3848 bytes --]

On 03/24/2017 03:25 AM, Markus Armbruster wrote:

>>>> -            value = host;
>>>> -            if (port) {

>>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>>> +        sprintf(keybuf, "server.%d.port", i);
>>>> +        port = qdict_get_str(options, keybuf);
>>>
>>> This segfaults if the port option isn't given.
>>
>> @port is mandatory in BlockdevOptionsRbd.  If it's not there here, the
>> options must have bypassed QAPI.  That would be bad news.  Can you
>> explain how it can happen?
> 
> Answering myself, please correct mistakes.
> 
> There are two ways to create @options:
> 
> 1. -blockdev and blockdev-add
> 
>    These create @options with a QAPI visitor from the command line
>    option argument or QMP arguments, respectively.  This checks them
>    against the QAPI schema.  Missing @port is rejected.
> 
> 2. -drive and drive_add
> 
>    These appear to create @options manually, without checking against
>    the QAPI schema.
> 
>    Crash reproducer: -drive driver=rbd,server.0.host=s0

Yep - and that's why the old code was checking 'if (port)'.

> 
> In other words, we have *two* specifications for @options: the QAPI
> schema, and the union of all the QemuOptsList that apply.  In case 1, we
> check against both (I think).  In case 2, we only check against the
> latter.
> 
> I understand how we got into this state, but it's not a good state to be
> in.  We need to have our options defined in one way and one way only.
> 
> For 2.9, we cope with missing @port.

Maybe for 2.10 we would make port optional rather than mandatory - but
that's a LOT of code to audit to add in appropriate 'has_port=true' so
it's too late to do for 2.9.  Or even better, maybe for 2.10 we finally
implement parameter defaults so that we don't need a has_port field (if
port is omitted, it gets assigned the default value).  Except that I
don't know if all the existing users of InetSocketAddress will want the
same default.  Maybe that argues that we want some way to declare a QAPI
subtype that changes the default of a field otherwise inherited from the
base class (so port is mandatory in the base class, but each instance
that wants a default port creates a new subclass with a new default value).

> 
> Post 2.9, we should either finish the QAPIfication of block
> configuration we started with blockdev-add, or back it out, i.e. make
> the QAPI schema accept anything, and rely on the QemuOpts-based
> checking.
> 
> I want us to finish QAPIfication.

I'd like to finish the QAPIfication as well.  I'm fine if port is
mandatory for QMP and -blockdev-add for 2.9, where only -drive gets away
with the default (but it DOES mean that we have to make sure that
QemuOpts created by -drive is augmented with the default before we pass
it through QAPI validation), so that back-compat of -drive omitting port
doesn't break.


>>> Of course, this also changes the behaviour so that additional options in
>>> server.* and auth-supported.* aren't silently ignored any more, but we
>>> complain that they are unknown. I consider this a bonus bug fix, but it
>>> should probably be spelt out in the commit message.
>>
>> Good point.
> 
> Note to self: this applies to -drive / drive_add, but not to -blockdev /
> blockdev_add, because the QAPI schema kicks in there.  Example:
> -drive driver=rbd,server.0.host=s0,server.0.port=p0,server.0.foo=bar

Yep - another instance where -drive parsing is slightly different than
-blockdev-add parsing.  However, while tightening the parse to require
port is not back-compat friendly to -drive, I think that tightening the
parse to reject garbage given to -drive is perfectly acceptable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Kevin Wolf]

Am 24.03.2017 um 09:25 hat Markus Armbruster geschrieben:
> Post 2.9, we should either finish the QAPIfication of block
> configuration we started with blockdev-add, or back it out, i.e. make
> the QAPI schema accept anything, and rely on the QemuOpts-based
> checking.
> 
> I want us to finish QAPIfication.

So our next task is probably getting us a way to have options in QAPI
that aren't exposed in QMP. You added some of these non-QMP options to a
block driver yourself only recently, so you probably already know of and
understand the need for it. If we ever get QAPI for the command line,
this might actually be a third set because I think we have some options
that are only meant to be set internally, but never from either QMP or
the command line.

Once we have support from QAPI to consider the context when parse
structs (i.e. reject non-QMP fields when parsing QMP commands), we can
look into converting the block layer step by step. I'm not sure if this
is the only problem we have (I would be surprised if it were), but it's
the big one that I know of.

Kevin

[Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Markus Armbruster]

BlockdevOptionsRbd member auth-supported is a list of struct
RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
reason for wrapping the enum in a struct.  Delete the wrapper, and
rename the enum.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c          |  2 +-
 qapi/block-core.json | 15 ++-------------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 8ba0f79..f460d71 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -566,7 +566,7 @@ static char *rbd_auth(QDict *options)
     int i;
 
     for (i = 0;; i++) {
-        sprintf(keybuf, "auth-supported.%d.auth", i);
+        sprintf(keybuf, "auth-supported.%d", i);
         val = qdict_get(options, keybuf);
         if (!val) {
             break;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f132fc..fe1e40f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2601,25 +2601,14 @@
 
 
 ##
-# @RbdAuthSupport:
-#
-# An enumeration of RBD auth support
-#
-# Since: 2.9
-##
-{ 'enum': 'RbdAuthSupport',
-  'data': [ 'cephx', 'none' ] }
-
-
-##
 # @RbdAuthMethod:
 #
 # An enumeration of rados auth_supported types
 #
 # Since: 2.9
 ##
-{ 'struct': 'RbdAuthMethod',
-  'data': { 'auth': 'RbdAuthSupport' } }
+{ 'enum': 'RbdAuthMethod',
+  'data': [ 'cephx', 'none' ] }
 
 ##
 # @BlockdevOptionsRbd:
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> BlockdevOptionsRbd member auth-supported is a list of struct
> RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
> reason for wrapping the enum in a struct.

Well, the previous patch removed the QemuOpts madness as a technical
reason that required the wrapper, leaving nothing else technically using
it at the moment.  But there's still the design to think about...

>  Delete the wrapper, and
> rename the enum.

This patch changes the QMP wire format. It MUST go in 2.9, otherwise,
we've baked in the insanity; that is, if we decide it is insanity [1]

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          |  2 +-
>  qapi/block-core.json | 15 ++-------------
>  2 files changed, 3 insertions(+), 14 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2601,25 +2601,14 @@
>  
>  
>  ##
> -# @RbdAuthSupport:
> -#
> -# An enumeration of RBD auth support
> -#
> -# Since: 2.9
> -##
> -{ 'enum': 'RbdAuthSupport',
> -  'data': [ 'cephx', 'none' ] }
> -
> -
> -##
>  # @RbdAuthMethod:
>  #
>  # An enumeration of rados auth_supported types
>  #
>  # Since: 2.9
>  ##
> -{ 'struct': 'RbdAuthMethod',
> -  'data': { 'auth': 'RbdAuthSupport' } }
> +{ 'enum': 'RbdAuthMethod',
> +  'data': [ 'cephx', 'none' ] }

Changes BlockdevOptionsRbd from:

..., "auth-supported": [ { "auth": "none" } ],

to the potentially much nicer:

..., "auth-supported": [ "none" ],

[1] But what if we want to make auth-supported be an array of flat
unions, where 'auth' is the discriminator that determines if additional
members are needed?  In other words, the existing API would allow:

"auth-supported": [ { "auth": "cephx" },
                    { "auth": "new", "password-id": "foo" } ]

for specifying a new auth type 'new' that comes with an associated
'password-id' field for looking up a corresponding 'secret' object used
for retrieving the associated password when using that type of
authorization.  In that scenario, RbdAuthMethod would look more like
this pseudo-qapi:

{ 'union': 'RbdAuthMethod', 'base': { 'auth': 'RbdAuthSupport' },
  'discriminator': 'auth',
  'data': { 'none': {},
            'cephx': {},
            'new': { 'password-id': 'str' } } }

Assuming we are okay with not needing to extend the current one-member
RbdAuthMethod struct into a flat union, then this patch is fine, and you
can add:

Reviewed-by: Eric Blake <eblake@redhat.com>

But if you think the flat union expansion path may ever prove useful,
then maybe we don't want this patch after all.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On 03/23/2017 01:10 PM, Eric Blake wrote:

>>  # @RbdAuthMethod:
>>  #
>>  # An enumeration of rados auth_supported types
>>  #
>>  # Since: 2.9
>>  ##
>> -{ 'struct': 'RbdAuthMethod',
>> -  'data': { 'auth': 'RbdAuthSupport' } }
>> +{ 'enum': 'RbdAuthMethod',
>> +  'data': [ 'cephx', 'none' ] }
> 
> Changes BlockdevOptionsRbd from:
> 
> ..., "auth-supported": [ { "auth": "none" } ],

Another question - why does auth-supported take an array?  Can you
really pass both 'none' and 'cephx' at the same time? Or can you only
pass an array of at most one element, at which point why is it an array?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4136 bytes --]

On 03/23/2017 03:59 PM, Eric Blake wrote:
> On 03/23/2017 01:10 PM, Eric Blake wrote:
> 
>>>  # @RbdAuthMethod:
>>>  #
>>>  # An enumeration of rados auth_supported types
>>>  #
>>>  # Since: 2.9
>>>  ##
>>> -{ 'struct': 'RbdAuthMethod',
>>> -  'data': { 'auth': 'RbdAuthSupport' } }
>>> +{ 'enum': 'RbdAuthMethod',
>>> +  'data': [ 'cephx', 'none' ] }
>>
>> Changes BlockdevOptionsRbd from:
>>
>> ..., "auth-supported": [ { "auth": "none" } ],
> 
> Another question - why does auth-supported take an array?  Can you
> really pass both 'none' and 'cephx' at the same time? Or can you only
> pass an array of at most one element, at which point why is it an array?

In fact, the more I look at this, the more I wonder if we really want
'auth' to be a flat union and move the 'password-secret' key to be part
of the cephx authorization scheme, since 'password-secret' only makes
sense with 'cephx', and not with 'none'.  Jeff pointed out to me that
libvirt is currently passing both at once; qemuBuildRBDSecinfoURI():

static int
qemuBuildRBDSecinfoURI(virBufferPtr buf,
                       qemuDomainSecretInfoPtr secinfo)
{
    char *base64secret = NULL;

    if (!secinfo) {
        virBufferAddLit(buf, ":auth_supported=none");
        return 0;
    }

    switch ((qemuDomainSecretInfoType) secinfo->type) {
    case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN:
        if (!(base64secret = virStringEncodeBase64(secinfo->s.plain.secret,

secinfo->s.plain.secretlen)))
            return -1;
        virBufferEscape(buf, '\\', ":", ":id=%s",
secinfo->s.plain.username);
        virBufferEscape(buf, '\\', ":",
                        ":key=%s:auth_supported=cephx\\;none",
                        base64secret);

But maybe what that _really_ means is that librados is letting us say
"I'd really prefer cephx authorization, and here's my key; but if you
can't honor that, I'm also okay with a fallback to no auth".

That feels wrong to me. If you've gone to the bother of providing an
encryption key, falling back to none should probably be an error.

So my proposal is to have:

{ 'enum': 'RbdAuthSupport', 'data': [ 'cephx', 'none' ] }
{ 'struct': 'RbdAuthNone', 'data': { } }
{ 'struct': 'RbdAuthCephx', 'data': { 'password-secret': 'str' } }
{ 'union', 'RbdAuthMethod', 'base': { 'type': 'RbdAuthSupport' },
  'discriminator': 'type',
  'data': { 'none': 'RbdAuthNone',
            'cephx': 'RbdAuthCephx' } } }
{ 'struct': 'BlockdevOptionsRbd',
  'data': { 'pool': 'str',
            'image': 'str',
            '*conf': 'str',
            '*snapshot': 'str',
            '*user': 'str',
            '*server': ['InetSocketAddress'],
            '*auth': 'RbdAuthMethod' } }

so that you pass at most one auth method, looking like:

..., "image": ..., "auth": { "type": "none" }

or like:

..., "image": ..., "auth": { "type": "cephx", "password-secret": "..." }

note that password-secret is NOT at the same level as image, so from the
command line, supporting either old-style insecure 'key=' or new-style
'password-secret' at the top-level of the QemuOpts will have to have
glue code that maps it to the right nested location within the QAPI
type.  If we like it, we'd have to get it implemented before 2.9 bakes
in any other design.

We'd still have to allow libvirt's use of
":key=...:auth_supported=cephx\\;none" on the command line, but from the
QMP side, we would support exactly one auth method, and libvirt will be
updated to match when it starts targetting blockdev-add instead of
legacy command line.

If librados really needs 'cephx;none' any time cephx authorization is
requested, then even though the QMP only requests 'cephx', we can still
map it to 'cephx;none' in qemu - but I'm hoping that setting
auth_supported=cephx rather than auth_supported=cephx;none on the
librados side gives us what we (and libvirt) really want in the first place.

Jeff or Josh, if you could chime in, that would be helpful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On 03/23/2017 04:43 PM, Eric Blake wrote:

> We'd still have to allow libvirt's use of
> ":key=...:auth_supported=cephx\\;none" on the command line, but from the
> QMP side, we would support exactly one auth method, and libvirt will be
> updated to match when it starts targetting blockdev-add instead of
> legacy command line.
> 
> If librados really needs 'cephx;none' any time cephx authorization is
> requested, then even though the QMP only requests 'cephx', we can still
> map it to 'cephx;none' in qemu - but I'm hoping that setting
> auth_supported=cephx rather than auth_supported=cephx;none on the
> librados side gives us what we (and libvirt) really want in the first place.

Or, if it becomes something that libvirt wants to allow users to tune in
their XML (right now, users don't get a choice, they get either 'none'
or 'cephx;none'), a forward-compatible solution under my QMP proposal
would be to have qemu add a third enum state, "none", "cephx", and
"cephx-no-fallback".

On the other hand, if supporting multiple methods at once makes sense
(say librados adds a 'cephy' method, and that users could legitimately
use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
keeping auth as an array of instances of a simple flat union scales
nicer than having to add a combinatorial explosion of branches to the
flat union to cover every useful combination of auth_supported methods.
Maybe I'm overthinking it.

> 
> Jeff or Josh, if you could chime in, that would be helpful.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Jeff Cody]

On Thu, Mar 23, 2017 at 04:56:30PM -0500, Eric Blake wrote:
> On 03/23/2017 04:43 PM, Eric Blake wrote:
> 
> > We'd still have to allow libvirt's use of
> > ":key=...:auth_supported=cephx\\;none" on the command line, but from the
> > QMP side, we would support exactly one auth method, and libvirt will be
> > updated to match when it starts targetting blockdev-add instead of
> > legacy command line.
> > 
> > If librados really needs 'cephx;none' any time cephx authorization is
> > requested, then even though the QMP only requests 'cephx', we can still
> > map it to 'cephx;none' in qemu - but I'm hoping that setting
> > auth_supported=cephx rather than auth_supported=cephx;none on the
> > librados side gives us what we (and libvirt) really want in the first place.
> 
> Or, if it becomes something that libvirt wants to allow users to tune in
> their XML (right now, users don't get a choice, they get either 'none'
> or 'cephx;none'), a forward-compatible solution under my QMP proposal
> would be to have qemu add a third enum state, "none", "cephx", and
> "cephx-no-fallback".
> 
> On the other hand, if supporting multiple methods at once makes sense
> (say librados adds a 'cephy' method, and that users could legitimately
> use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
> keeping auth as an array of instances of a simple flat union scales
> nicer than having to add a combinatorial explosion of branches to the
> flat union to cover every useful combination of auth_supported methods.
> Maybe I'm overthinking it.
> 

I spoke over email with Jason Dillaman (cc'ed), and this is what he told me
with regards to the authentication methods, and what cephx;none means:

On Thu, Mar 23, 2017 at 06:04:30PM -0400, Jason Dillaman wrote:
> It's saying that the client is willing to connect to a server that
> uses cephx auth or a server that uses no auth. Looking at the code,
> the "auth_supported" configuration key is actually deprecated and
> "auth_client_required" should be used instead (which defaults to
> 'cephx, none' already). Since it's been deprecated since v0.55 and if
> you are cleaning things up, feel free to switch to the new one if
> possible.

So from what Jason is saying, it seems like the conclusion we reached over
IRC is correct: it will attempt cephx but also fallback to no auth.

Also, since the preferred auth option may be named different depending on
ceph versions, I know think we probably should not tie the QAPI parameter to
the name of the older deprecated option.

I suggest that the 'auth_supported' parameter for QAPI be renamed to
'auth_method'.  If you don't like the array and the flexibility it provides
at the cost of complexity, I think a flat enum consisting of 3 values like
you mentioned is reasonable.  Since the QAPI does not need to map to the
legacy commandline used by libvirt, I would suggest maybe naming them
slightly different, though: any, none, strict

For 2.9, they could each map to 'auth_supported' like so:
    
    any:     "cephx;none"
    none:    "none"
    strict:  "cephx"

For 2.10, we could try to discover if 'auth_client_required' is supported or
not, and use either it or 'auth_supported' as appropriate (with the same
mappings as above).

The reason I like "any" and "strict", is it gives consistent meanings to
options even if new auth methods are introduced.  For a hypothetical "cephy"
method example:

    any:     "cephy;cephx;none"
    none:    "none"
    strict:  "cephy;cephx"

-Jeff

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Markus Armbruster]

Adding Daniel Berrange for QCryptoSecret expertise.

Jeff Cody <jcody@redhat.com> writes:

> On Thu, Mar 23, 2017 at 04:56:30PM -0500, Eric Blake wrote:
>> On 03/23/2017 04:43 PM, Eric Blake wrote:
>> 
>> > We'd still have to allow libvirt's use of
>> > ":key=...:auth_supported=cephx\\;none" on the command line, but from the
>> > QMP side, we would support exactly one auth method, and libvirt will be
>> > updated to match when it starts targetting blockdev-add instead of
>> > legacy command line.
>> > 
>> > If librados really needs 'cephx;none' any time cephx authorization is
>> > requested, then even though the QMP only requests 'cephx', we can still
>> > map it to 'cephx;none' in qemu - but I'm hoping that setting
>> > auth_supported=cephx rather than auth_supported=cephx;none on the
>> > librados side gives us what we (and libvirt) really want in the first place.
>> 
>> Or, if it becomes something that libvirt wants to allow users to tune in
>> their XML (right now, users don't get a choice, they get either 'none'
>> or 'cephx;none'), a forward-compatible solution under my QMP proposal
>> would be to have qemu add a third enum state, "none", "cephx", and
>> "cephx-no-fallback".
>> 
>> On the other hand, if supporting multiple methods at once makes sense
>> (say librados adds a 'cephy' method, and that users could legitimately
>> use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
>> keeping auth as an array of instances of a simple flat union scales
>> nicer than having to add a combinatorial explosion of branches to the
>> flat union to cover every useful combination of auth_supported methods.
>> Maybe I'm overthinking it.
>> 
>
> I spoke over email with Jason Dillaman (cc'ed), and this is what he told me
> with regards to the authentication methods, and what cephx;none means:
>
> On Thu, Mar 23, 2017 at 06:04:30PM -0400, Jason Dillaman wrote:
>> It's saying that the client is willing to connect to a server that
>> uses cephx auth or a server that uses no auth. Looking at the code,
>> the "auth_supported" configuration key is actually deprecated and
>> "auth_client_required" should be used instead (which defaults to
>> 'cephx, none' already). Since it's been deprecated since v0.55 and if
>> you are cleaning things up, feel free to switch to the new one if
>> possible.
>
> So from what Jason is saying, it seems like the conclusion we reached over
> IRC is correct: it will attempt cephx but also fallback to no auth.

So the client offers the server a list of authentication methods with
credentials, and the server picks one it likes.  Makes sense to me.

Inluding "none" in the default less so, but that's of no concern to the
QMP interface, so let's ignore it for now.

> Also, since the preferred auth option may be named different depending on
> ceph versions, I know think we probably should not tie the QAPI parameter to
> the name of the older deprecated option.

Yes.

> I suggest that the 'auth_supported' parameter for QAPI be renamed to
> 'auth_method'.  If you don't like the array and the flexibility it provides
> at the cost of complexity, I think a flat enum consisting of 3 values like
> you mentioned is reasonable.  Since the QAPI does not need to map to the
> legacy commandline used by libvirt, I would suggest maybe naming them
> slightly different, though: any, none, strict
>
> For 2.9, they could each map to 'auth_supported' like so:
>     
>     any:     "cephx;none"
>     none:    "none"
>     strict:  "cephx"
>
> For 2.10, we could try to discover if 'auth_client_required' is supported or
> not, and use either it or 'auth_supported' as appropriate (with the same
> mappings as above).
>
> The reason I like "any" and "strict", is it gives consistent meanings to
> options even if new auth methods are introduced.  For a hypothetical "cephy"
> method example:
>
>     any:     "cephy;cephx;none"
>     none:    "none"
>     strict:  "cephy;cephx"

Two years later, an unfixable cryptograhic weakness in cephx is
discovered.  Some users really, really want to select only "cephy", but
they can't.  We react by pushing out a QEMU update adding method
"stricter", which expands into "cephy".  Libvirt then reacts and pushes
out an update.  And so forth, up the stack.  Many moons later, users can
actually select "cephy".  Thanks, but no thanks.

I think we should expose the current, recommended way to configure
authentication, in a form that is suitable for QAPI/QMP, i.e. structured
data as JSON objects, not strings you need to parse.

If a future authentication method might need something else than a
QCryptoSecret object, we need to take Eric's proposal and make this an
array of unions, where the union tag is the method, and the variant
members supply whatever data the method needs (none: nothing, cephx: a
QCryptoSecret).  Member @password-secret goes away.

Else, we can make it an array of methods, and keep member
@password-secret.

We need to decide quickly to keep rbd fully supported in 2.9.

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Jeff Cody]

On Fri, Mar 24, 2017 at 08:05:49AM +0100, Markus Armbruster wrote:
> Adding Daniel Berrange for QCryptoSecret expertise.
> 
> Jeff Cody <jcody@redhat.com> writes:
> 
> > On Thu, Mar 23, 2017 at 04:56:30PM -0500, Eric Blake wrote:
> >> On 03/23/2017 04:43 PM, Eric Blake wrote:
> >> 
> >> > We'd still have to allow libvirt's use of
> >> > ":key=...:auth_supported=cephx\\;none" on the command line, but from the
> >> > QMP side, we would support exactly one auth method, and libvirt will be
> >> > updated to match when it starts targetting blockdev-add instead of
> >> > legacy command line.
> >> > 
> >> > If librados really needs 'cephx;none' any time cephx authorization is
> >> > requested, then even though the QMP only requests 'cephx', we can still
> >> > map it to 'cephx;none' in qemu - but I'm hoping that setting
> >> > auth_supported=cephx rather than auth_supported=cephx;none on the
> >> > librados side gives us what we (and libvirt) really want in the first place.
> >> 
> >> Or, if it becomes something that libvirt wants to allow users to tune in
> >> their XML (right now, users don't get a choice, they get either 'none'
> >> or 'cephx;none'), a forward-compatible solution under my QMP proposal
> >> would be to have qemu add a third enum state, "none", "cephx", and
> >> "cephx-no-fallback".
> >> 
> >> On the other hand, if supporting multiple methods at once makes sense
> >> (say librados adds a 'cephy' method, and that users could legitimately
> >> use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
> >> keeping auth as an array of instances of a simple flat union scales
> >> nicer than having to add a combinatorial explosion of branches to the
> >> flat union to cover every useful combination of auth_supported methods.
> >> Maybe I'm overthinking it.
> >> 
> >
> > I spoke over email with Jason Dillaman (cc'ed), and this is what he told me
> > with regards to the authentication methods, and what cephx;none means:
> >
> > On Thu, Mar 23, 2017 at 06:04:30PM -0400, Jason Dillaman wrote:
> >> It's saying that the client is willing to connect to a server that
> >> uses cephx auth or a server that uses no auth. Looking at the code,
> >> the "auth_supported" configuration key is actually deprecated and
> >> "auth_client_required" should be used instead (which defaults to
> >> 'cephx, none' already). Since it's been deprecated since v0.55 and if
> >> you are cleaning things up, feel free to switch to the new one if
> >> possible.
> >
> > So from what Jason is saying, it seems like the conclusion we reached over
> > IRC is correct: it will attempt cephx but also fallback to no auth.
> 
> So the client offers the server a list of authentication methods with
> credentials, and the server picks one it likes.  Makes sense to me.
> 
> Inluding "none" in the default less so, but that's of no concern to the
> QMP interface, so let's ignore it for now.
> 
> > Also, since the preferred auth option may be named different depending on
> > ceph versions, I know think we probably should not tie the QAPI parameter to
> > the name of the older deprecated option.
> 
> Yes.
> 
> > I suggest that the 'auth_supported' parameter for QAPI be renamed to
> > 'auth_method'.  If you don't like the array and the flexibility it provides
> > at the cost of complexity, I think a flat enum consisting of 3 values like
> > you mentioned is reasonable.  Since the QAPI does not need to map to the
> > legacy commandline used by libvirt, I would suggest maybe naming them
> > slightly different, though: any, none, strict
> >
> > For 2.9, they could each map to 'auth_supported' like so:
> >     
> >     any:     "cephx;none"
> >     none:    "none"
> >     strict:  "cephx"
> >
> > For 2.10, we could try to discover if 'auth_client_required' is supported or
> > not, and use either it or 'auth_supported' as appropriate (with the same
> > mappings as above).
> >
> > The reason I like "any" and "strict", is it gives consistent meanings to
> > options even if new auth methods are introduced.  For a hypothetical "cephy"
> > method example:
> >
> >     any:     "cephy;cephx;none"
> >     none:    "none"
> >     strict:  "cephy;cephx"
> 
> Two years later, an unfixable cryptograhic weakness in cephx is
> discovered.  Some users really, really want to select only "cephy", but
> they can't.  We react by pushing out a QEMU update adding method
> "stricter", which expands into "cephy".  Libvirt then reacts and pushes
> out an update.  And so forth, up the stack.  Many moons later, users can
> actually select "cephy".  Thanks, but no thanks.
> 

Good point; it could be dealt with, but only clumsily.


> I think we should expose the current, recommended way to configure
> authentication, in a form that is suitable for QAPI/QMP, i.e. structured
> data as JSON objects, not strings you need to parse.
> 
> If a future authentication method might need something else than a
> QCryptoSecret object, we need to take Eric's proposal and make this an
> array of unions, where the union tag is the method, and the variant
> members supply whatever data the method needs (none: nothing, cephx: a
> QCryptoSecret).  Member @password-secret goes away.
> 

I guess it is always technically a possibility that a new method would need
something different.  Different methods may of course change the assumptions
about the requirement of a 'user' or not, etc.., as well.

> Else, we can make it an array of methods, and keep member
> @password-secret.
> 

Do you mean keep it as it currently is (just renamed)?

One new bit we should add in, if we stay with this method, is to throw an
error if user/password-secret is provided but only "none" is selected as the
auth method.

The current method essentially pushes the decision of the correct
combination of methods to use to libvirt, which feels like the right place
to be making that decision anyway.


> We need to decide quickly to keep rbd fully supported in 2.9.

Agree.  My preference is to leave it as an array of methods, so long as that
is tenable to libvirt.


-Jeff

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On 03/24/2017 07:42 AM, Jeff Cody wrote:

> Agree.  My preference is to leave it as an array of methods, so long as that
> is tenable to libvirt.

The -drive syntax should remain unchanged (that's an absolute must for
libvirt).  But the QMP syntax being an array of methods sounds best to
me, and I think password-secret should be part of the array.  So my vote
would be for:

{ "driver": "rbd", "image": "foo",
  "auth": [ { "type": "cephx", "password-secret": "sec0" },
            { "type": "none" } ],
  "pool": "bar" }

It makes mapping -drive arguments into QMP form a bit trickier, but the
QMP form is then easily extensible if we add another auth method down
the road.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Jeff Cody]

On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
> On 03/24/2017 07:42 AM, Jeff Cody wrote:
> 
> > Agree.  My preference is to leave it as an array of methods, so long as that
> > is tenable to libvirt.
> 
> The -drive syntax should remain unchanged (that's an absolute must for
> libvirt).  But the QMP syntax being an array of methods sounds best to
> me, and I think password-secret should be part of the array.  So my vote
> would be for:
> 
> { "driver": "rbd", "image": "foo",
>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>             { "type": "none" } ],
>   "pool": "bar" }
> 
> It makes mapping -drive arguments into QMP form a bit trickier, but the
> QMP form is then easily extensible if we add another auth method down
> the road.
> 

In that case, what about adding user as well, and not just password-secret?

Jeff

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

On 03/24/2017 09:10 AM, Jeff Cody wrote:
> On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
>> On 03/24/2017 07:42 AM, Jeff Cody wrote:
>>
>>> Agree.  My preference is to leave it as an array of methods, so long as that
>>> is tenable to libvirt.
>>
>> The -drive syntax should remain unchanged (that's an absolute must for
>> libvirt).  But the QMP syntax being an array of methods sounds best to
>> me, and I think password-secret should be part of the array.  So my vote
>> would be for:
>>
>> { "driver": "rbd", "image": "foo",
>>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>>             { "type": "none" } ],
>>   "pool": "bar" }
>>
>> It makes mapping -drive arguments into QMP form a bit trickier, but the
>> QMP form is then easily extensible if we add another auth method down
>> the road.
>>
> 
> In that case, what about adding user as well, and not just password-secret?

Makes sense - anything that is associated solely with cephx should
belong to the same flat-union branch for cephx, rather than at the top
level.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/24/2017 09:10 AM, Jeff Cody wrote:
>> On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
>>> On 03/24/2017 07:42 AM, Jeff Cody wrote:
>>>
>>>> Agree.  My preference is to leave it as an array of methods, so long as that
>>>> is tenable to libvirt.
>>>
>>> The -drive syntax should remain unchanged (that's an absolute must for
>>> libvirt).  But the QMP syntax being an array of methods sounds best to
>>> me, and I think password-secret should be part of the array.  So my vote
>>> would be for:
>>>
>>> { "driver": "rbd", "image": "foo",
>>>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>>>             { "type": "none" } ],
>>>   "pool": "bar" }
>>>
>>> It makes mapping -drive arguments into QMP form a bit trickier, but the
>>> QMP form is then easily extensible if we add another auth method down
>>> the road.
>>>
>> 
>> In that case, what about adding user as well, and not just password-secret?
>
> Makes sense - anything that is associated solely with cephx should
> belong to the same flat-union branch for cephx, rather than at the top
> level.

I've thought about this some more and have come to the conclusion that
this design is clumsy and overly complex.  Moreover, I suspect our
testing has been poor.  Let me explain.

= Overview =

What we're trying to configure is authentication methods the client is
to offer to the server.

This is not a list, it's a set: the order doesn't matter, and multiple
entries of the same type make no sense.  We could reject multiple
entries, or let the last one win silently, but this is just unnecessary
complexity.

Type "cephx" uses a user name and a key.

Type "none" uses neither (it could theoretically use the user name, but
I've been assured it doesn't).

The user name defaults to "admin".

The key defaults to the user name's entry in a keyring.  There is a
configurable list of key ring files, with a default.  The default
commonly includes /etc/ceph/client.keyring.

= Librados configuration =

Librados takes these configuration bits as follows:

* The user name is to be passed to rados_create().  NULL means default
  to "admin".

* The key may be passed to rados_conf_set() with key "key" (value is the
  key) or "keyfile" (value is name of the file containing the key).
  Documentation discourages use of "key".

* The list of keyrings may passed to rados_conf_set() with key
  "keyring" and a value listing file names separated by ','.  At least
  according to the documentation; the source looks like any non-empty
  sequence of [;,= \t]* serves as separator.

* The offered authentication methods may be passed to rados_conf_set()
  with key "auth_supported" (deprecated) or "auth_client_required", and
  a value listing methods separated by "," (or maybe [;,= \t]*, see
  above).  The methods are "cephx" and "none".  Default is "cephx,none".

= Command line -drive =

With -drive file=rbd:pool/image[@snapshot][:key=value...], the
key=value... part lets you specify arbitrary rados_conf_set() settings,
except pseudo-key "id" is mapped to the user name instead, and
pseudo-key "conf" names a rados configuration file.  This overloading of
rados_conf_set() keys and our own pseudo-keys isn't pretty, but it's a
done deal.  The full set of authentication configuration discussed above
is available as keys "id", "key", "keyfile", "keyring", "auth_supported"
and "auth_client_required".  Also via "conf", of course.

These -drive rbd:...:key=value settings are also available in -drive
QemuOpts syntax -drive driver=rbd,key=value:

* Pseudo-key "id" is "user" in QemuOpts.

* Pseudo-key "conf" is "conf" in QemuOpts, too

* Any other keys together become "keyvalue-pairs" in QemuOpts, with
  the same key=value:... syntax.

Additionally, QemuOpts-only "password-secret" lets you set
rados_conf_set() key "key" to a value obtained from the QCryptoSecret
interface.

Note that @password-secret is a misnomer: this is *not* a password, it's
a *key*.  Calling it a password is confusing, and makes it harder to
mentally connect QMP and Ceph configuration.

The settings in file=rbd:... override the ones in QemuOpts (that's how
->bdrv_parse_filename() works), which in turn override (I think)
settings from a config file.  Note that *any* key other than "id" and
"conf" in file=rbd:... completely overrides *all* keys in
"keyvalue-pairs".

Except "password-secret" works the other way: it overrides "key" set in
file=rbd:... or "keyvalue-pairs".

As so often with syntactic sugar, once you actually explain how it
works, you realize what a bloody mess it is.

It's not quite clear whether "keyvalue-pairs" is really meant for the
user, or just for internal use to implement file=rbd:...  I posted a
patch to hide it from the user.

= QMP blockdev-add and command line -blockdev =

The current QAPI/QMP schema lets you specify only a few settings
directly: pseudo-keys "id" and "conf" (optional members @user and
@conf), keys "key" and "auth_supported" (optional members
@password-secret and @auth_supported).  The only way to specify other
rados_conf_set() settings such as "keyfile" and "keyring" is via "conf".

Begs the question how the settings you can specify directly interact
with the config file.  I figure they override the config file.

Let's review how this could be used.

If you specify no optional members, you get the Ceph defaults described
above.  Good.

If you want to override the default user, there's @user.  Good.

If you want to override the keyring, you have to fall back to @conf.
Not ideal, but good enough for 2.9.  I guess most users will be fine
with the default keyrings.

If you want to override the authentication methods, you have several
options:

* Use @conf and set auth_supported (deprecated) or auth_client_required
  in the config file

* Use @auth_supported like this:

      "auth_supported": [ { "auth": METHOD}, ... ]

  Clumsy.

If you want to override the key, you again have several options:

* Use @conf and set keyfile or key in the config file

* Use @password-secret to get it from the QCryptoSecret interface

  Did we actually test this?

= What to do for 2.9 =

I propose to

* drop both "auth_supported" and "password-secret" from the QAPI schema

* drop "password-secret" from QemuOpts

* hide "keyvalue-pairs" in QemuOpts

No existing usage is affected, since all these things are new in 2.9.

Users who need something else than the default keyring can continue to
configure alternate keys and keyrings with -drive, both directly with
file=rbd:...:key=value and with a config file.  With -blockdev /
blockdev_add, they need to use the config file.

We can consider improvements post 2.9.

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Jeff Cody]

On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 03/24/2017 09:10 AM, Jeff Cody wrote:
> >> On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
> >>> On 03/24/2017 07:42 AM, Jeff Cody wrote:
> >>>
> >>>> Agree.  My preference is to leave it as an array of methods, so long as that
> >>>> is tenable to libvirt.
> >>>
> >>> The -drive syntax should remain unchanged (that's an absolute must for
> >>> libvirt).  But the QMP syntax being an array of methods sounds best to
> >>> me, and I think password-secret should be part of the array.  So my vote
> >>> would be for:
> >>>
> >>> { "driver": "rbd", "image": "foo",
> >>>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
> >>>             { "type": "none" } ],
> >>>   "pool": "bar" }
> >>>
> >>> It makes mapping -drive arguments into QMP form a bit trickier, but the
> >>> QMP form is then easily extensible if we add another auth method down
> >>> the road.
> >>>
> >> 
> >> In that case, what about adding user as well, and not just password-secret?
> >
> > Makes sense - anything that is associated solely with cephx should
> > belong to the same flat-union branch for cephx, rather than at the top
> > level.
> 
> I've thought about this some more and have come to the conclusion that
> this design is clumsy and overly complex.  Moreover, I suspect our
> testing has been poor.  Let me explain.
> 
> = Overview =
> 
> What we're trying to configure is authentication methods the client is
> to offer to the server.
> 
> This is not a list, it's a set: the order doesn't matter, and multiple
> entries of the same type make no sense.  We could reject multiple
> entries, or let the last one win silently, but this is just unnecessary
> complexity.
> 
> Type "cephx" uses a user name and a key.
> 
> Type "none" uses neither (it could theoretically use the user name, but
> I've been assured it doesn't).
> 
> The user name defaults to "admin".
> 
> The key defaults to the user name's entry in a keyring.  There is a
> configurable list of key ring files, with a default.  The default
> commonly includes /etc/ceph/client.keyring.
> 

I don't think 'client.keyring' is one of the defaults.  I know
/etc/ceph/keyring is, however.


> = Librados configuration =
> 
> Librados takes these configuration bits as follows:
> 
> * The user name is to be passed to rados_create().  NULL means default
>   to "admin".
> 
> * The key may be passed to rados_conf_set() with key "key" (value is the
>   key) or "keyfile" (value is name of the file containing the key).
>   Documentation discourages use of "key".
> 
> * The list of keyrings may passed to rados_conf_set() with key
>   "keyring" and a value listing file names separated by ','.  At least
>   according to the documentation; the source looks like any non-empty
>   sequence of [;,= \t]* serves as separator.
> 
> * The offered authentication methods may be passed to rados_conf_set()
>   with key "auth_supported" (deprecated) or "auth_client_required", and
>   a value listing methods separated by "," (or maybe [;,= \t]*, see
>   above).  The methods are "cephx" and "none".  Default is "cephx,none".
> 
> = Command line -drive =
> 
> With -drive file=rbd:pool/image[@snapshot][:key=value...], the
> key=value... part lets you specify arbitrary rados_conf_set() settings,
> except pseudo-key "id" is mapped to the user name instead, and
> pseudo-key "conf" names a rados configuration file.  This overloading of
> rados_conf_set() keys and our own pseudo-keys isn't pretty, but it's a
> done deal.  The full set of authentication configuration discussed above
> is available as keys "id", "key", "keyfile", "keyring", "auth_supported"
> and "auth_client_required".  Also via "conf", of course.
> 
> These -drive rbd:...:key=value settings are also available in -drive
> QemuOpts syntax -drive driver=rbd,key=value:
> 
> * Pseudo-key "id" is "user" in QemuOpts.
> 
> * Pseudo-key "conf" is "conf" in QemuOpts, too
> 
> * Any other keys together become "keyvalue-pairs" in QemuOpts, with
>   the same key=value:... syntax.
> 
> Additionally, QemuOpts-only "password-secret" lets you set
> rados_conf_set() key "key" to a value obtained from the QCryptoSecret
> interface.
> 
> Note that @password-secret is a misnomer: this is *not* a password, it's
> a *key*.  Calling it a password is confusing, and makes it harder to
> mentally connect QMP and Ceph configuration.
> 

Good point; @key-secret would be a better name


> The settings in file=rbd:... override the ones in QemuOpts (that's how
> ->bdrv_parse_filename() works), which in turn override (I think)
> settings from a config file.  Note that *any* key other than "id" and
> "conf" in file=rbd:... completely overrides *all* keys in
> "keyvalue-pairs".
> 
> Except "password-secret" works the other way: it overrides "key" set in
> file=rbd:... or "keyvalue-pairs".
> 
> As so often with syntactic sugar, once you actually explain how it
> works, you realize what a bloody mess it is.
> 
> It's not quite clear whether "keyvalue-pairs" is really meant for the
> user, or just for internal use to implement file=rbd:...  I posted a
> patch to hide it from the user.
> 
> = QMP blockdev-add and command line -blockdev =
> 
> The current QAPI/QMP schema lets you specify only a few settings
> directly: pseudo-keys "id" and "conf" (optional members @user and
> @conf), keys "key" and "auth_supported" (optional members
> @password-secret and @auth_supported).  The only way to specify other
> rados_conf_set() settings such as "keyfile" and "keyring" is via "conf".
> 
> Begs the question how the settings you can specify directly interact
> with the config file.  I figure they override the config file.
> 

Yes, looking at the code rados_conf_set() will override both the config
file, and environment variables.


However, certain environment variables will override settings in the
specified "conf" file.  I think "CEPH_KEYRING" is the only one (corresponds
to "keyring"), but I am not sure there are not others.  

This is probably a reason to provide an option for "keyring" via QAPI.

> Let's review how this could be used.
> 
> If you specify no optional members, you get the Ceph defaults described
> above.  Good.
> 
> If you want to override the default user, there's @user.  Good.
> 
> If you want to override the keyring, you have to fall back to @conf.
> Not ideal, but good enough for 2.9.  I guess most users will be fine
> with the default keyrings.
> 

Unless the environment variable CEPH_KEYRING is set, at which point "conf"
won't override it, unless I am missing something.  However, we could either
provide a QAPI interface to set the keyring, or keep a user/key option.


> If you want to override the authentication methods, you have several
> options:
> 
> * Use @conf and set auth_supported (deprecated) or auth_client_required
>   in the config file
> 
> * Use @auth_supported like this:
> 
>       "auth_supported": [ { "auth": METHOD}, ... ]
> 
>   Clumsy.
> 
> If you want to override the key, you again have several options:
> 
> * Use @conf and set keyfile or key in the config file
> 
> * Use @password-secret to get it from the QCryptoSecret interface
> 
>   Did we actually test this?

Yes, and it works.  One caveat: the key obtained from the keyring or
"ceph auth list" are base64 encoded, and qemu_rbd_set_auth() base64 encodes
the key itself into base64.  The rados_conf_set() value for "key" is assumed
to be base64 encoded.  This means the secret file needs to have the key not
base64 encoded, with the current code.


> 
> = What to do for 2.9 =
> 
> I propose to
> 
> * drop both "auth_supported" and "password-secret" from the QAPI schema
> 
> * drop "password-secret" from QemuOpts
> 
> * hide "keyvalue-pairs" in QemuOpts
> 
> No existing usage is affected, since all these things are new in 2.9.
> 
> Users who need something else than the default keyring can continue to
> configure alternate keys and keyrings with -drive, both directly with
> file=rbd:...:key=value and with a config file.  With -blockdev /
> blockdev_add, they need to use the config file.
> 
> We can consider improvements post 2.9.

I am fine with this; the only real functional limitation with dropping this
for 2.9 is that default keyring cannot be overridden in certain scenarios
(it is set via an environment variable, and 'conf' won't override it).


-Jeff

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 03/24/2017 09:10 AM, Jeff Cody wrote:
>> >> On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
>> >>> On 03/24/2017 07:42 AM, Jeff Cody wrote:
>> >>>
>> >>>> Agree.  My preference is to leave it as an array of methods, so long as that
>> >>>> is tenable to libvirt.
>> >>>
>> >>> The -drive syntax should remain unchanged (that's an absolute must for
>> >>> libvirt).  But the QMP syntax being an array of methods sounds best to
>> >>> me, and I think password-secret should be part of the array.  So my vote
>> >>> would be for:
>> >>>
>> >>> { "driver": "rbd", "image": "foo",
>> >>>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>> >>>             { "type": "none" } ],
>> >>>   "pool": "bar" }
>> >>>
>> >>> It makes mapping -drive arguments into QMP form a bit trickier, but the
>> >>> QMP form is then easily extensible if we add another auth method down
>> >>> the road.
>> >>>
>> >> 
>> >> In that case, what about adding user as well, and not just password-secret?
>> >
>> > Makes sense - anything that is associated solely with cephx should
>> > belong to the same flat-union branch for cephx, rather than at the top
>> > level.
>> 
>> I've thought about this some more and have come to the conclusion that
>> this design is clumsy and overly complex.  Moreover, I suspect our
>> testing has been poor.  Let me explain.
>> 
>> = Overview =
>> 
>> What we're trying to configure is authentication methods the client is
>> to offer to the server.
>> 
>> This is not a list, it's a set: the order doesn't matter, and multiple
>> entries of the same type make no sense.  We could reject multiple
>> entries, or let the last one win silently, but this is just unnecessary
>> complexity.
>> 
>> Type "cephx" uses a user name and a key.
>> 
>> Type "none" uses neither (it could theoretically use the user name, but
>> I've been assured it doesn't).
>> 
>> The user name defaults to "admin".
>> 
>> The key defaults to the user name's entry in a keyring.  There is a
>> configurable list of key ring files, with a default.  The default
>> commonly includes /etc/ceph/client.keyring.
>> 
>
> I don't think 'client.keyring' is one of the defaults.  I know
> /etc/ceph/keyring is, however.

Double-checking...  source suggests the default is actually
/etc/ceph/$cluster.$name.keyring,/etc/ceph/$cluster.keyring,/etc/ceph/keyring,/etc/ceph/keyring.bin

https://github.com/ceph/ceph/blob/master/src/common/config_opts.h#L172

>> = Librados configuration =
>> 
>> Librados takes these configuration bits as follows:
>> 
>> * The user name is to be passed to rados_create().  NULL means default
>>   to "admin".
>> 
>> * The key may be passed to rados_conf_set() with key "key" (value is the
>>   key) or "keyfile" (value is name of the file containing the key).
>>   Documentation discourages use of "key".
>> 
>> * The list of keyrings may passed to rados_conf_set() with key
>>   "keyring" and a value listing file names separated by ','.  At least
>>   according to the documentation; the source looks like any non-empty
>>   sequence of [;,= \t]* serves as separator.
>> 
>> * The offered authentication methods may be passed to rados_conf_set()
>>   with key "auth_supported" (deprecated) or "auth_client_required", and
>>   a value listing methods separated by "," (or maybe [;,= \t]*, see
>>   above).  The methods are "cephx" and "none".  Default is "cephx,none".
>> 
>> = Command line -drive =
>> 
>> With -drive file=rbd:pool/image[@snapshot][:key=value...], the
>> key=value... part lets you specify arbitrary rados_conf_set() settings,
>> except pseudo-key "id" is mapped to the user name instead, and
>> pseudo-key "conf" names a rados configuration file.  This overloading of
>> rados_conf_set() keys and our own pseudo-keys isn't pretty, but it's a
>> done deal.  The full set of authentication configuration discussed above
>> is available as keys "id", "key", "keyfile", "keyring", "auth_supported"
>> and "auth_client_required".  Also via "conf", of course.
>> 
>> These -drive rbd:...:key=value settings are also available in -drive
>> QemuOpts syntax -drive driver=rbd,key=value:
>> 
>> * Pseudo-key "id" is "user" in QemuOpts.
>> 
>> * Pseudo-key "conf" is "conf" in QemuOpts, too
>> 
>> * Any other keys together become "keyvalue-pairs" in QemuOpts, with
>>   the same key=value:... syntax.
>> 
>> Additionally, QemuOpts-only "password-secret" lets you set
>> rados_conf_set() key "key" to a value obtained from the QCryptoSecret
>> interface.
>> 
>> Note that @password-secret is a misnomer: this is *not* a password, it's
>> a *key*.  Calling it a password is confusing, and makes it harder to
>> mentally connect QMP and Ceph configuration.
>> 
>
> Good point; @key-secret would be a better name
>
>
>> The settings in file=rbd:... override the ones in QemuOpts (that's how
>> ->bdrv_parse_filename() works), which in turn override (I think)
>> settings from a config file.  Note that *any* key other than "id" and
>> "conf" in file=rbd:... completely overrides *all* keys in
>> "keyvalue-pairs".
>> 
>> Except "password-secret" works the other way: it overrides "key" set in
>> file=rbd:... or "keyvalue-pairs".
>> 
>> As so often with syntactic sugar, once you actually explain how it
>> works, you realize what a bloody mess it is.
>> 
>> It's not quite clear whether "keyvalue-pairs" is really meant for the
>> user, or just for internal use to implement file=rbd:...  I posted a
>> patch to hide it from the user.
>> 
>> = QMP blockdev-add and command line -blockdev =
>> 
>> The current QAPI/QMP schema lets you specify only a few settings
>> directly: pseudo-keys "id" and "conf" (optional members @user and
>> @conf), keys "key" and "auth_supported" (optional members
>> @password-secret and @auth_supported).  The only way to specify other
>> rados_conf_set() settings such as "keyfile" and "keyring" is via "conf".
>> 
>> Begs the question how the settings you can specify directly interact
>> with the config file.  I figure they override the config file.
>> 
>
> Yes, looking at the code rados_conf_set() will override both the config
> file, and environment variables.
>
>
> However, certain environment variables will override settings in the
> specified "conf" file.  I think "CEPH_KEYRING" is the only one (corresponds
> to "keyring"), but I am not sure there are not others.  
>
> This is probably a reason to provide an option for "keyring" via QAPI.

I'm open to provide more configuration knobs via QAPI, but for 2.9, I
propose to stick to the smallest interface that can possibly work.

>> Let's review how this could be used.
>> 
>> If you specify no optional members, you get the Ceph defaults described
>> above.  Good.
>> 
>> If you want to override the default user, there's @user.  Good.
>> 
>> If you want to override the keyring, you have to fall back to @conf.
>> Not ideal, but good enough for 2.9.  I guess most users will be fine
>> with the default keyrings.
>> 
>
> Unless the environment variable CEPH_KEYRING is set, at which point "conf"
> won't override it, unless I am missing something.  However, we could either
> provide a QAPI interface to set the keyring, or keep a user/key option.

For 2.9 QAPI/QMP, I'd say our story for authentication should be "stick
to *Ceph* configuration; how you use it is up to you".  I.e. set up
suitable keyrings, and how exactly you tell Ceph to use non-default
keyrings (config file, environment variable, whatever) is your problem.

>> If you want to override the authentication methods, you have several
>> options:
>> 
>> * Use @conf and set auth_supported (deprecated) or auth_client_required
>>   in the config file
>> 
>> * Use @auth_supported like this:
>> 
>>       "auth_supported": [ { "auth": METHOD}, ... ]
>> 
>>   Clumsy.
>> 
>> If you want to override the key, you again have several options:
>> 
>> * Use @conf and set keyfile or key in the config file
>> 
>> * Use @password-secret to get it from the QCryptoSecret interface
>> 
>>   Did we actually test this?
>
> Yes, and it works.  One caveat: the key obtained from the keyring or
> "ceph auth list" are base64 encoded, and qemu_rbd_set_auth() base64 encodes
> the key itself into base64.  The rados_conf_set() value for "key" is assumed
> to be base64 encoded.  This means the secret file needs to have the key not
> base64 encoded, with the current code.

Good to know, thanks.  Documentation or at least comments explaining
this would be welcome.

>> = What to do for 2.9 =
>> 
>> I propose to
>> 
>> * drop both "auth_supported" and "password-secret" from the QAPI schema
>> 
>> * drop "password-secret" from QemuOpts
>> 
>> * hide "keyvalue-pairs" in QemuOpts
>> 
>> No existing usage is affected, since all these things are new in 2.9.
>> 
>> Users who need something else than the default keyring can continue to
>> configure alternate keys and keyrings with -drive, both directly with
>> file=rbd:...:key=value and with a config file.  With -blockdev /
>> blockdev_add, they need to use the config file.
>> 
>> We can consider improvements post 2.9.
>
> I am fine with this; the only real functional limitation with dropping this
> for 2.9 is that default keyring cannot be overridden in certain scenarios
> (it is set via an environment variable, and 'conf' won't override it).

My PATCH RFC v3 implements this, let's get it reviewed and merged.

Regarding the limitation: if you pass an environment variable to QEMU,
then realize it gets in your way, consider not passing it :)

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Daniel P. Berrange]

On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
> = What to do for 2.9 =
> 
> I propose to
> 
> * drop both "auth_supported" and "password-secret" from the QAPI schema
> 
> * drop "password-secret" from QemuOpts
> 
> * hide "keyvalue-pairs" in QemuOpts
> 
> No existing usage is affected, since all these things are new in 2.9.

Maybe I'm mis-understanding what you're suggesting wrt QemuOpts, but
'password-secret' with RBD is not new in 2.9.0

It was added in 2.6.0 in this commit:

commit 60390a2192e7b38aee18db6ce7fb740498709737
Author: Daniel P. Berrange <berrange@redhat.com>
Date:   Thu Jan 21 14:19:19 2016 +0000

    rbd: add support for getting password from QCryptoSecret object
    
    Currently RBD passwords must be provided on the command line
    via
    
      $QEMU -drive file=rbd:pool/image:id=myname:\
                   key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
                   auth_supported=cephx
    
    This is insecure because the key is visible in the OS process
    listing.
    
    This adds support for an 'password-secret' parameter in the RBD
    parameters that can be used with the QCryptoSecret object to
    provide the password via a file:
    
      echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
      $QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
            -drive driver=rbd,filename=rbd:pool/image:id=myname:\
                   auth_supported=cephx,password-secret=secret0
    
    Reviewed-by: Josh Durgin <jdurgin@redhat.com>
    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
    Message-id: 1453385961-10718-2-git-send-email-berrange@redhat.com
    Signed-off-by: Jeff Cody <jcody@redhat.com>

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 for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On 04/03/2017 06:25 AM, Daniel P. Berrange wrote:
> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> = What to do for 2.9 =
>>
>> I propose to
>>
>> * drop both "auth_supported" and "password-secret" from the QAPI schema
>>
>> * drop "password-secret" from QemuOpts
>>
>> * hide "keyvalue-pairs" in QemuOpts
>>
>> No existing usage is affected, since all these things are new in 2.9.
> 
> Maybe I'm mis-understanding what you're suggesting wrt QemuOpts, but
> 'password-secret' with RBD is not new in 2.9.0

We updated things in the respin of this series; the actual committed
version (ending in commit d1c13688) did NOT kill password-secret from
QemuOpts, but merely removed it from QMP (it was the QMP portion that
was attempted to be new in 2.9).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Kevin Wolf]

Am 24.03.2017 um 14:49 hat Eric Blake geschrieben:
> On 03/24/2017 07:42 AM, Jeff Cody wrote:
> 
> > Agree.  My preference is to leave it as an array of methods, so long as that
> > is tenable to libvirt.
> 
> The -drive syntax should remain unchanged (that's an absolute must for
> libvirt).  But the QMP syntax being an array of methods sounds best to
> me, and I think password-secret should be part of the array.  So my vote
> would be for:
> 
> { "driver": "rbd", "image": "foo",
>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>             { "type": "none" } ],
>   "pool": "bar" }
> 
> It makes mapping -drive arguments into QMP form a bit trickier, but the
> QMP form is then easily extensible if we add another auth method down
> the road.

Not sure if anybody cares, but I came to the same conclusion, so I like
this.

Kevin

Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Kevin Wolf]

Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
> BlockdevOptionsRbd member auth-supported is a list of struct
> RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
> reason for wrapping the enum in a struct.  Delete the wrapper, and
> rename the enum.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH for-2.9 5/5] rbd: Reject options server.*.{numeric, to, ipv4, ipv6}

[Markus Armbruster]

We use InetSocketAddress in the QAPI schema.  However, the code
doesn't use inet_connect_saddr(), but formats "host" and "port" into a
configuration string for rados_conf_set().  Thus, members "numeric",
"to", "ipv4" and "ipv6" are silently ignored.  Not nice.

Factor a suitable InetSocketAddressBase out of InetSocketAddress, and
use that.  "numeric", "to", "ipv4" and "ipv6" are now rejected.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json     | 21 ++++++++++++++-------
 qapi/block-core.json |  2 +-
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 68a4327..b921994 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4051,19 +4051,27 @@
   'data': [ 'all', 'rx', 'tx' ] }
 
 ##
+# @InetSocketAddressBase:
+#
+# @host: host part of the address
+# @port: port part of the address
+##
+{ 'struct': 'InetSocketAddressBase',
+  'data': {
+    'host': 'str',
+    'port': 'str' } }
+
+##
 # @InetSocketAddress:
 #
 # Captures a socket address or address range in the Internet namespace.
 #
-# @host: host part of the address
-#
-# @port: port part of the address, or lowest port if @to is present
-#
 # @numeric: true if the host/port are guaranteed to be numeric,
 #           false if name resolution should be attempted. Defaults to false.
 #           (Since 2.9)
 #
-# @to: highest port to try
+# @to: If present, this is range of possible addresses, with port
+#      between @port and @to.
 #
 # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
 #
@@ -4072,9 +4080,8 @@
 # Since: 1.3
 ##
 { 'struct': 'InetSocketAddress',
+  'base': 'InetSocketAddressBase',
   'data': {
-    'host': 'str',
-    'port': 'str',
     '*numeric':  'bool',
     '*to': 'uint16',
     '*ipv4': 'bool',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fe1e40f..a57c9e8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2641,7 +2641,7 @@
             '*conf': 'str',
             '*snapshot': 'str',
             '*user': 'str',
-            '*server': ['InetSocketAddress'],
+            '*server': ['InetSocketAddressBase'],
             '*auth-supported': ['RbdAuthMethod'],
             '*password-secret': 'str' } }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 5/5] rbd: Reject options server.*.{numeric, to, ipv4, ipv6}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2310 bytes --]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> We use InetSocketAddress in the QAPI schema.  However, the code
> doesn't use inet_connect_saddr(), but formats "host" and "port" into a
> configuration string for rados_conf_set().  Thus, members "numeric",
> "to", "ipv4" and "ipv6" are silently ignored.  Not nice.
> 
> Factor a suitable InetSocketAddressBase out of InetSocketAddress, and
> use that.  "numeric", "to", "ipv4" and "ipv6" are now rejected.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi-schema.json     | 21 ++++++++++++++-------
>  qapi/block-core.json |  2 +-
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 68a4327..b921994 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4051,19 +4051,27 @@
>    'data': [ 'all', 'rx', 'tx' ] }
>  
>  ##
> +# @InetSocketAddressBase:
> +#
> +# @host: host part of the address
> +# @port: port part of the address
> +##

No Since: - but we've argued that 'Since' on types is somewhat awkward
anyways, so I'm fine with it.

> +{ 'struct': 'InetSocketAddressBase',
> +  'data': {
> +    'host': 'str',
> +    'port': 'str' } }
> +
> +##

> @@ -4072,9 +4080,8 @@
>  # Since: 1.3
>  ##
>  { 'struct': 'InetSocketAddress',
> +  'base': 'InetSocketAddressBase',
>    'data': {
> -    'host': 'str',
> -    'port': 'str',
>      '*numeric':  'bool',
>      '*to': 'uint16',
>      '*ipv4': 'bool',

Slick example of how to split out the common portion into a base class
when refactoring (now I can point the thread on IOThrottle back to this
patch).

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index fe1e40f..a57c9e8 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2641,7 +2641,7 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> -            '*server': ['InetSocketAddress'],
> +            '*server': ['InetSocketAddressBase'],
>              '*auth-supported': ['RbdAuthMethod'],
>              '*password-secret': 'str' } }
>  
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]