[Qemu-devel] [PATCH v3 0/5] RBD: blockdev-add (for 2.9?)

[Qemu-devel] [PATCH v3 0/5] RBD: blockdev-add (for 2.9?)

[Jeff Cody]

This series adds blockdev-add for rbd.


Changes from v2:

Patch 2: Updated commit message, and documented the runtime opts
         (Thanks Eric)

Patch 3: Fixed commit type, added "FIXME" in ugly string concat spot
         (Thanks Eric)

Patch 4: Fixed all the nits - deleted lines, spaces.  Kept list 
         alphabetical.  (Thanks Eric)

Patch 5: Significant changes.  Both 'mon_host' became 'server', and an array.
         'auth_supported' became 'auth-supported', and an array.
         (Thanks Daniel, Eric)

          Patch 5 also contains a new function, qemu_rbd_array_opts(), to
          parse the array options.


Changes from v1:

Overall:

* QAPI interface does not allow arbitrary key/value pairs
  in v2 (Thanks Daniel)

* QAPI interface adds 'mon_host' and 'auth_supported' options (Thanks Daniel)

* Use 'user' instead of 'rbd-id' (Thanks Daniel)
v
By patch:

Patch 1:
 * Fixed some indentation in patch 1 (Thanks Markus)

Patch 2:
 * 'rbd-id' becomes 'user', and the commit message is fixed. (Thanks Daniel)

Patch 3:
 * Ripple-through from changes in patch 2
 * Removed the string unescape from qemu_rbd_set_keypairs(), because the
   strings have already been unescaped by the time they hit this function.

Patch 4:
 * 'rbd-id' becomes 'user'
 * drop the 'keyvalue-pairs' from the QAPI  (both, thanks Daniel)

Patch 5:
 * new patch
 * Adds the 'server' (mon_host) and 'auth_supported' options to the
   QAPI (Thanks Daniel)


Jeff Cody (5):
  block/rbd: don't copy strings in qemu_rbd_next_tok()
  block/rbd: add all the currently supported runtime_opts
  block/rbd: parse all options via bdrv_parse_filename
  block/rbd: add blockdev-add support
  block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

 block/rbd.c          | 553 ++++++++++++++++++++++++++++++++++-----------------
 qapi/block-core.json |  62 +++++-
 2 files changed, 427 insertions(+), 188 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/5] block/rbd: don't copy strings in qemu_rbd_next_tok()

[Jeff Cody]

This patch is prep work for parsing options for .bdrv_parse_filename,
and using QDict options.

The function qemu_rbd_next_tok() searched for various key/value pairs,
and copied them into buffers.  This will soon be an unnecessary extra
step, so we will now return found strings by reference only, and
offload the responsibility for safely handling/coping these strings to
the caller.

This also cleans up error handling some, as the callers now rely on
the Error object to determine if there is a parse error.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 99 +++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 64 insertions(+), 35 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 22e8e69..33c21d8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -102,10 +102,10 @@ typedef struct BDRVRBDState {
     char *snap;
 } BDRVRBDState;
 
-static int qemu_rbd_next_tok(char *dst, int dst_len,
-                             char *src, char delim,
-                             const char *name,
-                             char **p, Error **errp)
+static char *qemu_rbd_next_tok(int max_len,
+                               char *src, char delim,
+                               const char *name,
+                               char **p, Error **errp)
 {
     int l;
     char *end;
@@ -127,17 +127,15 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
         }
     }
     l = strlen(src);
-    if (l >= dst_len) {
+    if (l >= max_len) {
         error_setg(errp, "%s too long", name);
-        return -EINVAL;
+        return NULL;
     } else if (l == 0) {
         error_setg(errp, "%s too short", name);
-        return -EINVAL;
+        return NULL;
     }
 
-    pstrcpy(dst, dst_len, src);
-
-    return 0;
+    return src;
 }
 
 static void qemu_rbd_unescape(char *src)
@@ -162,7 +160,9 @@ static int qemu_rbd_parsename(const char *filename,
 {
     const char *start;
     char *p, *buf;
-    int ret;
+    int ret = 0;
+    char *found_str;
+    Error *local_err = NULL;
 
     if (!strstart(filename, "rbd:", &start)) {
         error_setg(errp, "File name must start with 'rbd:'");
@@ -174,36 +174,60 @@ static int qemu_rbd_parsename(const char *filename,
     *snap = '\0';
     *conf = '\0';
 
-    ret = qemu_rbd_next_tok(pool, pool_len, p,
-                            '/', "pool name", &p, errp);
-    if (ret < 0 || !p) {
+    found_str = qemu_rbd_next_tok(pool_len, p,
+                                  '/', "pool name", &p, &local_err);
+    if (local_err) {
+        goto done;
+    }
+    if (!p) {
         ret = -EINVAL;
+        error_setg(errp, "Pool name is required");
         goto done;
     }
-    qemu_rbd_unescape(pool);
+    qemu_rbd_unescape(found_str);
+    g_strlcpy(pool, found_str, pool_len);
 
     if (strchr(p, '@')) {
-        ret = qemu_rbd_next_tok(name, name_len, p,
-                                '@', "object name", &p, errp);
-        if (ret < 0) {
+        found_str = qemu_rbd_next_tok(name_len, p,
+                                      '@', "object name", &p, &local_err);
+        if (local_err) {
             goto done;
         }
-        ret = qemu_rbd_next_tok(snap, snap_len, p,
-                                ':', "snap name", &p, errp);
-        qemu_rbd_unescape(snap);
+        qemu_rbd_unescape(found_str);
+        g_strlcpy(name, found_str, name_len);
+
+        found_str = qemu_rbd_next_tok(snap_len, p,
+                                      ':', "snap name", &p, &local_err);
+        if (local_err) {
+            goto done;
+        }
+        qemu_rbd_unescape(found_str);
+        g_strlcpy(snap, found_str, snap_len);
     } else {
-        ret = qemu_rbd_next_tok(name, name_len, p,
-                                ':', "object name", &p, errp);
+        found_str = qemu_rbd_next_tok(name_len, p,
+                                      ':', "object name", &p, &local_err);
+        if (local_err) {
+            goto done;
+        }
+        qemu_rbd_unescape(found_str);
+        g_strlcpy(name, found_str, name_len);
     }
-    qemu_rbd_unescape(name);
-    if (ret < 0 || !p) {
+    if (!p) {
         goto done;
     }
 
-    ret = qemu_rbd_next_tok(conf, conf_len, p,
-                            '\0', "configuration", &p, errp);
+    found_str = qemu_rbd_next_tok(conf_len, p,
+                                  '\0', "configuration", &p, &local_err);
+    if (local_err) {
+        goto done;
+    }
+    g_strlcpy(conf, found_str, conf_len);
 
 done:
+    if (local_err) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+    }
     g_free(buf);
     return ret;
 }
@@ -262,17 +286,18 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
                              Error **errp)
 {
     char *p, *buf;
-    char name[RBD_MAX_CONF_NAME_SIZE];
-    char value[RBD_MAX_CONF_VAL_SIZE];
+    char *name;
+    char *value;
+    Error *local_err = NULL;
     int ret = 0;
 
     buf = g_strdup(conf);
     p = buf;
 
     while (p) {
-        ret = qemu_rbd_next_tok(name, sizeof(name), p,
-                                '=', "conf option name", &p, errp);
-        if (ret < 0) {
+        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
+                                 '=', "conf option name", &p, &local_err);
+        if (local_err) {
             break;
         }
         qemu_rbd_unescape(name);
@@ -283,9 +308,9 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
             break;
         }
 
-        ret = qemu_rbd_next_tok(value, sizeof(value), p,
-                                ':', "conf option value", &p, errp);
-        if (ret < 0) {
+        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
+                                  ':', "conf option value", &p, &local_err);
+        if (local_err) {
             break;
         }
         qemu_rbd_unescape(value);
@@ -313,6 +338,10 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
         }
     }
 
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+    }
     g_free(buf);
     return ret;
 }
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/5] block/rbd: add all the currently supported runtime_opts

[Jeff Cody]

This adds all the currently supported runtime opts, which
are the options as parsed from the filename.  All of these
options are explicitly checked for during during runtime,
with an exception to the "keyvalue-pairs" option.

This option contains all the key/value pairs that the QEMU rbd
driver merely unescapes, and passes along blindly to rados.  This
option is a "legacy" option, and will not be exposed in the QAPI
or available for introspection.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 19 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 33c21d8..67d680c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -357,6 +357,55 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
     }
 }
 
+static QemuOptsList runtime_opts = {
+    .name = "rbd",
+    .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",
+        },
+        {
+            .name = "image",
+            .type = QEMU_OPT_STRING,
+            .help = "Image name in the pool",
+        },
+        {
+            .name = "snapshot",
+            .type = QEMU_OPT_STRING,
+            .help = "Ceph snapshot name",
+        },
+        {
+            /* maps to 'id' in rados_create() */
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "Rados id name",
+        },
+        {
+            .name = "keyvalue-pairs",
+            .type = QEMU_OPT_STRING,
+            .help = "Legacy rados key/value option parameters",
+        },
+        { /* end of list */ }
+    },
+};
+
 static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     Error *local_err = NULL;
@@ -500,25 +549,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-/* TODO Convert to fine grained options */
-static QemuOptsList runtime_opts = {
-    .name = "rbd",
-    .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",
-        },
-        { /* end of list */ }
-    },
-};
-
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
-- 
2.9.3

[Qemu-devel] [PATCH v3 3/5] block/rbd: parse all options via bdrv_parse_filename

[Jeff Cody]

Get rid of qemu_rbd_parsename in favor of bdrv_parse_filename.
This simplifies a lot of the parsing as well, as we can treat everything
a bit simpler since nonexistent options are simply NULL pointers instead
of empty strings.

An important item to note:

Ceph has many extra option values that can be specified as key/value
pairs.  This was handled previously in the driver by extracting the
values that the QEMU driver cared about, and then blindly passing all
extra options to rbd after splitting them into key/value pairs, and
cleaning up any special character escaping.

The practice is continued in this patch; there is an option
"keyvalue-pairs" that is populated with all the key/value pairs that the
QEMU driver does not care about.  These key/value pairs will override
any settings in the 'conf' configuration file, just as they did before.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 303 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 153 insertions(+), 150 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 67d680c..cc43f42 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -18,6 +18,7 @@
 #include "block/block_int.h"
 #include "crypto/secret.h"
 #include "qemu/cutils.h"
+#include "qapi/qmp/qstring.h"
 
 #include <rbd/librbd.h>
 
@@ -151,113 +152,134 @@ static void qemu_rbd_unescape(char *src)
     *p = '\0';
 }
 
-static int qemu_rbd_parsename(const char *filename,
-                              char *pool, int pool_len,
-                              char *snap, int snap_len,
-                              char *name, int name_len,
-                              char *conf, int conf_len,
-                              Error **errp)
+static void qemu_rbd_parse_filename(const char *filename, QDict *options,
+                                    Error **errp)
 {
     const char *start;
-    char *p, *buf;
-    int ret = 0;
+    char *p, *buf, *keypairs;
     char *found_str;
+    size_t max_keypair_size;
     Error *local_err = NULL;
 
     if (!strstart(filename, "rbd:", &start)) {
         error_setg(errp, "File name must start with 'rbd:'");
-        return -EINVAL;
+        return;
     }
 
+    max_keypair_size = strlen(start) + 1;
     buf = g_strdup(start);
+    keypairs = g_malloc0(max_keypair_size);
     p = buf;
-    *snap = '\0';
-    *conf = '\0';
 
-    found_str = qemu_rbd_next_tok(pool_len, p,
+    found_str = qemu_rbd_next_tok(RBD_MAX_POOL_NAME_SIZE, p,
                                   '/', "pool name", &p, &local_err);
     if (local_err) {
         goto done;
     }
     if (!p) {
-        ret = -EINVAL;
         error_setg(errp, "Pool name is required");
         goto done;
     }
     qemu_rbd_unescape(found_str);
-    g_strlcpy(pool, found_str, pool_len);
+    qdict_put(options, "pool", qstring_from_str(found_str));
 
     if (strchr(p, '@')) {
-        found_str = qemu_rbd_next_tok(name_len, p,
+        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
                                       '@', "object name", &p, &local_err);
         if (local_err) {
             goto done;
         }
         qemu_rbd_unescape(found_str);
-        g_strlcpy(name, found_str, name_len);
+        qdict_put(options, "image", qstring_from_str(found_str));
 
-        found_str = qemu_rbd_next_tok(snap_len, p,
+        found_str = qemu_rbd_next_tok(RBD_MAX_SNAP_NAME_SIZE, p,
                                       ':', "snap name", &p, &local_err);
         if (local_err) {
             goto done;
         }
         qemu_rbd_unescape(found_str);
-        g_strlcpy(snap, found_str, snap_len);
+        qdict_put(options, "snapshot", qstring_from_str(found_str));
     } else {
-        found_str = qemu_rbd_next_tok(name_len, p,
+        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
                                       ':', "object name", &p, &local_err);
         if (local_err) {
             goto done;
         }
         qemu_rbd_unescape(found_str);
-        g_strlcpy(name, found_str, name_len);
+        qdict_put(options, "image", qstring_from_str(found_str));
     }
     if (!p) {
         goto done;
     }
 
-    found_str = qemu_rbd_next_tok(conf_len, p,
+    found_str = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
                                   '\0', "configuration", &p, &local_err);
     if (local_err) {
         goto done;
     }
-    g_strlcpy(conf, found_str, conf_len);
+
+    p = found_str;
+
+    /* The following are essentially all key/value pairs, and we treat
+     * 'id' and 'conf' a bit special.  Key/value pairs may be in any order. */
+    while (p) {
+        char *name, *value;
+        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
+                                 '=', "conf option name", &p, &local_err);
+        if (local_err) {
+            break;
+        }
+
+        if (!p) {
+            error_setg(errp, "conf option %s has no value", name);
+            break;
+        }
+
+        qemu_rbd_unescape(name);
+
+        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
+                                  ':', "conf option value", &p, &local_err);
+        if (local_err) {
+            break;
+        }
+        qemu_rbd_unescape(value);
+
+        if (!strcmp(name, "conf")) {
+            qdict_put(options, "conf", qstring_from_str(value));
+        } else if (!strcmp(name, "id")) {
+            qdict_put(options, "user" , qstring_from_str(value));
+        } else {
+            /* FIXME: This is pretty ugly, and not the right way to do this.
+             *        These should be contained in a structure, and then
+             *        passed explicitly as individual key/value pairs to
+             *        rados.  Consider this legacy code that needs to be
+             *        updated. */
+            char *tmp = g_malloc0(max_keypair_size);
+            /* only use a delimiter if it is not the first keypair found */
+            /* These are sets of unknown key/value pairs we'll pass along
+             * to ceph */
+            if (keypairs[0]) {
+                snprintf(tmp, max_keypair_size, ":%s=%s", name, value);
+                pstrcat(keypairs, max_keypair_size, tmp);
+            } else {
+                snprintf(keypairs, max_keypair_size, "%s=%s", name, value);
+            }
+            g_free(tmp);
+        }
+    }
+
+    if (keypairs[0]) {
+        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));
+    }
+
 
 done:
     if (local_err) {
-        ret = -EINVAL;
         error_propagate(errp, local_err);
     }
     g_free(buf);
-    return ret;
-}
-
-static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
-{
-    const char *p = conf;
-
-    while (*p) {
-        int len;
-        const char *end = strchr(p, ':');
-
-        if (end) {
-            len = end - p;
-        } else {
-            len = strlen(p);
-        }
-
-        if (strncmp(p, "id=", 3) == 0) {
-            len -= 3;
-            strncpy(clientname, p + 3, len);
-            clientname[len] = '\0';
-            return clientname;
-        }
-        if (end == NULL) {
-            break;
-        }
-        p = end + 1;
-    }
-    return NULL;
+    g_free(keypairs);
+    return;
 }
 
 
@@ -280,10 +302,8 @@ static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
     return 0;
 }
 
-
-static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
-                             bool only_read_conf_file,
-                             Error **errp)
+static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
+                                 Error **errp)
 {
     char *p, *buf;
     char *name;
@@ -291,7 +311,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
     Error *local_err = NULL;
     int ret = 0;
 
-    buf = g_strdup(conf);
+    buf = g_strdup(keypairs);
     p = buf;
 
     while (p) {
@@ -300,7 +320,6 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
         if (local_err) {
             break;
         }
-        qemu_rbd_unescape(name);
 
         if (!p) {
             error_setg(errp, "conf option %s has no value", name);
@@ -313,28 +332,12 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
         if (local_err) {
             break;
         }
-        qemu_rbd_unescape(value);
 
-        if (strcmp(name, "conf") == 0) {
-            /* read the conf file alone, so it doesn't override more
-               specific settings for a particular device */
-            if (only_read_conf_file) {
-                ret = rados_conf_read_file(cluster, value);
-                if (ret < 0) {
-                    error_setg_errno(errp, -ret, "error reading conf file %s",
-                                     value);
-                    break;
-                }
-            }
-        } else if (strcmp(name, "id") == 0) {
-            /* ignore, this is parsed by qemu_rbd_parse_clientname() */
-        } else if (!only_read_conf_file) {
-            ret = rados_conf_set(cluster, name, value);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret, "invalid conf option %s", name);
-                ret = -EINVAL;
-                break;
-            }
+        ret = rados_conf_set(cluster, name, value);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "invalid conf option %s", name);
+            ret = -EINVAL;
+            break;
         }
     }
 
@@ -412,27 +415,16 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t bytes = 0;
     int64_t objsize;
     int obj_order = 0;
-    char pool[RBD_MAX_POOL_NAME_SIZE];
-    char name[RBD_MAX_IMAGE_NAME_SIZE];
-    char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
-    char conf[RBD_MAX_CONF_SIZE];
-    char clientname_buf[RBD_MAX_CONF_SIZE];
-    char *clientname;
+    const char *pool, *name, *conf, *clientname, *keypairs;
     const char *secretid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
-    int ret;
+    QDict *options = NULL;
+    QemuOpts *rbd_opts = NULL;
+    int ret = 0;
 
     secretid = qemu_opt_get(opts, "password-secret");
 
-    if (qemu_rbd_parsename(filename, pool, sizeof(pool),
-                           snap_buf, sizeof(snap_buf),
-                           name, sizeof(name),
-                           conf, sizeof(conf), &local_err) < 0) {
-        error_propagate(errp, local_err);
-        return -EINVAL;
-    }
-
     /* Read out options */
     bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                      BDRV_SECTOR_SIZE);
@@ -440,35 +432,55 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     if (objsize) {
         if ((objsize - 1) & objsize) {    /* not a power of 2? */
             error_setg(errp, "obj size needs to be power of 2");
-            return -EINVAL;
+            ret = -EINVAL;
+            goto exit;
         }
         if (objsize < 4096) {
             error_setg(errp, "obj size too small");
-            return -EINVAL;
+            ret = -EINVAL;
+            goto exit;
         }
         obj_order = ctz32(objsize);
     }
 
-    clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
+    options = qdict_new();
+    qemu_rbd_parse_filename(filename, options, &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        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");
+
     ret = rados_create(&cluster, clientname);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error initializing");
-        return ret;
+        goto exit;
     }
 
-    if (strstr(conf, "conf=") == NULL) {
-        /* try default location, but ignore failure */
-        rados_conf_read_file(cluster, NULL);
-    } else if (conf[0] != '\0' &&
-               qemu_rbd_set_conf(cluster, conf, true, &local_err) < 0) {
-        error_propagate(errp, local_err);
+    /* try default location when conf=NULL, but ignore failure */
+    ret = rados_conf_read_file(cluster, conf);
+    if (conf && ret < 0) {
+        error_setg_errno(errp, -ret, "error reading conf file %s", conf);
         ret = -EIO;
         goto shutdown;
     }
 
-    if (conf[0] != '\0' &&
-        qemu_rbd_set_conf(cluster, conf, false, &local_err) < 0) {
-        error_propagate(errp, local_err);
+    ret = qemu_rbd_set_keypairs(cluster, keypairs, errp);
+    if (ret < 0) {
         ret = -EIO;
         goto shutdown;
     }
@@ -499,6 +511,10 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
 
 shutdown:
     rados_shutdown(cluster);
+
+exit:
+    QDECREF(options);
+    qemu_opts_del(rbd_opts);
     return ret;
 }
 
@@ -553,15 +569,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
-    char pool[RBD_MAX_POOL_NAME_SIZE];
-    char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
-    char conf[RBD_MAX_CONF_SIZE];
-    char clientname_buf[RBD_MAX_CONF_SIZE];
-    char *clientname;
+    const char *pool, *snap, *conf, *clientname, *name, *keypairs;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename;
     int r;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -572,44 +583,36 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    filename = qemu_opt_get(opts, "filename");
     secretid = qemu_opt_get(opts, "password-secret");
 
-    if (qemu_rbd_parsename(filename, pool, sizeof(pool),
-                           snap_buf, sizeof(snap_buf),
-                           s->name, sizeof(s->name),
-                           conf, sizeof(conf), errp) < 0) {
-        r = -EINVAL;
-        goto failed_opts;
-    }
+    pool           = qemu_opt_get(opts, "pool");
+    conf           = qemu_opt_get(opts, "conf");
+    snap           = qemu_opt_get(opts, "snapshot");
+    clientname     = qemu_opt_get(opts, "user");
+    name           = qemu_opt_get(opts, "image");
+    keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
 
-    clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     r = rados_create(&s->cluster, clientname);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
-    s->snap = NULL;
-    if (snap_buf[0] != '\0') {
-        s->snap = g_strdup(snap_buf);
+    s->snap = g_strdup(snap);
+    if (name) {
+        pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
     }
 
-    if (strstr(conf, "conf=") == NULL) {
-        /* try default location, but ignore failure */
-        rados_conf_read_file(s->cluster, NULL);
-    } else if (conf[0] != '\0') {
-        r = qemu_rbd_set_conf(s->cluster, conf, true, errp);
-        if (r < 0) {
-            goto failed_shutdown;
-        }
+    /* try default location when conf=NULL, but ignore failure */
+    r = rados_conf_read_file(s->cluster, conf);
+    if (conf && r < 0) {
+        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+        goto failed_shutdown;
     }
 
-    if (conf[0] != '\0') {
-        r = qemu_rbd_set_conf(s->cluster, conf, false, errp);
-        if (r < 0) {
-            goto failed_shutdown;
-        }
+    r = qemu_rbd_set_keypairs(s->cluster, keypairs, errp);
+    if (r < 0) {
+        goto failed_shutdown;
     }
 
     if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
@@ -1063,18 +1066,18 @@ static QemuOptsList qemu_rbd_create_opts = {
 };
 
 static BlockDriver bdrv_rbd = {
-    .format_name        = "rbd",
-    .instance_size      = sizeof(BDRVRBDState),
-    .bdrv_needs_filename = true,
-    .bdrv_file_open     = qemu_rbd_open,
-    .bdrv_close         = qemu_rbd_close,
-    .bdrv_create        = qemu_rbd_create,
-    .bdrv_has_zero_init = bdrv_has_zero_init_1,
-    .bdrv_get_info      = qemu_rbd_getinfo,
-    .create_opts        = &qemu_rbd_create_opts,
-    .bdrv_getlength     = qemu_rbd_getlength,
-    .bdrv_truncate      = qemu_rbd_truncate,
-    .protocol_name      = "rbd",
+    .format_name            = "rbd",
+    .instance_size          = sizeof(BDRVRBDState),
+    .bdrv_parse_filename    = qemu_rbd_parse_filename,
+    .bdrv_file_open         = qemu_rbd_open,
+    .bdrv_close             = qemu_rbd_close,
+    .bdrv_create            = qemu_rbd_create,
+    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .bdrv_get_info          = qemu_rbd_getinfo,
+    .create_opts            = &qemu_rbd_create_opts,
+    .bdrv_getlength         = qemu_rbd_getlength,
+    .bdrv_truncate          = qemu_rbd_truncate,
+    .protocol_name          = "rbd",
 
     .bdrv_aio_readv         = qemu_rbd_aio_readv,
     .bdrv_aio_writev        = qemu_rbd_aio_writev,
-- 
2.9.3

[Qemu-devel] [PATCH v3 4/5] block/rbd: add blockdev-add support

[Jeff Cody]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 qapi/block-core.json | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f82d35..f152953 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2111,6 +2111,7 @@
 # @replication: Since 2.8
 # @ssh: Since 2.8
 # @iscsi: Since 2.9
+# @rbd: Since 2.9
 #
 # Since: 2.0
 ##
@@ -2119,7 +2120,7 @@
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
-            'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
+            'quorum', 'raw', 'rbd', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
             'vpc', 'vvfat' ] }
 
 ##
@@ -2666,6 +2667,34 @@
             '*timeout': 'int' } }
 
 ##
+# @BlockdevOptionsRbd:
+#
+# @pool:               Ceph pool name.
+#
+# @image:              Image name in the Ceph pool.
+#
+# @conf:               #optional path to Ceph configuration file.  Values
+#                      in the configuration file will be overridden by
+#                      options specified via QAPI.
+#
+# @snapshot:           #optional Ceph snapshot name.
+#
+# @user:               #optional Ceph id name.
+#
+# @password-secret:    #optional The ID of a QCryptoSecret object providing
+#                      the password for the login.
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsRbd',
+  'data': { 'pool': 'str',
+            'image': 'str',
+            '*conf': 'str',
+            '*snapshot': 'str',
+            '*user': 'str',
+            '*password-secret': 'str' } }
+
+##
 # @ReplicationMode:
 #
 # An enumeration of replication modes.
@@ -2863,7 +2892,7 @@
       'qed':        'BlockdevOptionsGenericCOWFormat',
       'quorum':     'BlockdevOptionsQuorum',
       'raw':        'BlockdevOptionsRaw',
-# TODO rbd: Wait for structured options
+      'rbd':        'BlockdevOptionsRbd',
       'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
       'ssh':        'BlockdevOptionsSsh',
-- 
2.9.3

[Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Jeff Cody]

This adds support for three additional options that may be specified
by QAPI in blockdev-add:

    server: host, port
    auth method: either 'cephx' or 'none'

The "server" and "auth-supported" QAPI parameters are arrays.  To conform
with the rados API, the array items are join as a single string with a ';'
character as a delimiter when setting the configuration values.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c          | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  29 +++++++++++++
 2 files changed, 148 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index cc43f42..dfa52cc 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -405,6 +405,19 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
+        {
+            .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 */ }
     },
 };
@@ -565,14 +578,89 @@ 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)
+{
+    size_t num_entries;
+    QemuOpts *opts = NULL;
+    QDict *sub_options;
+    const char *host;
+    const char *port;
+    char *str;
+    char *rados_str = NULL;
+    Error *local_err = NULL;
+
+    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
+
+    num_entries = qdict_array_entries(options, prefix);
+
+    if (num_entries) {
+        for (int i = 0; i < num_entries; i++) {
+            char *tmp = 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);
+                goto exit;
+            }
+
+            if (type == RBD_MON_HOST) {
+                host = qemu_opt_get(opts, "host");
+                port = qemu_opt_get(opts, "port");
+
+                value = host;
+                if (port) {
+                    tmp = g_strdup_printf("%s:%s", host, port);
+                    value = tmp;
+                }
+            } else {
+                value = qemu_opt_get(opts, "auth");
+            }
+
+
+            /* 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);
+            } else {
+                rados_str = g_strdup(value);
+            }
+
+            g_free(tmp);
+            qemu_opts_del(opts);
+            opts = NULL;
+        }
+    }
+
+exit:
+    qemu_opts_del(opts);
+    return rados_str;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     const char *pool, *snap, *conf, *clientname, *name, *keypairs;
+    const char *auth_supported;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
+    char *mon_host = NULL;
     int r;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -583,6 +671,22 @@ 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;
+    }
+
     secretid = qemu_opt_get(opts, "password-secret");
 
     pool           = qemu_opt_get(opts, "pool");
@@ -615,6 +719,20 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
+    if (mon_host) {
+        r = rados_conf_set(s->cluster, "mon_host", mon_host);
+        if (r < 0) {
+            goto failed_shutdown;
+        }
+    }
+
+    if (auth_supported) {
+        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
+        if (r < 0) {
+            goto failed_shutdown;
+        }
+    }
+
     if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
         r = -EIO;
         goto failed_shutdown;
@@ -663,6 +781,7 @@ failed_shutdown:
     g_free(s->snap);
 failed_opts:
     qemu_opts_del(opts);
+    g_free(mon_host);
     return r;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f152953..5f74f92 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2666,6 +2666,28 @@
             '*header-digest': 'IscsiHeaderDigest',
             '*timeout': 'int' } }
 
+
+##
+# @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' } }
+
 ##
 # @BlockdevOptionsRbd:
 #
@@ -2681,6 +2703,11 @@
 #
 # @user:               #optional Ceph id name.
 #
+# @server:             #optional Monitor host address and port.  This maps
+#                      to the "mon_host" Ceph option.
+#
+# @auth-supported:     #optional Authentication supported.
+#
 # @password-secret:    #optional The ID of a QCryptoSecret object providing
 #                      the password for the login.
 #
@@ -2692,6 +2719,8 @@
             '*conf': 'str',
             '*snapshot': 'str',
             '*user': 'str',
+            '*server': ['InetSocketAddress'],
+            '*auth-supported': ['RbdAuthMethod'],
             '*password-secret': 'str' } }
 
 ##
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Markus Armbruster]

Starting with just the QAPI schema.

Jeff Cody <jcody@redhat.com> writes:

> This adds support for three additional options that may be specified
> by QAPI in blockdev-add:
>
>     server: host, port
>     auth method: either 'cephx' or 'none'
>
> The "server" and "auth-supported" QAPI parameters are arrays.  To conform
> with the rados API, the array items are join as a single string with a ';'
> character as a delimiter when setting the configuration values.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f152953..5f74f92 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2666,6 +2666,28 @@
>              '*header-digest': 'IscsiHeaderDigest',
>              '*timeout': 'int' } }
>  
> +
> +##
> +# @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' } }
> +

Any particular reason for wrapping the enum in a struct?  Do you
envisage adding members to the struct?

>  ##
>  # @BlockdevOptionsRbd:
>  #
> @@ -2681,6 +2703,11 @@
>  #
>  # @user:               #optional Ceph id name.
>  #
> +# @server:             #optional Monitor host address and port.  This maps
> +#                      to the "mon_host" Ceph option.

Suggest something like "Monitor addresses", for consistency with how we
document *SocketAddress members elsewhere, and plural to hint at it
being a list, not just one.

> +#
> +# @auth-supported:     #optional Authentication supported.
> +#
>  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>  #                      the password for the login.
>  #
> @@ -2692,6 +2719,8 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> +            '*server': ['InetSocketAddress'],
> +            '*auth-supported': ['RbdAuthMethod'],
>              '*password-secret': 'str' } }
>  
>  ##

Re: [Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Jeff Cody]

On Tue, Feb 28, 2017 at 03:34:10PM +0100, Markus Armbruster wrote:
> Starting with just the QAPI schema.
> 
> Jeff Cody <jcody@redhat.com> writes:
> 
> > This adds support for three additional options that may be specified
> > by QAPI in blockdev-add:
> >
> >     server: host, port
> >     auth method: either 'cephx' or 'none'
> >
> > The "server" and "auth-supported" QAPI parameters are arrays.  To conform
> > with the rados API, the array items are join as a single string with a ';'
> > character as a delimiter when setting the configuration values.
> >
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index f152953..5f74f92 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2666,6 +2666,28 @@
> >              '*header-digest': 'IscsiHeaderDigest',
> >              '*timeout': 'int' } }
> >  
> > +
> > +##
> > +# @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' } }
> > +
> 
> Any particular reason for wrapping the enum in a struct?  Do you
> envisage adding members to the struct?
>

I am going to admit, mainly it was my frustration with trying to deal with a
qapi array of just enums in a QDict, and structs was more straightforward.
What is the best way to parse an array of enums inside a QDict?  Do you need
to extract the subqdict via qdict_extract_subqdict() still?

> >  ##
> >  # @BlockdevOptionsRbd:
> >  #
> > @@ -2681,6 +2703,11 @@
> >  #
> >  # @user:               #optional Ceph id name.
> >  #
> > +# @server:             #optional Monitor host address and port.  This maps
> > +#                      to the "mon_host" Ceph option.
> 
> Suggest something like "Monitor addresses", for consistency with how we
> document *SocketAddress members elsewhere, and plural to hint at it
> being a list, not just one.
> 

OK, thanks.

> > +#
> > +# @auth-supported:     #optional Authentication supported.
> > +#
> >  # @password-secret:    #optional The ID of a QCryptoSecret object providing
> >  #                      the password for the login.
> >  #
> > @@ -2692,6 +2719,8 @@
> >              '*conf': 'str',
> >              '*snapshot': 'str',
> >              '*user': 'str',
> > +            '*server': ['InetSocketAddress'],
> > +            '*auth-supported': ['RbdAuthMethod'],
> >              '*password-secret': 'str' } }
> >  
> >  ##

Re: [Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> This adds support for three additional options that may be specified
> by QAPI in blockdev-add:
>
>     server: host, port
>     auth method: either 'cephx' or 'none'
>
> The "server" and "auth-supported" QAPI parameters are arrays.  To conform
> with the rados API, the array items are join as a single string with a ';'
> character as a delimiter when setting the configuration values.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c          | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  29 +++++++++++++
>  2 files changed, 148 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index cc43f42..dfa52cc 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -405,6 +405,19 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Legacy rados key/value option parameters",
>          },
> +        {
> +            .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 */ }
>      },
>  };
> @@ -565,14 +578,89 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>      qemu_aio_unref(acb);
>  }
>  
> +#define RBD_MON_HOST          0
> +#define RBD_AUTH_SUPPORTED    1

Blank line here, please.

> +static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> +                                 Error **errp)
> +{
> +    size_t num_entries;
> +    QemuOpts *opts = NULL;
> +    QDict *sub_options;
> +    const char *host;
> +    const char *port;
> +    char *str;
> +    char *rados_str = NULL;
> +    Error *local_err = NULL;
> +
> +    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> +
> +    num_entries = qdict_array_entries(options, prefix);

Can this fail?

> +
> +    if (num_entries) {

Superfluous conditional: if !num_entries, the loop rejects.

> +        for (int i = 0; i < num_entries; i++) {
> +            char *tmp = 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);
> +                goto exit;

Hmm.  Unless this is the first iteration, rados_str is already non-null,
i.e. we fail and return a string the caller must free.  That's bad
practice; it's better to return NULL on failre.

> +            }
> +
> +            if (type == RBD_MON_HOST) {
> +                host = qemu_opt_get(opts, "host");
> +                port = qemu_opt_get(opts, "port");
> +
> +                value = host;
> +                if (port) {
> +                    tmp = g_strdup_printf("%s:%s", host, port);

Problematic when @host is numeric IPv6.  What syntax does ceph expect in
that case?

> +                    value = tmp;
> +                }
> +            } else {
> +                value = qemu_opt_get(opts, "auth");
> +            }
> +
> +
> +            /* 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);

I'd make rados_str a GString.  But your code isn't wrong.

> +            } else {
> +                rados_str = g_strdup(value);
> +            }
> +
> +            g_free(tmp);

Aha, @tmp is just for getting the g_strdup_printf() freed.  Rename to
strbuf?

> +            qemu_opts_del(opts);
> +            opts = NULL;
> +        }
> +    }
> +
> +exit:
> +    qemu_opts_del(opts);
> +    return rados_str;
> +}
> +
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>                           Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      const char *pool, *snap, *conf, *clientname, *name, *keypairs;
> +    const char *auth_supported;
>      const char *secretid;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> +    char *mon_host = NULL;
>      int r;
>  
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> @@ -583,6 +671,22 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> +   auth_supported = qemu_rbd_array_opts(options, "auth-supported.",

Indentation's off.

> +                                         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;
> +    }
> +
>      secretid = qemu_opt_get(opts, "password-secret");
>  
>      pool           = qemu_opt_get(opts, "pool");
> @@ -615,6 +719,20 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    if (mon_host) {
> +        r = rados_conf_set(s->cluster, "mon_host", mon_host);
> +        if (r < 0) {
> +            goto failed_shutdown;
> +        }
> +    }
> +
> +    if (auth_supported) {
> +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
> +        if (r < 0) {
> +            goto failed_shutdown;
> +        }
> +    }
> +
>      if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
>          r = -EIO;
>          goto failed_shutdown;
> @@ -663,6 +781,7 @@ failed_shutdown:
>      g_free(s->snap);
>  failed_opts:
>      qemu_opts_del(opts);
> +    g_free(mon_host);

Need to free auth_supported.

>      return r;
>  }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f152953..5f74f92 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2666,6 +2666,28 @@
>              '*header-digest': 'IscsiHeaderDigest',
>              '*timeout': 'int' } }
>  
> +
> +##
> +# @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' } }
> +
>  ##
>  # @BlockdevOptionsRbd:
>  #
> @@ -2681,6 +2703,11 @@
>  #
>  # @user:               #optional Ceph id name.
>  #
> +# @server:             #optional Monitor host address and port.  This maps
> +#                      to the "mon_host" Ceph option.
> +#
> +# @auth-supported:     #optional Authentication supported.
> +#
>  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>  #                      the password for the login.
>  #
> @@ -2692,6 +2719,8 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> +            '*server': ['InetSocketAddress'],
> +            '*auth-supported': ['RbdAuthMethod'],
>              '*password-secret': 'str' } }
>  
>  ##

Re: [Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Jeff Cody]

On Tue, Feb 28, 2017 at 04:07:14PM +0100, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > This adds support for three additional options that may be specified
> > by QAPI in blockdev-add:
> >
> >     server: host, port
> >     auth method: either 'cephx' or 'none'
> >
> > The "server" and "auth-supported" QAPI parameters are arrays.  To conform
> > with the rados API, the array items are join as a single string with a ';'
> > character as a delimiter when setting the configuration values.
> >
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c          | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi/block-core.json |  29 +++++++++++++
> >  2 files changed, 148 insertions(+)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index cc43f42..dfa52cc 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -405,6 +405,19 @@ static QemuOptsList runtime_opts = {
> >              .type = QEMU_OPT_STRING,
> >              .help = "Legacy rados key/value option parameters",
> >          },
> > +        {
> > +            .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 */ }
> >      },
> >  };
> > @@ -565,14 +578,89 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
> >      qemu_aio_unref(acb);
> >  }
> >  
> > +#define RBD_MON_HOST          0
> > +#define RBD_AUTH_SUPPORTED    1
> 
> Blank line here, please.
>

OK

> > +static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> > +                                 Error **errp)
> > +{
> > +    size_t num_entries;
> > +    QemuOpts *opts = NULL;
> > +    QDict *sub_options;
> > +    const char *host;
> > +    const char *port;
> > +    char *str;
> > +    char *rados_str = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> > +
> > +    num_entries = qdict_array_entries(options, prefix);
> 
> Can this fail?
> 

Yes, I should check for < 0 for error.

> > +
> > +    if (num_entries) {
> 
> Superfluous conditional: if !num_entries, the loop rejects.
> 

Will drop.

> > +        for (int i = 0; i < num_entries; i++) {
> > +            char *tmp = 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);
> > +                goto exit;
> 
> Hmm.  Unless this is the first iteration, rados_str is already non-null,
> i.e. we fail and return a string the caller must free.  That's bad
> practice; it's better to return NULL on failre.
> 

OK

> > +            }
> > +
> > +            if (type == RBD_MON_HOST) {
> > +                host = qemu_opt_get(opts, "host");
> > +                port = qemu_opt_get(opts, "port");
> > +
> > +                value = host;
> > +                if (port) {
> > +                    tmp = g_strdup_printf("%s:%s", host, port);
> 
> Problematic when @host is numeric IPv6.  What syntax does ceph expect in
> that case?
> 

I think we will need to encapsulate that with a '[]' for ipv6.

> > +                    value = tmp;
> > +                }
> > +            } else {
> > +                value = qemu_opt_get(opts, "auth");
> > +            }
> > +
> > +
> > +            /* 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);
> 
> I'd make rados_str a GString.  But your code isn't wrong.
> 
> > +            } else {
> > +                rados_str = g_strdup(value);
> > +            }
> > +
> > +            g_free(tmp);
> 
> Aha, @tmp is just for getting the g_strdup_printf() freed.  Rename to
> strbuf?
> 

Sure

> > +            qemu_opts_del(opts);
> > +            opts = NULL;
> > +        }
> > +    }
> > +
> > +exit:
> > +    qemu_opts_del(opts);
> > +    return rados_str;
> > +}
> > +
> >  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >                           Error **errp)
> >  {
> >      BDRVRBDState *s = bs->opaque;
> >      const char *pool, *snap, *conf, *clientname, *name, *keypairs;
> > +    const char *auth_supported;
> >      const char *secretid;
> >      QemuOpts *opts;
> >      Error *local_err = NULL;
> > +    char *mon_host = NULL;
> >      int r;
> >  
> >      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> > @@ -583,6 +671,22 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > +   auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
> 
> Indentation's off.
> 

OK, will fix

> > +                                         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;
> > +    }
> > +
> >      secretid = qemu_opt_get(opts, "password-secret");
> >  
> >      pool           = qemu_opt_get(opts, "pool");
> > @@ -615,6 +719,20 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto failed_shutdown;
> >      }
> >  
> > +    if (mon_host) {
> > +        r = rados_conf_set(s->cluster, "mon_host", mon_host);
> > +        if (r < 0) {
> > +            goto failed_shutdown;
> > +        }
> > +    }
> > +
> > +    if (auth_supported) {
> > +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
> > +        if (r < 0) {
> > +            goto failed_shutdown;
> > +        }
> > +    }
> > +
> >      if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
> >          r = -EIO;
> >          goto failed_shutdown;
> > @@ -663,6 +781,7 @@ failed_shutdown:
> >      g_free(s->snap);
> >  failed_opts:
> >      qemu_opts_del(opts);
> > +    g_free(mon_host);
> 
> Need to free auth_supported.
>

Thanks

> >      return r;
> >  }
> >  
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index f152953..5f74f92 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2666,6 +2666,28 @@
> >              '*header-digest': 'IscsiHeaderDigest',
> >              '*timeout': 'int' } }
> >  
> > +
> > +##
> > +# @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' } }
> > +
> >  ##
> >  # @BlockdevOptionsRbd:
> >  #
> > @@ -2681,6 +2703,11 @@
> >  #
> >  # @user:               #optional Ceph id name.
> >  #
> > +# @server:             #optional Monitor host address and port.  This maps
> > +#                      to the "mon_host" Ceph option.
> > +#
> > +# @auth-supported:     #optional Authentication supported.
> > +#
> >  # @password-secret:    #optional The ID of a QCryptoSecret object providing
> >  #                      the password for the login.
> >  #
> > @@ -2692,6 +2719,8 @@
> >              '*conf': 'str',
> >              '*snapshot': 'str',
> >              '*user': 'str',
> > +            '*server': ['InetSocketAddress'],
> > +            '*auth-supported': ['RbdAuthMethod'],
> >              '*password-secret': 'str' } }
> >  
> >  ##

Re: [Qemu-devel] [PATCH v3 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Tue, Feb 28, 2017 at 03:34:10PM +0100, Markus Armbruster wrote:
>> Starting with just the QAPI schema.
>> 
>> Jeff Cody <jcody@redhat.com> writes:
>> 
>> > This adds support for three additional options that may be specified
>> > by QAPI in blockdev-add:
>> >
>> >     server: host, port
>> >     auth method: either 'cephx' or 'none'
>> >
>> > The "server" and "auth-supported" QAPI parameters are arrays.  To conform
>> > with the rados API, the array items are join as a single string with a ';'
>> > character as a delimiter when setting the configuration values.
>> >
>> > Signed-off-by: Jeff Cody <jcody@redhat.com>
>> > ---
>> [...]
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index f152953..5f74f92 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -2666,6 +2666,28 @@
>> >              '*header-digest': 'IscsiHeaderDigest',
>> >              '*timeout': 'int' } }
>> >  
>> > +
>> > +##
>> > +# @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' } }
>> > +
>> 
>> Any particular reason for wrapping the enum in a struct?  Do you
>> envisage adding members to the struct?
>>
>
> I am going to admit, mainly it was my frustration with trying to deal with a
> qapi array of just enums in a QDict, and structs was more straightforward.
> What is the best way to parse an array of enums inside a QDict?  Do you need
> to extract the subqdict via qdict_extract_subqdict() still?

I admit I'm not so familiar with the block layer's QObject mangling, and
the helper functions we've grown to support that.

I *am* familiar with QAPI visitors (I better be, I maintain the
subsystem), and they are my preferred tool to go from QObject to C data
structures and back.

The "to C" direction uses a QObject input visitor.  Roughly like this:

    Error *err = NULL;
    Visitor *v;
    QapiType *qapi;

    v = qobject_input_visitor_new(qobj);
    visit_type_QapiType(v, NULL, &qapi, &err);
    visit_free(v);

You now either got an Error object in @err, or a QapiType object in
@qapi.

To free the QapiType:

    qapi_free_QapiType(qapi);

The QapiType for ['RbdAuthSupport'] (i.e. list of enum RbdAuthSupport) is
called RbdAuthSupportList, and should look like this:

    struct RbdAuthSupportList {
        RbdAuthSupportList *next;
        RbdAuthSupport value;
    };

The next thing I have to admit is that I fail to see where you're
dealing "with a qapi array of just enums in a QDict".  Can you help?

>> >  ##
>> >  # @BlockdevOptionsRbd:
>> >  #
>> > @@ -2681,6 +2703,11 @@
>> >  #
>> >  # @user:               #optional Ceph id name.
>> >  #
>> > +# @server:             #optional Monitor host address and port.  This maps
>> > +#                      to the "mon_host" Ceph option.
>> 
>> Suggest something like "Monitor addresses", for consistency with how we
>> document *SocketAddress members elsewhere, and plural to hint at it
>> being a list, not just one.
>> 
>
> OK, thanks.
>
>> > +#
>> > +# @auth-supported:     #optional Authentication supported.
>> > +#
>> >  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>> >  #                      the password for the login.
>> >  #
>> > @@ -2692,6 +2719,8 @@
>> >              '*conf': 'str',
>> >              '*snapshot': 'str',
>> >              '*user': 'str',
>> > +            '*server': ['InetSocketAddress'],
>> > +            '*auth-supported': ['RbdAuthMethod'],
>> >              '*password-secret': 'str' } }
>> >  
>> >  ##