[Qemu-devel] [PATCH v2 0/5] RBD: blockdev-add

[Qemu-devel] [PATCH v2 0/5] RBD: blockdev-add

[Jeff Cody]

This series adds blockdev-add for rbd.

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)


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          | 464 ++++++++++++++++++++++++++++++---------------------
 qapi/block-core.json |  42 ++++-
 2 files changed, 316 insertions(+), 190 deletions(-)

-- 
2.9.3v

[Qemu-devel] [PATCH v2 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.

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

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

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> 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.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Eric Blake]

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

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> 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.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 99 +++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 64 insertions(+), 35 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 --]

[Qemu-devel] [PATCH v2 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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 62 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 33c21d8..ff5def4 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -357,6 +357,49 @@ 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,
+        },
+        {
+            .name = "pool",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "image",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "snapshot",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            /* maps to 'id' in rados_create() */
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "keyvalue-pairs",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     Error *local_err = NULL;
@@ -500,25 +543,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

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

[Eric Blake]

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

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> 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.

Maybe worth adding a comment that keyvalue-pairs will NOT be exposed in
QAPI in the later patches, making it command-line only and
non-introspectible.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 62 ++++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 43 insertions(+), 19 deletions(-)
> 

> +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",

Is "conf" the best name, or do we want "configuration"?

> +            .type = QEMU_OPT_STRING,
> +        },

Worth documenting all the options?

I'm not seeing where "keyvalue-pairs" is used yet, but assume it is in a
later patch. But assuming the QAPI version in a later patch matches,
other than keyvalue-pairs, I think you're okay.

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 v2 2/5] block/rbd: add all the currently supported runtime_opts

[Jeff Cody]

On Mon, Feb 27, 2017 at 04:18:57PM -0600, Eric Blake wrote:
> On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > 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.
> 
> Maybe worth adding a comment that keyvalue-pairs will NOT be exposed in
> QAPI in the later patches, making it command-line only and
> non-introspectible.
>

Yes, I will do that.


> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 62 ++++++++++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 43 insertions(+), 19 deletions(-)
> > 
> 
> > +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",
> 
> Is "conf" the best name, or do we want "configuration"?
> 

I chose "conf" because it matches the rados option name (and the command
line option name; it is of the form "conf=filename").

> > +            .type = QEMU_OPT_STRING,
> > +        },
> 
> Worth documenting all the options?
> 

Yes, probably so - and especially to map them up with what rados/ceph
options they correspond to.

> I'm not seeing where "keyvalue-pairs" is used yet, but assume it is in a
> later patch. But assuming the QAPI version in a later patch matches,
> other than keyvalue-pairs, I think you're okay.
> 

Yep, the next patch (where we switch over to .bdrv_parse_filename()).

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

Thanks!

-Jeff

[Qemu-devel] [PATCH v2 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 empy 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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 298 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 148 insertions(+), 150 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ff5def4..e04a5e1 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,129 @@ 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 {
+            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 +297,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 +306,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 +315,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 +327,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;
         }
     }
 
@@ -406,27 +404,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);
@@ -434,35 +421,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;
     }
@@ -493,6 +500,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;
 }
 
@@ -547,15 +558,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);
@@ -566,44 +572,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) {
@@ -1057,18 +1055,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

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

[Eric Blake]

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

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> 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 empy strings.

s/empy/empty/

> 
> 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.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 298 ++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 148 insertions(+), 150 deletions(-)
> 

> +
> +    /* 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) {

> +        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 {
> +            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));

Uggh.  Why are we compressing this into a single string, instead of
using a GList?  True, we aren't exposing it through QAPI, but I still
wonder if a smarter representation than a flat string is warranted.

> @@ -434,35 +421,55 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>      if (objsize) {
>          if ((objsize - 1) & objsize) {    /* not a power of 2? */

Drive-by comment (if you fix it, do it as a separate followup patch): we
have is_power_of_2() to make code like this more legible.

>  
>  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,

Pointless &; might as well remove it for consistency while touching it.

> +    .bdrv_getlength         = qemu_rbd_getlength,
> +    .bdrv_truncate          = qemu_rbd_truncate,
> +    .protocol_name          = "rbd",
>  

I don't know if it is worth respinning to change keyvalue-pairs into a
more appropriate data type; given our desire to make blockdev-add stable
for 2.9 and the fact that keyvalue-pairs is not exposed to QAPI, I can
live with passing around a flat string.  You may want to add FIXME
comments to call attention to the fact that we know it is gross but why
we do it anyways.

But since adding comments, and fixing minor things like &, doesn't
change the real meat of this patch, I can live 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 v2 3/5] block/rbd: parse all options via bdrv_parse_filename

[Jeff Cody]

On Mon, Feb 27, 2017 at 04:35:58PM -0600, Eric Blake wrote:
> On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > 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 empy strings.
> 
> s/empy/empty/
>

Thanks

> > 
> > 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.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 298 ++++++++++++++++++++++++++++++------------------------------
> >  1 file changed, 148 insertions(+), 150 deletions(-)
> > 
> 
> > +
> > +    /* 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) {
> 
> > +        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 {
> > +            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));
> 
> Uggh.  Why are we compressing this into a single string, instead of
> using a GList?  True, we aren't exposing it through QAPI, but I still
> wonder if a smarter representation than a flat string is warranted.
> 

Yes, a bit gross.  I wouldn't mind cleaning it up (and maybe some other rbd
cleanup as well), but for this series I wanted to keep the "other" parsing
the same as before, and just try to (as much as possible) layer the
blockdev-add QAPI on top.

As you suggest below, I'll add a 'FIXME' here detailing that this is left in
place as legacy code, and should be cleaned up into something more palatable
like a GList (or at least _some_ sort of structured data).

> > @@ -434,35 +421,55 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
> >      if (objsize) {
> >          if ((objsize - 1) & objsize) {    /* not a power of 2? */
> 
> Drive-by comment (if you fix it, do it as a separate followup patch): we
> have is_power_of_2() to make code like this more legible.
>

I'll add that to my post 2.9 rbd cleanup queue, thanks.

> >  
> >  static BlockDriver bdrv_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,
> 
> Pointless &; might as well remove it for consistency while touching it.
> 

I'm not sure I understand - we need the '&' here for the .create_opts
initializer, unless I am overlooking something.

> > +    .bdrv_getlength         = qemu_rbd_getlength,
> > +    .bdrv_truncate          = qemu_rbd_truncate,
> > +    .protocol_name          = "rbd",
> >  
> 
> I don't know if it is worth respinning to change keyvalue-pairs into a
> more appropriate data type; given our desire to make blockdev-add stable
> for 2.9 and the fact that keyvalue-pairs is not exposed to QAPI, I can
> live with passing around a flat string.  You may want to add FIXME
> comments to call attention to the fact that we know it is gross but why
> we do it anyways.
>
> But since adding comments, and fixing minor things like &, doesn't
> change the real meat of this patch, I can live with:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

-Jeff

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

[Eric Blake]

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

On 02/27/2017 04:56 PM, Jeff Cody wrote:

>>>  static BlockDriver bdrv_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,
>>
>> Pointless &; might as well remove it for consistency while touching it.
>>
> 
> I'm not sure I understand - we need the '&' here for the .create_opts
> initializer, unless I am overlooking something.

Nope, I'm overlooking. I assumed that everything here was a function
pointer, but you are right that .create_opts takes an object (not a
function) pointer, so the & is necessary.  Maybe I need to take a short
break from reviewing...

-- 
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 --]

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

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 qapi/block-core.json | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f82d35..5b311ff 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
 ##
@@ -2120,7 +2121,7 @@
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
             'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
-            'vpc', 'vvfat' ] }
+            'vpc', 'vvfat', 'rbd' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2376,7 +2377,6 @@
             'path': 'str',
             '*user': 'str' } }
 
-
 ##
 # @BlkdebugEvent:
 #
@@ -2666,6 +2666,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 +2891,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

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

[Eric Blake]

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

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5f82d35..5b311ff 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
>  ##
> @@ -2120,7 +2121,7 @@
>              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
>              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
>              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> -            'vpc', 'vvfat' ] }
> +            'vpc', 'vvfat', 'rbd' ] }

Please keep the list alphabetical.

>  
>  ##
>  # @BlockdevOptionsFile:
> @@ -2376,7 +2377,6 @@
>              'path': 'str',
>              '*user': 'str' } }
>  
> -
>  ##
>  # @BlkdebugEvent:
>  #

Spurious hunk?

> @@ -2666,6 +2666,34 @@
>              '*timeout': 'int' } }
>  
>  ##
> +# @BlockdevOptionsRbd:
> +#
> +# @pool:               Ceph pool name.
> +#
> +# @image:              Image name in the Ceph pool.
> +#
> +# @conf:               # optional path to Ceph configuration file.  Values

No space between # and optional

> +#                      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.


Indentation off?

> +#
> +# 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 +2891,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',
> 

Omits the problematic keyvalue-pairs, and otherwise matches the previous
patches.  With the nits fixed,

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 --]

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

[Jeff Cody]

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

    mon_host: servername and port
    auth_supported: either 'cephx' or 'none'

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

diff --git a/block/rbd.c b/block/rbd.c
index e04a5e1..51e971e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
             .name = "keyvalue-pairs",
             .type = QEMU_OPT_STRING,
         },
+        {
+            .name = "server.host",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "server.port",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "auth_supported",
+            .type = QEMU_OPT_STRING,
+        },
         { /* end of list */ }
     },
 };
@@ -559,6 +571,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVRBDState *s = bs->opaque;
     const char *pool, *snap, *conf, *clientname, *name, *keypairs;
+    const char *host, *port, *auth_supported;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -580,6 +593,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     clientname     = qemu_opt_get(opts, "user");
     name           = qemu_opt_get(opts, "image");
     keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
+    host           = qemu_opt_get(opts, "server.host");
+    port           = qemu_opt_get(opts, "server.port");
+    auth_supported = qemu_opt_get(opts, "auth_supported");
 
     r = rados_create(&s->cluster, clientname);
     if (r < 0) {
@@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
+    /* if mon_host was specified */
+    if (host) {
+        const char *hostname = host;
+        char *mon_host = NULL;
+
+        if (port) {
+            mon_host = g_strdup_printf("%s:%s", host, port);
+            hostname = mon_host;
+        }
+        r = rados_conf_set(s->cluster, "mon_host", hostname);
+        g_free(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;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b311ff..376512c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2680,6 +2680,12 @@
 #
 # @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.
+#                      Either "cephx" or"none".
+#
 # @password-secret:    #optional The ID of a QCryptoSecret object providing
 #                       the password for the login.
 #
@@ -2691,6 +2697,8 @@
             '*conf': 'str',
             '*snapshot': 'str',
             '*user': 'str',
+            '*server': 'InetSocketAddress',
+            '*auth_supported': 'str',
             '*password-secret': 'str' } }
 
 ##
-- 
2.9.3

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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 01:58:48PM -0500, Jeff Cody wrote:
> This adds support for two additional options that may be specified
> by QAPI in blockdev-add:
> 
>     mon_host: servername and port
>     auth_supported: either 'cephx' or 'none'
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c          | 39 +++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  8 ++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index e04a5e1..51e971e 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
>              .name = "keyvalue-pairs",
>              .type = QEMU_OPT_STRING,
>          },
> +        {
> +            .name = "server.host",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "server.port",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "auth_supported",
> +            .type = QEMU_OPT_STRING,
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -559,6 +571,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>  {
>      BDRVRBDState *s = bs->opaque;
>      const char *pool, *snap, *conf, *clientname, *name, *keypairs;
> +    const char *host, *port, *auth_supported;
>      const char *secretid;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> @@ -580,6 +593,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      clientname     = qemu_opt_get(opts, "user");
>      name           = qemu_opt_get(opts, "image");
>      keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
> +    host           = qemu_opt_get(opts, "server.host");
> +    port           = qemu_opt_get(opts, "server.port");
> +    auth_supported = qemu_opt_get(opts, "auth_supported");
>  
>      r = rados_create(&s->cluster, clientname);
>      if (r < 0) {
> @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    /* if mon_host was specified */
> +    if (host) {
> +        const char *hostname = host;
> +        char *mon_host = NULL;
> +
> +        if (port) {
> +            mon_host = g_strdup_printf("%s:%s", host, port);
> +            hostname = mon_host;
> +        }
> +        r = rados_conf_set(s->cluster, "mon_host", hostname);
> +        g_free(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;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5b311ff..376512c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2680,6 +2680,12 @@
>  #
>  # @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.
> +#                      Either "cephx" or"none".
> +#
>  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>  #                       the password for the login.
>  #
> @@ -2691,6 +2697,8 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> +            '*server': 'InetSocketAddress',

This needs to be an array

> +            '*auth_supported': 'str',

IIUC, you're allowed to list multiple auth options too

>              '*password-secret': 'str' } }
>  


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 v2 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Eric Blake]

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

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> This adds support for two additional options that may be specified
> by QAPI in blockdev-add:
> 
>     mon_host: servername and port
>     auth_supported: either 'cephx' or 'none'

Please spell new options with '-'

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c          | 39 +++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  8 ++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index e04a5e1..51e971e 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
>              .name = "keyvalue-pairs",
>              .type = QEMU_OPT_STRING,
>          },
> +        {
> +            .name = "server.host",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "server.port",
> +            .type = QEMU_OPT_STRING,
> +        },

See Dan's comment about supporting more than one server via an array in
QAPI.

> +        {
> +            .name = "auth_supported",

Should be auth-supported in QAPI.


> @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    /* if mon_host was specified */
> +    if (host) {
> +        const char *hostname = host;
> +        char *mon_host = NULL;
> +
> +        if (port) {
> +            mon_host = g_strdup_printf("%s:%s", host, port);

Does Ceph care about IPv6 (in which case you may need [host]:port when
host itself includes ':')?

> +            hostname = mon_host;
> +        }
> +        r = rados_conf_set(s->cluster, "mon_host", hostname);
> +        g_free(mon_host);
> +        if (r < 0) {
> +            goto failed_shutdown;
> +        }
> +    }
> +
> +    if (auth_supported) {
> +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);

Translating QAPI auth-supported to rados auth_supported is fine.


> +++ b/qapi/block-core.json
> @@ -2680,6 +2680,12 @@
>  #
>  # @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.
> +#                      Either "cephx" or"none".

Missing a space.

If you're going to support only a finite set of strings, this should be
a QAPI enum type, not 'str'.

> +#
>  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>  #                       the password for the login.
>  #
> @@ -2691,6 +2697,8 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> +            '*server': 'InetSocketAddress',
> +            '*auth_supported': 'str',
>              '*password-secret': 'str' } }
>  
>  ##
> 

Looks like we'll need a v3 to tweak this one.

-- 
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 v2 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Jeff Cody]

On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > This adds support for two additional options that may be specified
> > by QAPI in blockdev-add:
> > 
> >     mon_host: servername and port
> >     auth_supported: either 'cephx' or 'none'
> 
> Please spell new options with '-'
> 

OK, thanks.

> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c          | 39 +++++++++++++++++++++++++++++++++++++++
> >  qapi/block-core.json |  8 ++++++++
> >  2 files changed, 47 insertions(+)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index e04a5e1..51e971e 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
> >              .name = "keyvalue-pairs",
> >              .type = QEMU_OPT_STRING,
> >          },
> > +        {
> > +            .name = "server.host",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "server.port",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> 
> See Dan's comment about supporting more than one server via an array in
> QAPI.
> 
> > +        {
> > +            .name = "auth_supported",
> 
> Should be auth-supported in QAPI.
> 
> 

OK

> > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto failed_shutdown;
> >      }
> >  
> > +    /* if mon_host was specified */
> > +    if (host) {
> > +        const char *hostname = host;
> > +        char *mon_host = NULL;
> > +
> > +        if (port) {
> > +            mon_host = g_strdup_printf("%s:%s", host, port);
> 
> Does Ceph care about IPv6 (in which case you may need [host]:port when
> host itself includes ':')?
> 

Good question - it looks like it can be enabled in the conf file at least,
from this: 

http://docs.ceph.com/docs/master/install/manual-deployment/


> > +            hostname = mon_host;
> > +        }
> > +        r = rados_conf_set(s->cluster, "mon_host", hostname);
> > +        g_free(mon_host);
> > +        if (r < 0) {
> > +            goto failed_shutdown;
> > +        }
> > +    }
> > +
> > +    if (auth_supported) {
> > +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
> 
> Translating QAPI auth-supported to rados auth_supported is fine.
> 
> 
> > +++ b/qapi/block-core.json
> > @@ -2680,6 +2680,12 @@
> >  #
> >  # @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.
> > +#                      Either "cephx" or"none".
> 
> Missing a space.
> 
> If you're going to support only a finite set of strings, this should be
> a QAPI enum type, not 'str'.
>

OK, I'll make it an enum (and fix the space, of course).

> > +#
> >  # @password-secret:    #optional The ID of a QCryptoSecret object providing
> >  #                       the password for the login.
> >  #
> > @@ -2691,6 +2697,8 @@
> >              '*conf': 'str',
> >              '*snapshot': 'str',
> >              '*user': 'str',
> > +            '*server': 'InetSocketAddress',
> > +            '*auth_supported': 'str',
> >              '*password-secret': 'str' } }
> >  
> >  ##
> > 
> 
> Looks like we'll need a v3 to tweak this one.
>

Yep - working on that now.  Thanks!

-Jeff

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

[Jeff Cody]

On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > This adds support for two additional options that may be specified
> > by QAPI in blockdev-add:
> > 
> >     mon_host: servername and port
> >     auth_supported: either 'cephx' or 'none'
> 
> Please spell new options with '-'
> 
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c          | 39 +++++++++++++++++++++++++++++++++++++++
> >  qapi/block-core.json |  8 ++++++++
> >  2 files changed, 47 insertions(+)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index e04a5e1..51e971e 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
> >              .name = "keyvalue-pairs",
> >              .type = QEMU_OPT_STRING,
> >          },
> > +        {
> > +            .name = "server.host",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "server.port",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> 
> See Dan's comment about supporting more than one server via an array in
> QAPI.
> 
> > +        {
> > +            .name = "auth_supported",
> 
> Should be auth-supported in QAPI.
> 
> 
> > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto failed_shutdown;
> >      }
> >  
> > +    /* if mon_host was specified */
> > +    if (host) {
> > +        const char *hostname = host;
> > +        char *mon_host = NULL;
> > +
> > +        if (port) {
> > +            mon_host = g_strdup_printf("%s:%s", host, port);
> 
> Does Ceph care about IPv6 (in which case you may need [host]:port when
> host itself includes ':')?
>

Some quick sanity testing seems to show that it does not need [] for ipv6
addresses, which is nice.


> > +            hostname = mon_host;
> > +        }
> > +        r = rados_conf_set(s->cluster, "mon_host", hostname);
> > +        g_free(mon_host);
> > +        if (r < 0) {
> > +            goto failed_shutdown;
> > +        }
> > +    }
> > +
> > +    if (auth_supported) {
> > +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
> 
> Translating QAPI auth-supported to rados auth_supported is fine.
> 
> 
> > +++ b/qapi/block-core.json
> > @@ -2680,6 +2680,12 @@
> >  #
> >  # @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.
> > +#                      Either "cephx" or"none".
> 
> Missing a space.
> 
> If you're going to support only a finite set of strings, this should be
> a QAPI enum type, not 'str'.
> 
> > +#
> >  # @password-secret:    #optional The ID of a QCryptoSecret object providing
> >  #                       the password for the login.
> >  #
> > @@ -2691,6 +2697,8 @@
> >              '*conf': 'str',
> >              '*snapshot': 'str',
> >              '*user': 'str',
> > +            '*server': 'InetSocketAddress',
> > +            '*auth_supported': 'str',
> >              '*password-secret': 'str' } }
> >  
> >  ##
> > 
> 
> Looks like we'll need a v3 to tweak this one.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote:
> On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> > On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > >          goto failed_shutdown;
> > >      }
> > >  
> > > +    /* if mon_host was specified */
> > > +    if (host) {
> > > +        const char *hostname = host;
> > > +        char *mon_host = NULL;
> > > +
> > > +        if (port) {
> > > +            mon_host = g_strdup_printf("%s:%s", host, port);
> > 
> > Does Ceph care about IPv6 (in which case you may need [host]:port when
> > host itself includes ':')?
> >
> 
> Some quick sanity testing seems to show that it does not need [] for ipv6
> addresses, which is nice.

Hmm, that is very odd to me, as that means parsing is ambiguous. The
ceph 'mon_host' option allows the port to be omitted, so given a
string  2001:242:24:23   there's no way of knowing whether it is
a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with
port of 23


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 v2 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Daniel P. Berrange]

On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote:
> On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote:
> > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> > > On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > > >          goto failed_shutdown;
> > > >      }
> > > >  
> > > > +    /* if mon_host was specified */
> > > > +    if (host) {
> > > > +        const char *hostname = host;
> > > > +        char *mon_host = NULL;
> > > > +
> > > > +        if (port) {
> > > > +            mon_host = g_strdup_printf("%s:%s", host, port);
> > > 
> > > Does Ceph care about IPv6 (in which case you may need [host]:port when
> > > host itself includes ':')?
> > >
> > 
> > Some quick sanity testing seems to show that it does not need [] for ipv6
> > addresses, which is nice.
> 
> Hmm, that is very odd to me, as that means parsing is ambiguous. The
> ceph 'mon_host' option allows the port to be omitted, so given a
> string  2001:242:24:23   there's no way of knowing whether it is
> a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with
> port of 23

Looking at the source code to ceph it appears that if you omit the
use of [], then it will treat the entire string as being the address
without port. It does look to support use of [], so we should use
that IIUC.

https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/

See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is
what I think parses the mon addr. NB, I've not tested this yet, so
if you have a live ceph cluster with ipv6, it'd be good to verify that
using [] is correct.

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 v2 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Jeff Cody]

On Tue, Feb 28, 2017 at 10:28:49AM +0000, Daniel P. Berrange wrote:
> On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote:
> > On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote:
> > > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> > > > On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > > > >          goto failed_shutdown;
> > > > >      }
> > > > >  
> > > > > +    /* if mon_host was specified */
> > > > > +    if (host) {
> > > > > +        const char *hostname = host;
> > > > > +        char *mon_host = NULL;
> > > > > +
> > > > > +        if (port) {
> > > > > +            mon_host = g_strdup_printf("%s:%s", host, port);
> > > > 
> > > > Does Ceph care about IPv6 (in which case you may need [host]:port when
> > > > host itself includes ':')?
> > > >
> > > 
> > > Some quick sanity testing seems to show that it does not need [] for ipv6
> > > addresses, which is nice.
> > 
> > Hmm, that is very odd to me, as that means parsing is ambiguous. The
> > ceph 'mon_host' option allows the port to be omitted, so given a
> > string  2001:242:24:23   there's no way of knowing whether it is
> > a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with
> > port of 23
> 
> Looking at the source code to ceph it appears that if you omit the
> use of [], then it will treat the entire string as being the address
> without port. It does look to support use of [], so we should use
> that IIUC.

For the sake of clarity, when you say we should use [] for ipv6, you mean
QEMU (rather than doing it in libvirt)?
> 
> https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/
> 
> See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is
> what I think parses the mon addr. NB, I've not tested this yet, so
> if you have a live ceph cluster with ipv6, it'd be good to verify that
> using [] is correct.
>

Do you think this needs to be in place before either A) we pull the series
for softfreeze, or B) 2.10?  I.e., is this something we can fix up later?
It is OK if not, but I have a flight leaving soon and can't work on the
series anymore -- so depending on this and how v3 review goes, we may just
need to slip the series to 2.10.

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

[Daniel P. Berrange]

On Tue, Feb 28, 2017 at 07:34:52AM -0500, Jeff Cody wrote:
> On Tue, Feb 28, 2017 at 10:28:49AM +0000, Daniel P. Berrange wrote:
> > On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote:
> > > On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote:
> > > > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> > > > > On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > > > > >          goto failed_shutdown;
> > > > > >      }
> > > > > >  
> > > > > > +    /* if mon_host was specified */
> > > > > > +    if (host) {
> > > > > > +        const char *hostname = host;
> > > > > > +        char *mon_host = NULL;
> > > > > > +
> > > > > > +        if (port) {
> > > > > > +            mon_host = g_strdup_printf("%s:%s", host, port);
> > > > > 
> > > > > Does Ceph care about IPv6 (in which case you may need [host]:port when
> > > > > host itself includes ':')?
> > > > >
> > > > 
> > > > Some quick sanity testing seems to show that it does not need [] for ipv6
> > > > addresses, which is nice.
> > > 
> > > Hmm, that is very odd to me, as that means parsing is ambiguous. The
> > > ceph 'mon_host' option allows the port to be omitted, so given a
> > > string  2001:242:24:23   there's no way of knowing whether it is
> > > a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with
> > > port of 23
> > 
> > Looking at the source code to ceph it appears that if you omit the
> > use of [], then it will treat the entire string as being the address
> > without port. It does look to support use of [], so we should use
> > that IIUC.
> 
> For the sake of clarity, when you say we should use [] for ipv6, you mean
> QEMU (rather than doing it in libvirt)?

Yes, in QAPI JSON, it should be the bare IPv6 address in the server
host field. When QEMU conbines the host + port to form the mon_host
string, it must add [] if it sees the host from QAPI is a raw IPv6
address

> > https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/
> > 
> > See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is
> > what I think parses the mon addr. NB, I've not tested this yet, so
> > if you have a live ceph cluster with ipv6, it'd be good to verify that
> > using [] is correct.
> >
> 
> Do you think this needs to be in place before either A) we pull the series
> for softfreeze, or B) 2.10?  I.e., is this something we can fix up later?
> It is OK if not, but I have a flight leaving soon and can't work on the
> series anymore -- so depending on this and how v3 review goes, we may just
> need to slip the series to 2.10.

The fix doesn't affect QAPI protocol spec, so as long as it is fixed
before 2.9 release it'd be ok.

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/ :|