[Qemu-devel] [PATCH RFC v3 for-2.9 00/11] rbd: Clean up API and code

[Qemu-devel] [PATCH RFC v3 for-2.9 00/11] rbd: Clean up API and code

[Markus Armbruster]

This is RFC because it needs more testing.  I'm sending it out now in
the hope of getting some review while we test.

v3:
* PATCH 01-07 unchanged except for a doc tweak in PATCH 06
* PATCH 08-10 replace PATCH 9
* PATCH 8 becomes PATCH 11, rebased on top of 08-10, commit message
  updated, R-by dropped

Markus Armbruster (11):
  rbd: Reject -blockdev server.*.{numeric,to,ipv4,ipv6}
  rbd: Fix to cleanly reject -drive without pool or image
  rbd: Don't limit length of parameter values
  rbd: Clean up after the previous commit
  rbd: Don't accept -drive driver=rbd,keyvalue-pairs=...
  rbd: Clean up runtime_opts, fix -drive to reject filename
  rbd: Clean up qemu_rbd_create()'s detour through QemuOpts
  rbd: Revert -blockdev and -drive parameter auth-supported
  rbd: Revert -blockdev parameter password-secret
  Revert "rbd: add support for getting password from QCryptoSecret
    object"
  rbd: Fix bugs around -drive parameter "server"

 block/rbd.c          | 356 ++++++++++++---------------------------------------
 qapi-schema.json     |  21 ++-
 qapi/block-core.json |  29 +----
 3 files changed, 95 insertions(+), 311 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 01/11] rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

[Markus Armbruster]

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

    -blockdev rbd,node-name=nn,pool=p,image=i,server.0.host=h0,server.0.port=12345,server.0.ipv4=off

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

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

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

[Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Markus Armbruster]

qemu_rbd_open() neglects to check pool and image are present.
Reproducer:

    $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
    Segmentation fault (core dumped)
    $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
    qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)

Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
always sets both pool and image.

Doesn't affect -blockdev, because pool and image are mandatory in the
QAPI schema.

Fix by adding the missing checks.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/rbd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ee13f3d..5ba2a87 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -711,6 +711,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     name           = qemu_opt_get(opts, "image");
     keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
 
+    if (!pool || !name) {
+        error_setg(errp, "Parameters 'pool' and 'image' are required");
+        r = -EINVAL;
+        goto failed_opts;
+    }
+
     r = rados_create(&s->cluster, clientname);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
@@ -718,9 +724,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->snap = g_strdup(snap);
-    if (name) {
-        pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
-    }
+    pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
 
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(s->cluster, conf);
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 03/11] rbd: Don't limit length of parameter values

[Markus Armbruster]

We laboriously enforce parameter values are between one and some
arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
librbd.h, and I'm not sure it applies.  Where the other limits come
from is unclear.

Drop the length checking.  The limits librbd actually imposes must be
checked by librbd anyway.

There's one minor complication: BDRVRBDState member name is a
fixed-size array.  Depends on the length limit.  Make it a pointer to
a dynamically allocated string.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/rbd.c | 91 ++++++++++---------------------------------------------------
 1 file changed, 14 insertions(+), 77 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 5ba2a87..0fea348 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -56,11 +56,6 @@
 
 #define OBJ_MAX_SIZE (1UL << OBJ_DEFAULT_OBJ_ORDER)
 
-#define RBD_MAX_CONF_NAME_SIZE 128
-#define RBD_MAX_CONF_VAL_SIZE 512
-#define RBD_MAX_CONF_SIZE 1024
-#define RBD_MAX_POOL_NAME_SIZE 128
-#define RBD_MAX_SNAP_NAME_SIZE 128
 #define RBD_MAX_SNAPS 100
 
 /* The LIBRBD_SUPPORTS_IOVEC is defined in librbd.h */
@@ -99,16 +94,12 @@ typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
     rbd_image_t image;
-    char name[RBD_MAX_IMAGE_NAME_SIZE];
+    char *name;
     char *snap;
 } BDRVRBDState;
 
-static char *qemu_rbd_next_tok(int max_len,
-                               char *src, char delim,
-                               const char *name,
-                               char **p, Error **errp)
+static char *qemu_rbd_next_tok(char *src, char delim, char **p)
 {
-    int l;
     char *end;
 
     *p = NULL;
@@ -127,15 +118,6 @@ static char *qemu_rbd_next_tok(int max_len,
             *end = '\0';
         }
     }
-    l = strlen(src);
-    if (l >= max_len) {
-        error_setg(errp, "%s too long", name);
-        return NULL;
-    } else if (l == 0) {
-        error_setg(errp, "%s too short", name);
-        return NULL;
-    }
-
     return src;
 }
 
@@ -159,7 +141,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     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:'");
@@ -171,11 +152,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     keypairs = g_malloc0(max_keypair_size);
     p = buf;
 
-    found_str = qemu_rbd_next_tok(RBD_MAX_POOL_NAME_SIZE, p,
-                                  '/', "pool name", &p, &local_err);
-    if (local_err) {
-        goto done;
-    }
+    found_str = qemu_rbd_next_tok(p, '/', &p);
     if (!p) {
         error_setg(errp, "Pool name is required");
         goto done;
@@ -184,27 +161,15 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     qdict_put(options, "pool", qstring_from_str(found_str));
 
     if (strchr(p, '@')) {
-        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
-                                      '@', "object name", &p, &local_err);
-        if (local_err) {
-            goto done;
-        }
+        found_str = qemu_rbd_next_tok(p, '@', &p);
         qemu_rbd_unescape(found_str);
         qdict_put(options, "image", qstring_from_str(found_str));
 
-        found_str = qemu_rbd_next_tok(RBD_MAX_SNAP_NAME_SIZE, p,
-                                      ':', "snap name", &p, &local_err);
-        if (local_err) {
-            goto done;
-        }
+        found_str = qemu_rbd_next_tok(p, ':', &p);
         qemu_rbd_unescape(found_str);
         qdict_put(options, "snapshot", qstring_from_str(found_str));
     } else {
-        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
-                                      ':', "object name", &p, &local_err);
-        if (local_err) {
-            goto done;
-        }
+        found_str = qemu_rbd_next_tok(p, ':', &p);
         qemu_rbd_unescape(found_str);
         qdict_put(options, "image", qstring_from_str(found_str));
     }
@@ -212,11 +177,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
         goto done;
     }
 
-    found_str = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
-                                  '\0', "configuration", &p, &local_err);
-    if (local_err) {
-        goto done;
-    }
+    found_str = qemu_rbd_next_tok(p, '\0', &p);
 
     p = found_str;
 
@@ -224,12 +185,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
      * '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;
-        }
-
+        name = qemu_rbd_next_tok(p, '=', &p);
         if (!p) {
             error_setg(errp, "conf option %s has no value", name);
             break;
@@ -237,11 +193,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
 
         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;
-        }
+        value = qemu_rbd_next_tok(p, ':', &p);
         qemu_rbd_unescape(value);
 
         if (!strcmp(name, "conf")) {
@@ -274,9 +226,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
 
 
 done:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
     g_free(buf);
     g_free(keypairs);
     return;
@@ -308,30 +257,20 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
     char *p, *buf;
     char *name;
     char *value;
-    Error *local_err = NULL;
     int ret = 0;
 
     buf = g_strdup(keypairs);
     p = buf;
 
     while (p) {
-        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
-                                 '=', "conf option name", &p, &local_err);
-        if (local_err) {
-            break;
-        }
-
+        name = qemu_rbd_next_tok(p, '=', &p);
         if (!p) {
             error_setg(errp, "conf option %s has no value", name);
             ret = -EINVAL;
             break;
         }
 
-        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
-                                  ':', "conf option value", &p, &local_err);
-        if (local_err) {
-            break;
-        }
+        value = qemu_rbd_next_tok(p, ':', &p);
 
         ret = rados_conf_set(cluster, name, value);
         if (ret < 0) {
@@ -341,10 +280,6 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
         }
     }
 
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-    }
     g_free(buf);
     return ret;
 }
@@ -724,7 +659,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->snap = g_strdup(snap);
-    pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
+    s->name = g_strdup(name);
 
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(s->cluster, conf);
@@ -798,6 +733,7 @@ failed_open:
 failed_shutdown:
     rados_shutdown(s->cluster);
     g_free(s->snap);
+    g_free(s->name);
 failed_opts:
     qemu_opts_del(opts);
     g_free(mon_host);
@@ -812,6 +748,7 @@ static void qemu_rbd_close(BlockDriverState *bs)
     rbd_close(s->image);
     rados_ioctx_destroy(s->io_ctx);
     g_free(s->snap);
+    g_free(s->name);
     rados_shutdown(s->cluster);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 04/11] rbd: Clean up after the previous commit

[Markus Armbruster]

This code in qemu_rbd_parse_filename()

    found_str = qemu_rbd_next_tok(p, '\0', &p);
    p = found_str;

has no effect.  Drop it, and simplify qemu_rbd_next_tok().

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

diff --git a/block/rbd.c b/block/rbd.c
index 0fea348..182a5a3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -104,19 +104,17 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
 
     *p = NULL;
 
-    if (delim != '\0') {
-        for (end = src; *end; ++end) {
-            if (*end == delim) {
-                break;
-            }
-            if (*end == '\\' && end[1] != '\0') {
-                end++;
-            }
-        }
+    for (end = src; *end; ++end) {
         if (*end == delim) {
-            *p = end + 1;
-            *end = '\0';
+            break;
         }
+        if (*end == '\\' && end[1] != '\0') {
+            end++;
+        }
+    }
+    if (*end == delim) {
+        *p = end + 1;
+        *end = '\0';
     }
     return src;
 }
@@ -177,10 +175,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
         goto done;
     }
 
-    found_str = qemu_rbd_next_tok(p, '\0', &p);
-
-    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) {
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 05/11] rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

[Markus Armbruster]

The way we communicate extra key-value pairs from
qemu_rbd_parse_filename() to qemu_rbd_open() exposes option parameter
"keyvalue-pairs" on the command line.  It's not wanted there.  Hack:
rename the parameter to "=keyvalue-pairs" to make it inaccessible.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/rbd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 182a5a3..2632533 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -215,7 +215,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     }
 
     if (keypairs[0]) {
-        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));
+        qdict_put(options, "=keyvalue-pairs", qstring_from_str(keypairs));
     }
 
 
@@ -330,7 +330,11 @@ static QemuOptsList runtime_opts = {
             .help = "Rados id name",
         },
         {
-            .name = "keyvalue-pairs",
+            /*
+             * HACK: name starts with '=' so that qemu_opts_parse()
+             * can't set it
+             */
+            .name = "=keyvalue-pairs",
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
@@ -405,7 +409,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     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");
+    keypairs   = qemu_opt_get(rbd_opts, "=keyvalue-pairs");
 
     ret = rados_create(&cluster, clientname);
     if (ret < 0) {
@@ -638,7 +642,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     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");
+    keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 
     if (!pool || !name) {
         error_setg(errp, "Parameters 'pool' and 'image' are required");
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 06/11] rbd: Clean up runtime_opts, fix -drive to reject filename

[Markus Armbruster]

runtime_opts is used for three different purposes:

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

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

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

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

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

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

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

* Delete "filename", because none of the three users actually uses it.
  This fixes -drive to reject parameter filename instead of silently
  ignoring it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/rbd.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 2632533..b2afe07 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -294,21 +294,6 @@ static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-            .help = "Specification of the rbd image",
-        },
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
-        {
-            .name = "conf",
-            .type = QEMU_OPT_STRING,
-            .help = "Rados config file location",
-        },
-        {
             .name = "pool",
             .type = QEMU_OPT_STRING,
             .help = "Rados pool name",
@@ -319,6 +304,11 @@ static QemuOptsList runtime_opts = {
             .help = "Image name in the pool",
         },
         {
+            .name = "conf",
+            .type = QEMU_OPT_STRING,
+            .help = "Rados config file location",
+        },
+        {
             .name = "snapshot",
             .type = QEMU_OPT_STRING,
             .help = "Ceph snapshot name",
@@ -329,6 +319,19 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Rados id name",
         },
+        /*
+         * server.* and auth-supported.* extracted manually, see
+         * qemu_rbd_array_opts()
+         */
+        {
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret providing the password",
+        },
+
+        /*
+         * Keys for qemu_rbd_parse_filename(), not in the QAPI schema
+         */
         {
             /*
              * HACK: name starts with '=' so that qemu_opts_parse()
@@ -338,6 +341,13 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
+
+        /*
+         * The remainder aren't option keys, but option sub-sub-keys,
+         * so that qemu_rbd_array_opts() can abuse runtime_opts for
+         * its own purposes
+         * TODO clean this up
+         */
         {
             .name = "host",
             .type = QEMU_OPT_STRING,
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 07/11] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Markus Armbruster]

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

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

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

[Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Markus Armbruster]

This reverts half of commit 0a55679.  We're having second thoughts on
the QAPI schema (and thus the external interface), and haven't reached
consensus, yet.  Issues include:

* The implementation uses deprecated rados_conf_set() key
  "auth_supported".  No biggie.

* The implementation makes -drive silently ignore invalid parameters
  "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
  fact I'm going to fix similar bugs around parameter server), so
  again no biggie.

* BlockdevOptionsRbd member @password-secret applies only to
  authentication method cephx.  Should it be a variant member of
  RbdAuthMethod?

* BlockdevOptionsRbd member @user could apply to both methods cephx
  and none, but I'm not sure it's actually used with none.  If it
  isn't, should it be a variant member of RbdAuthMethod?

* The client offers a *set* of authentication methods, not a list.
  Should the methods be optional members of BlockdevOptionsRbd instead
  of members of list @auth-supported?  The latter begs the question
  what multiple entries for the same method mean.  Trivial question
  now that RbdAuthMethod contains nothing but @type, but less so when
  RbdAuthMethod acquires other members, such the ones discussed above.

* How BlockdevOptionsRbd member @auth-supported interacts with
  settings from a configuration file specified with @conf is
  undocumented.  I suspect it's untested, too.

Let's avoid painting ourselves into a corner now, and revert the
feature for 2.9.

Note that users can still configure authentication methods with a
configuration file.  They probably do that anyway if they use Ceph
outside QEMU as well.

qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
which is silly.  This will be cleaned up shortly.

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

diff --git a/block/rbd.c b/block/rbd.c
index cf0bab0..103ce44 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -320,8 +320,7 @@ static QemuOptsList runtime_opts = {
             .help = "Rados id name",
         },
         /*
-         * server.* and auth-supported.* extracted manually, see
-         * qemu_rbd_array_opts()
+         * server.* extracted manually, see qemu_rbd_array_opts()
          */
         {
             .name = "password-secret",
@@ -356,11 +355,6 @@ static QemuOptsList runtime_opts = {
             .name = "port",
             .type = QEMU_OPT_STRING,
         },
-        {
-            .name = "auth",
-            .type = QEMU_OPT_STRING,
-            .help = "Supported authentication method, either cephx or none",
-        },
         { /* end of list */ }
     },
 };
@@ -512,7 +506,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
 }
 
 #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)
@@ -527,7 +520,7 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
     Error *local_err = NULL;
     int i;
 
-    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
+    assert(type == RBD_MON_HOST);
 
     num_entries = qdict_array_entries(options, prefix);
 
@@ -573,10 +566,9 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
                 value = strbuf;
             }
         } else {
-            value = qemu_opt_get(opts, "auth");
+            abort();
         }
 
-
         /* 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) {
@@ -608,7 +600,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts;
     Error *local_err = NULL;
     char *mon_host = NULL;
-    char *auth_supported = NULL;
     int r;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -619,14 +610,6 @@ 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) {
@@ -678,13 +661,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    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;
@@ -735,7 +711,6 @@ failed_shutdown:
 failed_opts:
     qemu_opts_del(opts);
     g_free(mon_host);
-    g_free(auth_supported);
     return r;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5d2efe4..6a7ca0b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2601,27 +2601,6 @@
 
 
 ##
-# @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:
 #
 # @pool:               Ceph pool name.
@@ -2639,8 +2618,6 @@
 # @server:             Monitor host address and port.  This maps
 #                      to the "mon_host" Ceph option.
 #
-# @auth-supported:     Authentication supported.
-#
 # @password-secret:    The ID of a QCryptoSecret object providing
 #                      the password for the login.
 #
@@ -2653,7 +2630,6 @@
             '*snapshot': 'str',
             '*user': 'str',
             '*server': ['InetSocketAddressBase'],
-            '*auth-supported': ['RbdAuthMethod'],
             '*password-secret': 'str' } }
 
 ##
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Markus Armbruster]

This reverts a part of commit 8a47e8e.  We're having second thoughts
on the QAPI schema (and thus the external interface), and haven't
reached consensus, yet.  Issues include:

* BlockdevOptionsRbd member @password-secret isn't actually a
  password, it's a key generated by Ceph.

* We're not sure where member @password-secret belongs (see the
  previous commit).

* How @password-secret interacts with settings from a configuration
  file specified with @conf is undocumented.  I suspect it's untested,
  too.

Let's avoid painting ourselves into a corner now, and revert the
feature for 2.9.

Note that users can still configure an authentication key with a
configuration file.  They probably do that anyway if they use Ceph
outside QEMU as well.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6a7ca0b..2e60ab5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2618,9 +2618,6 @@
 # @server:             Monitor host address and port.  This maps
 #                      to the "mon_host" Ceph option.
 #
-# @password-secret:    The ID of a QCryptoSecret object providing
-#                      the password for the login.
-#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsRbd',
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 10/11] Revert "rbd: add support for getting password from QCryptoSecret object"

[Markus Armbruster]

This reverts commit 60390a2192e7b38aee18db6ce7fb740498709737.

The commit's rationale

    Currently RBD passwords must be provided on the command line
    via

      $QEMU -drive file=rbd:pool/image:id=myname:\
                   key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
                   auth_supported=cephx

    This is insecure because the key is visible in the OS process
    listing.

is invalid.  You can easily avoid passing keys on the command line by
using "keyfile" instead of "key".  In fact, the Ceph documentation
calls use of key "not recommended".  But the most common way to
provide keys is a keyring.  The default keyrings should be just fine
for most users.  When they aren't, you can configure your own keyrings
with "keyring" or override the key with "keyfile".

The commit adds parameter password-secret to -drive.  Support for it
was included in -blockdev, but reverted in the previous commit due to
concerns about the QMP interface.  Revert it from -drive, too.

Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 47 -----------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 103ce44..5a58d3e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -16,7 +16,6 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
-#include "crypto/secret.h"
 #include "qemu/cutils.h"
 #include "qapi/qmp/qstring.h"
 
@@ -225,26 +224,6 @@ done:
     return;
 }
 
-
-static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
-                             Error **errp)
-{
-    if (secretid == 0) {
-        return 0;
-    }
-
-    gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
-                                                    errp);
-    if (!secret) {
-        return -1;
-    }
-
-    rados_conf_set(cluster, "key", secret);
-    g_free(secret);
-
-    return 0;
-}
-
 static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
                                  Error **errp)
 {
@@ -322,11 +301,6 @@ static QemuOptsList runtime_opts = {
         /*
          * server.* extracted manually, see qemu_rbd_array_opts()
          */
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
 
         /*
          * Keys for qemu_rbd_parse_filename(), not in the QAPI schema
@@ -366,14 +340,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t objsize;
     int obj_order = 0;
     const char *pool, *name, *conf, *clientname, *keypairs;
-    const char *secretid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
     QDict *options = NULL;
     int ret = 0;
 
-    secretid = qemu_opt_get(opts, "password-secret");
-
     /* Read out options */
     bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                      BDRV_SECTOR_SIZE);
@@ -426,11 +397,6 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto shutdown;
     }
 
-    if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) {
-        ret = -EIO;
-        goto shutdown;
-    }
-
     ret = rados_connect(cluster);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error connecting");
@@ -596,7 +562,6 @@ 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 *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
     char *mon_host = NULL;
@@ -618,8 +583,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_opts;
     }
 
-    secretid = qemu_opt_get(opts, "password-secret");
-
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
@@ -661,11 +624,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
-        r = -EIO;
-        goto failed_shutdown;
-    }
-
     /*
      * Fallback to more conservative semantics if setting cache
      * options fails. Ignore errors from setting rbd_cache because the
@@ -1105,11 +1063,6 @@ static QemuOptsList qemu_rbd_create_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "RBD object size"
         },
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
         { /* end of list */ }
     }
 };
-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 for-2.9 11/11] rbd: Fix bugs around -drive parameter "server"

[Markus Armbruster]

qemu_rbd_open() takes option parameters as a flattened QDict, with
keys of the form server.%d.host, server.%d.port, where %d counts up
from zero.

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

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

The extraction of keys starting with server.%d into another QDict
makes us ignore parameters like server.0.neither-host-nor-port
silently.

The conversion to QemuOpts abuses runtime_opts, as described a few
commits ago.

Rewrite to simply get the values straight from the options QDict.

Fixes -drive not to crash when server.*.* are present, but
server.*.host is absent.

Fixes -drive to reject invalid server.*.*.

Permits cleaning up runtime_opts.  Do that, and fix -drive to reject
bogus parameters host and port instead of silently ignoring them.

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

diff --git a/block/rbd.c b/block/rbd.c
index 5a58d3e..7c6f084 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -13,14 +13,13 @@
 
 #include "qemu/osdep.h"
 
+#include <rbd/librbd.h>
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
 #include "qemu/cutils.h"
 #include "qapi/qmp/qstring.h"
 
-#include <rbd/librbd.h>
-
 /*
  * When specifying the image filename use:
  *
@@ -299,7 +298,7 @@ static QemuOptsList runtime_opts = {
             .help = "Rados id name",
         },
         /*
-         * server.* extracted manually, see qemu_rbd_array_opts()
+         * server.* extracted manually, see qemu_rbd_mon_host()
          */
 
         /*
@@ -314,21 +313,6 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Legacy rados key/value option parameters",
         },
-
-        /*
-         * The remainder aren't option keys, but option sub-sub-keys,
-         * so that qemu_rbd_array_opts() can abuse runtime_opts for
-         * its own purposes
-         * TODO clean this up
-         */
-        {
-            .name = "host",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "port",
-            .type = QEMU_OPT_STRING,
-        },
         { /* end of list */ }
     },
 };
@@ -471,89 +455,43 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-#define RBD_MON_HOST          0
-
-static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
-                                 Error **errp)
+static char *qemu_rbd_mon_host(QDict *options, Error **errp)
 {
-    int num_entries;
-    QemuOpts *opts = NULL;
-    QDict *sub_options;
-    const char *host;
-    const char *port;
-    char *str;
-    char *rados_str = NULL;
-    Error *local_err = NULL;
+    const char **vals = g_new(const char *, qdict_size(options) + 1);
+    char keybuf[32];
+    const char *host, *port;
+    char *rados_str;
     int i;
 
-    assert(type == RBD_MON_HOST);
-
-    num_entries = qdict_array_entries(options, prefix);
-
-    if (num_entries < 0) {
-        error_setg(errp, "Parse error on RBD QDict array");
-        return NULL;
-    }
-
-    for (i = 0; i < num_entries; i++) {
-        char *strbuf = NULL;
-        const char *value;
-        char *rados_str_tmp;
-
-        str = g_strdup_printf("%s%d.", prefix, i);
-        qdict_extract_subqdict(options, &sub_options, str);
-        g_free(str);
-
-        opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-        qemu_opts_absorb_qdict(opts, sub_options, &local_err);
-        QDECREF(sub_options);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            g_free(rados_str);
+    for (i = 0;; i++) {
+        sprintf(keybuf, "server.%d.host", i);
+        host = qdict_get_try_str(options, keybuf);
+        qdict_del(options, keybuf);
+        sprintf(keybuf, "server.%d.port", i);
+        port = qdict_get_try_str(options, keybuf);
+        qdict_del(options, keybuf);
+        if (!host && !port) {
+            break;
+        }
+        if (!host) {
+            error_setg(errp, "Parameter server.%d.host is missing", i);
             rados_str = NULL;
-            goto exit;
+            goto out;
         }
 
-        if (type == RBD_MON_HOST) {
-            host = qemu_opt_get(opts, "host");
-            port = qemu_opt_get(opts, "port");
-
-            value = host;
-            if (port) {
-                /* check for ipv6 */
-                if (strchr(host, ':')) {
-                    strbuf = g_strdup_printf("[%s]:%s", host, port);
-                } else {
-                    strbuf = g_strdup_printf("%s:%s", host, port);
-                }
-                value = strbuf;
-            } else if (strchr(host, ':')) {
-                strbuf = g_strdup_printf("[%s]", host);
-                value = strbuf;
-            }
+        if (strchr(host, ':')) {
+            vals[i] = port ? g_strdup_printf("[%s]:%s", host, port)
+                : g_strdup_printf("[%s]", host);
         } else {
-            abort();
+            vals[i] = port ? g_strdup_printf("%s:%s", host, port)
+                : g_strdup(host);
         }
-
-        /* 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(strbuf);
-        qemu_opts_del(opts);
-        opts = NULL;
     }
+    vals[i] = NULL;
 
-exit:
-    qemu_opts_del(opts);
+    rados_str = i ? g_strjoinv(";", (char **)vals) : NULL;
+out:
+    g_strfreev((char **)vals);
     return rados_str;
 }
 
@@ -571,12 +509,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        qemu_opts_del(opts);
-        return -EINVAL;
+        r = -EINVAL;
+        goto failed_opts;
     }
 
-    mon_host = qemu_rbd_array_opts(options, "server.",
-                                   RBD_MON_HOST, &local_err);
+    mon_host = qemu_rbd_mon_host(options, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         r = -EINVAL;
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 01/11] rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> We use InetSocketAddress in the QAPI schema.  However, the code
> doesn't use inet_connect_saddr(), but formats "host" and "port" into a
> configuration string for rados_conf_set().  Thus, members "numeric",
> "to", "ipv4" and "ipv6" are silently ignored.  Not nice.  Example:
> 
>     -blockdev rbd,node-name=nn,pool=p,image=i,server.0.host=h0,server.0.port=12345,server.0.ipv4=off
> 
> Factor a suitable InetSocketAddressBase out of InetSocketAddress, and
> use that.  "numeric", "to", "ipv4" and "ipv6" are now rejected.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi-schema.json     | 21 ++++++++++++++-------
>  qapi/block-core.json |  2 +-
>  2 files changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> qemu_rbd_open() neglects to check pool and image are present.
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>     Segmentation fault (core dumped)
>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
> 
> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> always sets both pool and image.
> 
> Doesn't affect -blockdev, because pool and image are mandatory in the
> QAPI schema.
> 
> Fix by adding the missing checks.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/rbd.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Max Reitz]

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

On 27.03.2017 18:10, Max Reitz wrote:
> On 27.03.2017 15:26, Markus Armbruster wrote:
>> qemu_rbd_open() neglects to check pool and image are present.
>> Reproducer:
>>
>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>>     Segmentation fault (core dumped)
>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
>>
>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
>> always sets both pool and image.
>>
>> Doesn't affect -blockdev, because pool and image are mandatory in the
>> QAPI schema.
>>
>> Fix by adding the missing checks.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  block/rbd.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

That said, don't we have a similar issue with qemu_rbd_create()? It too
doesn't check whether those options are given but I guess they're just
as mandatory.

Max


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 03/11] rbd: Don't limit length of parameter values

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> We laboriously enforce parameter values are between one and some
> arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
> librbd.h, and I'm not sure it applies.  Where the other limits come
> from is unclear.
> 
> Drop the length checking.  The limits librbd actually imposes must be
> checked by librbd anyway.
> 
> There's one minor complication: BDRVRBDState member name is a
> fixed-size array.  Depends on the length limit.  Make it a pointer to
> a dynamically allocated string.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/rbd.c | 91 ++++++++++---------------------------------------------------
>  1 file changed, 14 insertions(+), 77 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 04/11] rbd: Clean up after the previous commit

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> This code in qemu_rbd_parse_filename()
> 
>     found_str = qemu_rbd_next_tok(p, '\0', &p);
>     p = found_str;
> 
> has no effect.  Drop it, and simplify qemu_rbd_next_tok().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/rbd.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 05/11] rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> The way we communicate extra key-value pairs from
> qemu_rbd_parse_filename() to qemu_rbd_open() exposes option parameter
> "keyvalue-pairs" on the command line.  It's not wanted there.  Hack:
> rename the parameter to "=keyvalue-pairs" to make it inaccessible.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/rbd.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

(Or you could just use x- like blkdebug does...)

Max


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 06/11] rbd: Clean up runtime_opts, fix -drive to reject filename

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> runtime_opts is used for three different purposes:
> 
> * qemu_rbd_open() uses it to accept options it recognizes, such as
>   "pool" and "image".  Other .bdrv_open() methods do it similarly.
> 
> * qemu_rbd_open() accepts additional list-valued options
>   auth-supported and server, with the help of qemu_rbd_array_opts().
>   The list elements are again dictionaries.  qemu_rbd_array_opts()
>   uses runtime_opts to accept their members.  Thus, runtime_opts
>   contains recognized sub-sub-options "auth", "host", "port" in
>   addition to recognized options.  No other block driver does that.
> 
> * qemu_rbd_create() uses it to convert the QDict produced by
>   qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
>   that.  The keys produced by qemu_rbd_parse_filename() are "pool",
>   "image", "snapshot", "conf", "user" and "keyvalue-pairs".
>   qemu_rbd_open() accepts these, so no additional ones here.
> 
> This is a confusing mess.  Dates back to commit 0f9d252.  First step
> to clean it up is documenting runtime_opts.desc[]:
> 
> * Reorder entries to match the QAPI schema, like we do in other block
>   drivers.
> 
> * Document why the schema's "server" and "auth-supported" aren't in
>   .desc[].
> 
> * Document why "keyvalue-pairs", "host", "port" and "auth" are in
>   .desc[], but not the schema.
> 
> * Delete "filename", because none of the three users actually uses it.
>   This fixes -drive to reject parameter filename instead of silently
>   ignoring it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/rbd.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 07/11] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> The conversion from QDict to QemuOpts is pointless.  Simply get the
> stuff straight from the QDict.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/rbd.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index b2afe07..cf0bab0 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -376,7 +376,6 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>      rados_t cluster;
>      rados_ioctx_t io_ctx;
>      QDict *options = NULL;
> -    QemuOpts *rbd_opts = NULL;
>      int ret = 0;
>  
>      secretid = qemu_opt_get(opts, "password-secret");
> @@ -407,19 +406,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>          goto exit;
>      }
>  
> -    rbd_opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> -    qemu_opts_absorb_qdict(rbd_opts, options, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        ret = -EINVAL;
> -        goto exit;
> -    }
> -
> -    pool       = qemu_opt_get(rbd_opts, "pool");
> -    conf       = qemu_opt_get(rbd_opts, "conf");
> -    clientname = qemu_opt_get(rbd_opts, "user");
> -    name       = qemu_opt_get(rbd_opts, "image");
> -    keypairs   = qemu_opt_get(rbd_opts, "=keyvalue-pairs");
> +    pool       = qdict_get_str(options, "pool");
> +    conf       = qdict_get_str(options, "conf");
> +    clientname = qdict_get_str(options, "user");
> +    name       = qdict_get_str(options, "image");
> +    keypairs   = qdict_get_str(options, "=keyvalue-pairs");

This assumes that all of these options are given which they are not
necessarily:

$ ./qemu-img create -f rbd rbd:blub/ 1M
Formatting 'rbd:blub/', fmt=rbd size=1048576
[1]    27387 segmentation fault (core dumped)  ./qemu-img create -f rbd
rbd:blub/ 1M

qdict_get_try_str() would probably be better.

Max

>  
>      ret = rados_create(&cluster, clientname);
>      if (ret < 0) {
> @@ -470,7 +461,6 @@ shutdown:
>  
>  exit:
>      QDECREF(options);
> -    qemu_opts_del(rbd_opts);
>      return ret;
>  }
>  
> 



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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> This reverts half of commit 0a55679.  We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet.  Issues include:
> 
> * The implementation uses deprecated rados_conf_set() key
>   "auth_supported".  No biggie.
> 
> * The implementation makes -drive silently ignore invalid parameters
>   "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
>   fact I'm going to fix similar bugs around parameter server), so
>   again no biggie.
> 
> * BlockdevOptionsRbd member @password-secret applies only to
>   authentication method cephx.  Should it be a variant member of
>   RbdAuthMethod?
> 
> * BlockdevOptionsRbd member @user could apply to both methods cephx
>   and none, but I'm not sure it's actually used with none.  If it
>   isn't, should it be a variant member of RbdAuthMethod?
> 
> * The client offers a *set* of authentication methods, not a list.
>   Should the methods be optional members of BlockdevOptionsRbd instead
>   of members of list @auth-supported?  The latter begs the question
>   what multiple entries for the same method mean.  Trivial question
>   now that RbdAuthMethod contains nothing but @type, but less so when
>   RbdAuthMethod acquires other members, such the ones discussed above.
> 
> * How BlockdevOptionsRbd member @auth-supported interacts with
>   settings from a configuration file specified with @conf is
>   undocumented.  I suspect it's untested, too.
> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure authentication methods with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly.  This will be cleaned up shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          | 31 +++----------------------------
>  qapi/block-core.json | 24 ------------------------
>  2 files changed, 3 insertions(+), 52 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Max Reitz]

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

On 27.03.2017 15:26, Markus Armbruster wrote:
> This reverts a part of commit 8a47e8e.  We're having second thoughts
> on the QAPI schema (and thus the external interface), and haven't
> reached consensus, yet.  Issues include:
> 
> * BlockdevOptionsRbd member @password-secret isn't actually a
>   password, it's a key generated by Ceph.
> 
> * We're not sure where member @password-secret belongs (see the
>   previous commit).
> 
> * How @password-secret interacts with settings from a configuration
>   file specified with @conf is undocumented.  I suspect it's untested,
>   too.
> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure an authentication key with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Eric Blake]

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

On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> This reverts half of commit 0a55679.  We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet.  Issues include:
> 

> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure authentication methods with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.

If we're only reverting the QMP blockdev-add feature, then this makes
absolute sense (it's not a regression since we don't have a release with
it yet, and we don't want to bake something into the release that can't
be supported).  But breaking -drive usage seems risky, especially since
libvirt is already expecting to work - I'm worried that doing this may
break existing libvirt command line usage if the QemuOpts side doesn't
permit anything at all.  Maybe we need to rely on your '=foo' or 'x-foo'
hack for letting QemuOpts still accept the old spelling during -drive
but not during QMP.

> 
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly.  This will be cleaned up shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          | 31 +++----------------------------
>  qapi/block-core.json | 24 ------------------------
>  2 files changed, 3 insertions(+), 52 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2601,27 +2601,6 @@
>  
>  
>  ##
> -# @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' } }
> -

Removing the .json QMP support is fine. But I'm reluctant to give R-b
without knowing for sure that -drive usage won't regress.

-- 
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 RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Eric Blake]

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

On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> This reverts a part of commit 8a47e8e.  We're having second thoughts
> on the QAPI schema (and thus the external interface), and haven't
> reached consensus, yet.  Issues include:
> 
> * BlockdevOptionsRbd member @password-secret isn't actually a
>   password, it's a key generated by Ceph.
> 
> * We're not sure where member @password-secret belongs (see the
>   previous commit).
> 
> * How @password-secret interacts with settings from a configuration
>   file specified with @conf is undocumented.  I suspect it's untested,
>   too.
> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure an authentication key with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json | 3 ---
>  1 file changed, 3 deletions(-)

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

This time, it's fairly obvious that you aren't affecting -drive usage.

> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6a7ca0b..2e60ab5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2618,9 +2618,6 @@
>  # @server:             Monitor host address and port.  This maps
>  #                      to the "mon_host" Ceph option.
>  #
> -# @password-secret:    The ID of a QCryptoSecret object providing
> -#                      the password for the login.
> -#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsRbd',
> 

-- 
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 RFC v3 for-2.9 10/11] Revert "rbd: add support for getting password from QCryptoSecret object"

[Eric Blake]

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

On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> This reverts commit 60390a2192e7b38aee18db6ce7fb740498709737.
> 
> The commit's rationale
> 
>     Currently RBD passwords must be provided on the command line
>     via
> 
>       $QEMU -drive file=rbd:pool/image:id=myname:\
>                    key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
>                    auth_supported=cephx
> 
>     This is insecure because the key is visible in the OS process
>     listing.
> 
> is invalid.  You can easily avoid passing keys on the command line by
> using "keyfile" instead of "key".  In fact, the Ceph documentation
> calls use of key "not recommended".  But the most common way to
> provide keys is a keyring.  The default keyrings should be just fine
> for most users.  When they aren't, you can configure your own keyrings
> with "keyring" or override the key with "keyfile".
> 
> The commit adds parameter password-secret to -drive.  Support for it
> was included in -blockdev, but reverted in the previous commit due to
> concerns about the QMP interface.  Revert it from -drive, too.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 47 -----------------------------------------------
>  1 file changed, 47 deletions(-)

Are we sure this won't be breaking existing libvirt clients?

-- 
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 RFC v3 for-2.9 11/11] rbd: Fix bugs around -drive parameter "server"

[Eric Blake]

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

On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> qemu_rbd_open() takes option parameters as a flattened QDict, with
> keys of the form server.%d.host, server.%d.port, where %d counts up
> from zero.
> 
> qemu_rbd_array_opts() extracts these values as follows.  First, it
> calls qdict_array_entries() to find the list's length.  For each list
> element, it formats the list's key prefix (e.g. "server.0."), then
> creates a new QDict holding the options with that key prefix, then
> converts that to a QemuOpts, so it can finally get the member values
> from there.
> 
> If there's one surefire way to make code using QDict more awkward,
> it's creating more of them and mixing in QemuOpts for good measure.
> 
> The extraction of keys starting with server.%d into another QDict
> makes us ignore parameters like server.0.neither-host-nor-port
> silently.
> 
> The conversion to QemuOpts abuses runtime_opts, as described a few
> commits ago.
> 
> Rewrite to simply get the values straight from the options QDict.
> 
> Fixes -drive not to crash when server.*.* are present, but
> server.*.host is absent.
> 
> Fixes -drive to reject invalid server.*.*.
> 
> Permits cleaning up runtime_opts.  Do that, and fix -drive to reject
> bogus parameters host and port instead of silently ignoring them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 127 +++++++++++++++---------------------------------------------
>  1 file changed, 32 insertions(+), 95 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 27.03.2017 18:10, Max Reitz wrote:
>> On 27.03.2017 15:26, Markus Armbruster wrote:
>>> qemu_rbd_open() neglects to check pool and image are present.
>>> Reproducer:
>>>
>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>>>     Segmentation fault (core dumped)
>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
>>>
>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
>>> always sets both pool and image.
>>>
>>> Doesn't affect -blockdev, because pool and image are mandatory in the
>>> QAPI schema.
>>>
>>> Fix by adding the missing checks.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  block/rbd.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> That said, don't we have a similar issue with qemu_rbd_create()? It too
> doesn't check whether those options are given but I guess they're just
> as mandatory.

Looks like it.  I'll try to stick a fix into v4.

Thanks!

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 05/11] rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 27.03.2017 15:26, Markus Armbruster wrote:
>> The way we communicate extra key-value pairs from
>> qemu_rbd_parse_filename() to qemu_rbd_open() exposes option parameter
>> "keyvalue-pairs" on the command line.  It's not wanted there.  Hack:
>> rename the parameter to "=keyvalue-pairs" to make it inaccessible.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  block/rbd.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

> (Or you could just use x- like blkdebug does...)

Yes.

An "x-" prefix tells you "don't use this", which you're free to ignore.
The "=" prefix cannot be ignored, because the QemuOpts parser will
choke on it.

Matter of taste, I guess.

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 07/11] rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 27.03.2017 15:26, Markus Armbruster wrote:
>> The conversion from QDict to QemuOpts is pointless.  Simply get the
>> stuff straight from the QDict.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/rbd.c | 20 +++++---------------
>>  1 file changed, 5 insertions(+), 15 deletions(-)
>> 
>> diff --git a/block/rbd.c b/block/rbd.c
>> index b2afe07..cf0bab0 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -376,7 +376,6 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>>      rados_t cluster;
>>      rados_ioctx_t io_ctx;
>>      QDict *options = NULL;
>> -    QemuOpts *rbd_opts = NULL;
>>      int ret = 0;
>>  
>>      secretid = qemu_opt_get(opts, "password-secret");
>> @@ -407,19 +406,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>>          goto exit;
>>      }
>>  
>> -    rbd_opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> -    qemu_opts_absorb_qdict(rbd_opts, options, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        ret = -EINVAL;
>> -        goto exit;
>> -    }
>> -
>> -    pool       = qemu_opt_get(rbd_opts, "pool");
>> -    conf       = qemu_opt_get(rbd_opts, "conf");
>> -    clientname = qemu_opt_get(rbd_opts, "user");
>> -    name       = qemu_opt_get(rbd_opts, "image");
>> -    keypairs   = qemu_opt_get(rbd_opts, "=keyvalue-pairs");
>> +    pool       = qdict_get_str(options, "pool");
>> +    conf       = qdict_get_str(options, "conf");
>> +    clientname = qdict_get_str(options, "user");
>> +    name       = qdict_get_str(options, "image");
>> +    keypairs   = qdict_get_str(options, "=keyvalue-pairs");
>
> This assumes that all of these options are given which they are not
> necessarily:
>
> $ ./qemu-img create -f rbd rbd:blub/ 1M
> Formatting 'rbd:blub/', fmt=rbd size=1048576
> [1]    27387 segmentation fault (core dumped)  ./qemu-img create -f rbd
> rbd:blub/ 1M
>
> qdict_get_try_str() would probably be better.

Yes, will fix.  I've fallen into this trap before...

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2017 08:26 AM, Markus Armbruster wrote:
>> This reverts half of commit 0a55679.  We're having second thoughts on
>> the QAPI schema (and thus the external interface), and haven't reached
>> consensus, yet.  Issues include:
>> 
>
>> Let's avoid painting ourselves into a corner now, and revert the
>> feature for 2.9.
>> 
>> Note that users can still configure authentication methods with a
>> configuration file.  They probably do that anyway if they use Ceph
>> outside QEMU as well.
>
> If we're only reverting the QMP blockdev-add feature, then this makes
> absolute sense (it's not a regression since we don't have a release with
> it yet, and we don't want to bake something into the release that can't
> be supported).  But breaking -drive usage seems risky, especially since
> libvirt is already expecting to work - I'm worried that doing this may
> break existing libvirt command line usage if the QemuOpts side doesn't
> permit anything at all.  Maybe we need to rely on your '=foo' or 'x-foo'
> hack for letting QemuOpts still accept the old spelling during -drive
> but not during QMP.
>
>> 
>> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
>> which is silly.  This will be cleaned up shortly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/rbd.c          | 31 +++----------------------------
>>  qapi/block-core.json | 24 ------------------------
>>  2 files changed, 3 insertions(+), 52 deletions(-)
>> 
>
>> +++ b/qapi/block-core.json
>> @@ -2601,27 +2601,6 @@
>>  
>>  
>>  ##
>> -# @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' } }
>> -
>
> Removing the .json QMP support is fine. But I'm reluctant to give R-b
> without knowing for sure that -drive usage won't regress.

auth-supported landed in master only on March 2nd.

What libvirt usage exactly do you think this could break?

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 10/11] Revert "rbd: add support for getting password from QCryptoSecret object"

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2017 08:26 AM, Markus Armbruster wrote:
>> This reverts commit 60390a2192e7b38aee18db6ce7fb740498709737.
>> 
>> The commit's rationale
>> 
>>     Currently RBD passwords must be provided on the command line
>>     via
>> 
>>       $QEMU -drive file=rbd:pool/image:id=myname:\
>>                    key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
>>                    auth_supported=cephx
>> 
>>     This is insecure because the key is visible in the OS process
>>     listing.
>> 
>> is invalid.  You can easily avoid passing keys on the command line by
>> using "keyfile" instead of "key".  In fact, the Ceph documentation
>> calls use of key "not recommended".  But the most common way to
>> provide keys is a keyring.  The default keyrings should be just fine
>> for most users.  When they aren't, you can configure your own keyrings
>> with "keyring" or override the key with "keyfile".
>> 
>> The commit adds parameter password-secret to -drive.  Support for it
>> was included in -blockdev, but reverted in the previous commit due to
>> concerns about the QMP interface.  Revert it from -drive, too.
>> 
>> Cc: Daniel P. Berrange <berrange@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/rbd.c | 47 -----------------------------------------------
>>  1 file changed, 47 deletions(-)
>
> Are we sure this won't be breaking existing libvirt clients?

I somehow misread the date on commit 60390a2.  It's actually too late to
revert it.  We'll have to live with this.  I'll drop this patch and
rework 11/11.

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Max Reitz <mreitz@redhat.com> writes:
>
>> On 27.03.2017 18:10, Max Reitz wrote:
>>> On 27.03.2017 15:26, Markus Armbruster wrote:
>>>> qemu_rbd_open() neglects to check pool and image are present.
>>>> Reproducer:
>>>>
>>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>>>>     Segmentation fault (core dumped)
>>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
>>>>
>>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
>>>> always sets both pool and image.
>>>>
>>>> Doesn't affect -blockdev, because pool and image are mandatory in the
>>>> QAPI schema.
>>>>
>>>> Fix by adding the missing checks.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>  block/rbd.c | 10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> That said, don't we have a similar issue with qemu_rbd_create()? It too
>> doesn't check whether those options are given but I guess they're just
>> as mandatory.
>
> Looks like it.  I'll try to stick a fix into v4.

Hmm, ignorant question: how can I reach qemu_rbd_create() without going
through qemu_rbd_parse_filename()?

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Eric Blake]

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

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

>>> -
>>> -##
>>> -# @RbdAuthMethod:
>>> -#
>>> -# An enumeration of rados auth_supported types
>>> -#
>>> -# Since: 2.9
>>> -##
>>> -{ 'struct': 'RbdAuthMethod',
>>> -  'data': { 'auth': 'RbdAuthSupport' } }
>>> -
>>
>> Removing the .json QMP support is fine. But I'm reluctant to give R-b
>> without knowing for sure that -drive usage won't regress.
> 
> auth-supported landed in master only on March 2nd.

auth-supported via -blockdev-add only landed on March 2nd.  But
auth-supported via -drive landed in commit 60390a2, Jan 2016; and is in
use by libvirt:

src/qemu/qemu_command.c:
":key=%s:auth_supported=cephx\\;none",

> 
> What libvirt usage exactly do you think this could break?
> 

Libvirt has been managing rbd drives using -drive since at least libvirt
commit 5745cd1, in Nov 2011, where even back then it was passing:
            virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",

(back when it used space instead of ; to separate the list of supported
auth types).

As I've never personally used RBD (whether through qemu directly, or
through libvirt), I'm extremely wary of breaking -drive usage that
"works" (for some definition of "works"), even though I have no qualms
making the QMP interface extremely limited.

-- 
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 RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2017 01:31 PM, Markus Armbruster wrote:
>
>>>> -
>>>> -##
>>>> -# @RbdAuthMethod:
>>>> -#
>>>> -# An enumeration of rados auth_supported types
>>>> -#
>>>> -# Since: 2.9
>>>> -##
>>>> -{ 'struct': 'RbdAuthMethod',
>>>> -  'data': { 'auth': 'RbdAuthSupport' } }
>>>> -
>>>
>>> Removing the .json QMP support is fine. But I'm reluctant to give R-b
>>> without knowing for sure that -drive usage won't regress.
>> 
>> auth-supported landed in master only on March 2nd.
>
> auth-supported via -blockdev-add only landed on March 2nd.  But
> auth-supported via -drive landed in commit 60390a2, Jan 2016; and is in
> use by libvirt:
>
> src/qemu/qemu_command.c:
> ":key=%s:auth_supported=cephx\\;none",

That's a key-value part of the pseudo-filename.  *Not* reverted by this
patch.  Only QemuOpts parameter auth_supported is.

QemuOpts parameter: -drive driver=rbd,auth_supported.0.auth=none,...
Pseudo-filename:    -drive file=rbd:...:auth_supported=none

>> What libvirt usage exactly do you think this could break?
>> 
>
> Libvirt has been managing rbd drives using -drive since at least libvirt
> commit 5745cd1, in Nov 2011, where even back then it was passing:
>             virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
>
> (back when it used space instead of ; to separate the list of supported
> auth types).
>
> As I've never personally used RBD (whether through qemu directly, or
> through libvirt), I'm extremely wary of breaking -drive usage that
> "works" (for some definition of "works"), even though I have no qualms
> making the QMP interface extremely limited.

All clear now?

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Eric Blake]

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

On 03/27/2017 02:14 PM, Markus Armbruster wrote:

>>>>
>>>> Removing the .json QMP support is fine. But I'm reluctant to give R-b
>>>> without knowing for sure that -drive usage won't regress.
>>>
>>> auth-supported landed in master only on March 2nd.
>>
>> auth-supported via -blockdev-add only landed on March 2nd.  But
>> auth-supported via -drive landed in commit 60390a2, Jan 2016; and is in
>> use by libvirt:
>>
>> src/qemu/qemu_command.c:
>> ":key=%s:auth_supported=cephx\\;none",
> 
> That's a key-value part of the pseudo-filename.  *Not* reverted by this
> patch.  Only QemuOpts parameter auth_supported is.
> 
> QemuOpts parameter: -drive driver=rbd,auth_supported.0.auth=none,...
> Pseudo-filename:    -drive file=rbd:...:auth_supported=none
> 
>>> What libvirt usage exactly do you think this could break?
>>>
>>
>> Libvirt has been managing rbd drives using -drive since at least libvirt
>> commit 5745cd1, in Nov 2011, where even back then it was passing:
>>             virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
>>
>> (back when it used space instead of ; to separate the list of supported
>> auth types).

Ah, so as long as libvirt uses 'drive file=rbd:...' with key-value
pairs, our backdoor of =key-values will let it continue to work.  And
looking more at libvirt, it definitely looks like it is sticking to
file=rbd: pseudo-filenames for now; for example:

tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args:-drive
'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\


I'm not sure if (or where) a big fat comment to that effect would be
beneficial, but it certainly goes a long ways in explaining the goals of
your series.

>>
>> As I've never personally used RBD (whether through qemu directly, or
>> through libvirt), I'm extremely wary of breaking -drive usage that
>> "works" (for some definition of "works"), even though I have no qualms
>> making the QMP interface extremely limited.
> 
> All clear now?

I think so.  Commit 60390a2 (Jan 2016) mentions the use of
auth_supported in the commit message (matching libvirt usage), but under
the pseudo-file format; the actual rbd.c file did not special case it
differently from any other key-value pair.  "auth" wasn't added to
QemuOpts (for the -drive driver=rbd, form) until commit 0a55679, which
is unreleased, so libvirt can't have been relying on it, and ripping it
out now is safe enough.

Under that light, I'm now going to re-read your patch...

-- 
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 RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Eric Blake]

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

On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> This reverts half of commit 0a55679.  We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet.  Issues include:
> 

> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.

There may still be some tweaks to improve the commit message and/or code
comments to clarify things that tripped me up until later in the
subthread, but now that I understand the difference between pseudo-file
format (where the key-value pair backdoor still works for libvirt's
usage of -drive file=rbd:...) and QemuOpts format (-drive driver=rbd,...
which didn't really exist in 2.8, and where we don't want to bake in
something we don't like in 2.9), I agree with the move.

> 
> Note that users can still configure authentication methods with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly.  This will be cleaned up shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          | 31 +++----------------------------
>  qapi/block-core.json | 24 ------------------------
>  2 files changed, 3 insertions(+), 52 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 01/11] rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:25PM +0200, Markus Armbruster wrote:
> We use InetSocketAddress in the QAPI schema.  However, the code
> doesn't use inet_connect_saddr(), but formats "host" and "port" into a
> configuration string for rados_conf_set().  Thus, members "numeric",
> "to", "ipv4" and "ipv6" are silently ignored.  Not nice.  Example:
> 
>     -blockdev rbd,node-name=nn,pool=p,image=i,server.0.host=h0,server.0.port=12345,server.0.ipv4=off
> 
> Factor a suitable InetSocketAddressBase out of InetSocketAddress, and
> use that.  "numeric", "to", "ipv4" and "ipv6" are now rejected.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Jeff Cody]

On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Max Reitz <mreitz@redhat.com> writes:
> >
> >> On 27.03.2017 18:10, Max Reitz wrote:
> >>> On 27.03.2017 15:26, Markus Armbruster wrote:
> >>>> qemu_rbd_open() neglects to check pool and image are present.
> >>>> Reproducer:
> >>>>
> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
> >>>>     Segmentation fault (core dumped)
> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
> >>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
> >>>>
> >>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> >>>> always sets both pool and image.
> >>>>
> >>>> Doesn't affect -blockdev, because pool and image are mandatory in the
> >>>> QAPI schema.
> >>>>
> >>>> Fix by adding the missing checks.
> >>>>
> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>> ---
> >>>>  block/rbd.c | 10 +++++++---
> >>>>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>> 
> >>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>
> >> That said, don't we have a similar issue with qemu_rbd_create()? It too
> >> doesn't check whether those options are given but I guess they're just
> >> as mandatory.
> >
> > Looks like it.  I'll try to stick a fix into v4.
> 
> Hmm, ignorant question: how can I reach qemu_rbd_create() without going
> through qemu_rbd_parse_filename()?

You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call
qemu_rbd_parse_filename().  And in qemu_rbd_parse_filename(), it will
complain if pool is not provided (and that is what causes the abort, not the
missing image parameter).  So I think we are safe, but a nicer error message
for a missing 'image' parameter might be nice anyway.

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:26PM +0200, Markus Armbruster wrote:
> qemu_rbd_open() neglects to check pool and image are present.
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>     Segmentation fault (core dumped)

This reproducer is wrong, I think.  Omitting the image should be caught
earlier, but it is an error caught by the rbd_open() call.

What doesn't work is omitting the pool name, and that causes an abort()
from rados_ioctx_create(), e.g.:


$ qemu-system-x86_64 -nodefaults -drive driver=rbd,id=rbd,image=i,server.0.port=6789,server.0.host=192.168.15.180
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_M_construct null not valid
Aborted (core dumped)


>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
> 
> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> always sets both pool and image.
> 
> Doesn't affect -blockdev, because pool and image are mandatory in the
> QAPI schema.
> 
> Fix by adding the missing checks.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

With an updated commit message:

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/rbd.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index ee13f3d..5ba2a87 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -711,6 +711,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      name           = qemu_opt_get(opts, "image");
>      keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
>  
> +    if (!pool || !name) {
> +        error_setg(errp, "Parameters 'pool' and 'image' are required");
> +        r = -EINVAL;
> +        goto failed_opts;
> +    }
> +
>      r = rados_create(&s->cluster, clientname);
>      if (r < 0) {
>          error_setg_errno(errp, -r, "error initializing");
> @@ -718,9 +724,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      s->snap = g_strdup(snap);
> -    if (name) {
> -        pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
> -    }
> +    pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
>  
>      /* try default location when conf=NULL, but ignore failure */
>      r = rados_conf_read_file(s->cluster, conf);
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 03/11] rbd: Don't limit length of parameter values

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:27PM +0200, Markus Armbruster wrote:
> We laboriously enforce parameter values are between one and some

s/are/that are/

or maybe just s/are//

> arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
> librbd.h, and I'm not sure it applies.  Where the other limits come
> from is unclear.
> 
> Drop the length checking.  The limits librbd actually imposes must be
> checked by librbd anyway.
> 
> There's one minor complication: BDRVRBDState member name is a
> fixed-size array.  Depends on the length limit.  Make it a pointer to
> a dynamically allocated string.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>


> ---
>  block/rbd.c | 91 ++++++++++---------------------------------------------------
>  1 file changed, 14 insertions(+), 77 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 5ba2a87..0fea348 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -56,11 +56,6 @@
>  
>  #define OBJ_MAX_SIZE (1UL << OBJ_DEFAULT_OBJ_ORDER)
>  
> -#define RBD_MAX_CONF_NAME_SIZE 128
> -#define RBD_MAX_CONF_VAL_SIZE 512
> -#define RBD_MAX_CONF_SIZE 1024
> -#define RBD_MAX_POOL_NAME_SIZE 128
> -#define RBD_MAX_SNAP_NAME_SIZE 128
>  #define RBD_MAX_SNAPS 100
>  
>  /* The LIBRBD_SUPPORTS_IOVEC is defined in librbd.h */
> @@ -99,16 +94,12 @@ typedef struct BDRVRBDState {
>      rados_t cluster;
>      rados_ioctx_t io_ctx;
>      rbd_image_t image;
> -    char name[RBD_MAX_IMAGE_NAME_SIZE];
> +    char *name;
>      char *snap;
>  } BDRVRBDState;
>  
> -static char *qemu_rbd_next_tok(int max_len,
> -                               char *src, char delim,
> -                               const char *name,
> -                               char **p, Error **errp)
> +static char *qemu_rbd_next_tok(char *src, char delim, char **p)
>  {
> -    int l;
>      char *end;
>  
>      *p = NULL;
> @@ -127,15 +118,6 @@ static char *qemu_rbd_next_tok(int max_len,
>              *end = '\0';
>          }
>      }
> -    l = strlen(src);
> -    if (l >= max_len) {
> -        error_setg(errp, "%s too long", name);
> -        return NULL;
> -    } else if (l == 0) {
> -        error_setg(errp, "%s too short", name);
> -        return NULL;
> -    }
> -
>      return src;
>  }
>  
> @@ -159,7 +141,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      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:'");
> @@ -171,11 +152,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      keypairs = g_malloc0(max_keypair_size);
>      p = buf;
>  
> -    found_str = qemu_rbd_next_tok(RBD_MAX_POOL_NAME_SIZE, p,
> -                                  '/', "pool name", &p, &local_err);
> -    if (local_err) {
> -        goto done;
> -    }
> +    found_str = qemu_rbd_next_tok(p, '/', &p);
>      if (!p) {
>          error_setg(errp, "Pool name is required");
>          goto done;
> @@ -184,27 +161,15 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      qdict_put(options, "pool", qstring_from_str(found_str));
>  
>      if (strchr(p, '@')) {
> -        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
> -                                      '@', "object name", &p, &local_err);
> -        if (local_err) {
> -            goto done;
> -        }
> +        found_str = qemu_rbd_next_tok(p, '@', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put(options, "image", qstring_from_str(found_str));
>  
> -        found_str = qemu_rbd_next_tok(RBD_MAX_SNAP_NAME_SIZE, p,
> -                                      ':', "snap name", &p, &local_err);
> -        if (local_err) {
> -            goto done;
> -        }
> +        found_str = qemu_rbd_next_tok(p, ':', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put(options, "snapshot", qstring_from_str(found_str));
>      } else {
> -        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
> -                                      ':', "object name", &p, &local_err);
> -        if (local_err) {
> -            goto done;
> -        }
> +        found_str = qemu_rbd_next_tok(p, ':', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put(options, "image", qstring_from_str(found_str));
>      }
> @@ -212,11 +177,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>          goto done;
>      }
>  
> -    found_str = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
> -                                  '\0', "configuration", &p, &local_err);
> -    if (local_err) {
> -        goto done;
> -    }
> +    found_str = qemu_rbd_next_tok(p, '\0', &p);
>  
>      p = found_str;
>  
> @@ -224,12 +185,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>       * '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;
> -        }
> -
> +        name = qemu_rbd_next_tok(p, '=', &p);
>          if (!p) {
>              error_setg(errp, "conf option %s has no value", name);
>              break;
> @@ -237,11 +193,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>  
>          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;
> -        }
> +        value = qemu_rbd_next_tok(p, ':', &p);
>          qemu_rbd_unescape(value);
>  
>          if (!strcmp(name, "conf")) {
> @@ -274,9 +226,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>  
>  
>  done:
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
>      g_free(buf);
>      g_free(keypairs);
>      return;
> @@ -308,30 +257,20 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
>      char *p, *buf;
>      char *name;
>      char *value;
> -    Error *local_err = NULL;
>      int ret = 0;
>  
>      buf = g_strdup(keypairs);
>      p = buf;
>  
>      while (p) {
> -        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
> -                                 '=', "conf option name", &p, &local_err);
> -        if (local_err) {
> -            break;
> -        }
> -
> +        name = qemu_rbd_next_tok(p, '=', &p);
>          if (!p) {
>              error_setg(errp, "conf option %s has no value", name);
>              ret = -EINVAL;
>              break;
>          }
>  
> -        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
> -                                  ':', "conf option value", &p, &local_err);
> -        if (local_err) {
> -            break;
> -        }
> +        value = qemu_rbd_next_tok(p, ':', &p);
>  
>          ret = rados_conf_set(cluster, name, value);
>          if (ret < 0) {
> @@ -341,10 +280,6 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs,
>          }
>      }
>  
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        ret = -EINVAL;
> -    }
>      g_free(buf);
>      return ret;
>  }
> @@ -724,7 +659,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      s->snap = g_strdup(snap);
> -    pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
> +    s->name = g_strdup(name);
>  
>      /* try default location when conf=NULL, but ignore failure */
>      r = rados_conf_read_file(s->cluster, conf);
> @@ -798,6 +733,7 @@ failed_open:
>  failed_shutdown:
>      rados_shutdown(s->cluster);
>      g_free(s->snap);
> +    g_free(s->name);
>  failed_opts:
>      qemu_opts_del(opts);
>      g_free(mon_host);
> @@ -812,6 +748,7 @@ static void qemu_rbd_close(BlockDriverState *bs)
>      rbd_close(s->image);
>      rados_ioctx_destroy(s->io_ctx);
>      g_free(s->snap);
> +    g_free(s->name);
>      rados_shutdown(s->cluster);
>  }
>  
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 04/11] rbd: Clean up after the previous commit

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:28PM +0200, Markus Armbruster wrote:
> This code in qemu_rbd_parse_filename()
> 
>     found_str = qemu_rbd_next_tok(p, '\0', &p);
>     p = found_str;
> 
> has no effect.  Drop it, and simplify qemu_rbd_next_tok().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/rbd.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 0fea348..182a5a3 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,19 +104,17 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
>  
>      *p = NULL;
>  
> -    if (delim != '\0') {
> -        for (end = src; *end; ++end) {
> -            if (*end == delim) {
> -                break;
> -            }
> -            if (*end == '\\' && end[1] != '\0') {
> -                end++;
> -            }
> -        }
> +    for (end = src; *end; ++end) {
>          if (*end == delim) {
> -            *p = end + 1;
> -            *end = '\0';
> +            break;
>          }
> +        if (*end == '\\' && end[1] != '\0') {
> +            end++;
> +        }
> +    }
> +    if (*end == delim) {
> +        *p = end + 1;
> +        *end = '\0';
>      }
>      return src;
>  }
> @@ -177,10 +175,6 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>          goto done;
>      }
>  
> -    found_str = qemu_rbd_next_tok(p, '\0', &p);
> -
> -    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) {
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 05/11] rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:29PM +0200, Markus Armbruster wrote:
> The way we communicate extra key-value pairs from
> qemu_rbd_parse_filename() to qemu_rbd_open() exposes option parameter
> "keyvalue-pairs" on the command line.  It's not wanted there.  Hack:
> rename the parameter to "=keyvalue-pairs" to make it inaccessible.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/rbd.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 182a5a3..2632533 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -215,7 +215,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      }
>  
>      if (keypairs[0]) {
> -        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));
> +        qdict_put(options, "=keyvalue-pairs", qstring_from_str(keypairs));
>      }
>  
>  
> @@ -330,7 +330,11 @@ static QemuOptsList runtime_opts = {
>              .help = "Rados id name",
>          },
>          {
> -            .name = "keyvalue-pairs",
> +            /*
> +             * HACK: name starts with '=' so that qemu_opts_parse()
> +             * can't set it
> +             */
> +            .name = "=keyvalue-pairs",
>              .type = QEMU_OPT_STRING,
>              .help = "Legacy rados key/value option parameters",
>          },
> @@ -405,7 +409,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>      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");
> +    keypairs   = qemu_opt_get(rbd_opts, "=keyvalue-pairs");
>  
>      ret = rados_create(&cluster, clientname);
>      if (ret < 0) {
> @@ -638,7 +642,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      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");
> +    keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
>  
>      if (!pool || !name) {
>          error_setg(errp, "Parameters 'pool' and 'image' are required");
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 06/11] rbd: Clean up runtime_opts, fix -drive to reject filename

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:30PM +0200, Markus Armbruster wrote:
> runtime_opts is used for three different purposes:
> 
> * qemu_rbd_open() uses it to accept options it recognizes, such as
>   "pool" and "image".  Other .bdrv_open() methods do it similarly.
> 
> * qemu_rbd_open() accepts additional list-valued options
>   auth-supported and server, with the help of qemu_rbd_array_opts().
>   The list elements are again dictionaries.  qemu_rbd_array_opts()
>   uses runtime_opts to accept their members.  Thus, runtime_opts
>   contains recognized sub-sub-options "auth", "host", "port" in
>   addition to recognized options.  No other block driver does that.
> 
> * qemu_rbd_create() uses it to convert the QDict produced by
>   qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
>   that.  The keys produced by qemu_rbd_parse_filename() are "pool",
>   "image", "snapshot", "conf", "user" and "keyvalue-pairs".
>   qemu_rbd_open() accepts these, so no additional ones here.
> 
> This is a confusing mess.  Dates back to commit 0f9d252.  First step
> to clean it up is documenting runtime_opts.desc[]:
> 
> * Reorder entries to match the QAPI schema, like we do in other block
>   drivers.
> 
> * Document why the schema's "server" and "auth-supported" aren't in
>   .desc[].
> 
> * Document why "keyvalue-pairs", "host", "port" and "auth" are in
>   .desc[], but not the schema.
> 
> * Delete "filename", because none of the three users actually uses it.
>   This fixes -drive to reject parameter filename instead of silently
>   ignoring it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/rbd.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 2632533..b2afe07 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -294,21 +294,6 @@ static QemuOptsList runtime_opts = {
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
>          {
> -            .name = "filename",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Specification of the rbd image",
> -        },
> -        {
> -            .name = "password-secret",
> -            .type = QEMU_OPT_STRING,
> -            .help = "ID of secret providing the password",
> -        },
> -        {
> -            .name = "conf",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Rados config file location",
> -        },
> -        {
>              .name = "pool",
>              .type = QEMU_OPT_STRING,
>              .help = "Rados pool name",
> @@ -319,6 +304,11 @@ static QemuOptsList runtime_opts = {
>              .help = "Image name in the pool",
>          },
>          {
> +            .name = "conf",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Rados config file location",
> +        },
> +        {
>              .name = "snapshot",
>              .type = QEMU_OPT_STRING,
>              .help = "Ceph snapshot name",
> @@ -329,6 +319,19 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Rados id name",
>          },
> +        /*
> +         * server.* and auth-supported.* extracted manually, see
> +         * qemu_rbd_array_opts()
> +         */
> +        {
> +            .name = "password-secret",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
> +
> +        /*
> +         * Keys for qemu_rbd_parse_filename(), not in the QAPI schema
> +         */
>          {
>              /*
>               * HACK: name starts with '=' so that qemu_opts_parse()
> @@ -338,6 +341,13 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Legacy rados key/value option parameters",
>          },
> +
> +        /*
> +         * The remainder aren't option keys, but option sub-sub-keys,
> +         * so that qemu_rbd_array_opts() can abuse runtime_opts for
> +         * its own purposes
> +         * TODO clean this up
> +         */
>          {
>              .name = "host",
>              .type = QEMU_OPT_STRING,
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:32PM +0200, Markus Armbruster wrote:
> This reverts half of commit 0a55679.  We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet.  Issues include:
> 
> * The implementation uses deprecated rados_conf_set() key
>   "auth_supported".  No biggie.
> 
> * The implementation makes -drive silently ignore invalid parameters
>   "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
>   fact I'm going to fix similar bugs around parameter server), so
>   again no biggie.
> 
> * BlockdevOptionsRbd member @password-secret applies only to
>   authentication method cephx.  Should it be a variant member of
>   RbdAuthMethod?
> 
> * BlockdevOptionsRbd member @user could apply to both methods cephx
>   and none, but I'm not sure it's actually used with none.  If it
>   isn't, should it be a variant member of RbdAuthMethod?
> 
> * The client offers a *set* of authentication methods, not a list.
>   Should the methods be optional members of BlockdevOptionsRbd instead
>   of members of list @auth-supported?  The latter begs the question
>   what multiple entries for the same method mean.  Trivial question
>   now that RbdAuthMethod contains nothing but @type, but less so when
>   RbdAuthMethod acquires other members, such the ones discussed above.
> 
> * How BlockdevOptionsRbd member @auth-supported interacts with
>   settings from a configuration file specified with @conf is
>   undocumented.  I suspect it's untested, too.
> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure authentication methods with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly.  This will be cleaned up shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


I think this move makes sense; it allows blockdev-add to still be supported
for rbd, but does not lock us into a perhaps unwieldy API.

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/rbd.c          | 31 +++----------------------------
>  qapi/block-core.json | 24 ------------------------
>  2 files changed, 3 insertions(+), 52 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index cf0bab0..103ce44 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -320,8 +320,7 @@ static QemuOptsList runtime_opts = {
>              .help = "Rados id name",
>          },
>          /*
> -         * server.* and auth-supported.* extracted manually, see
> -         * qemu_rbd_array_opts()
> +         * server.* extracted manually, see qemu_rbd_array_opts()
>           */
>          {
>              .name = "password-secret",
> @@ -356,11 +355,6 @@ static QemuOptsList runtime_opts = {
>              .name = "port",
>              .type = QEMU_OPT_STRING,
>          },
> -        {
> -            .name = "auth",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Supported authentication method, either cephx or none",
> -        },
>          { /* end of list */ }
>      },
>  };
> @@ -512,7 +506,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>  }
>  
>  #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)
> @@ -527,7 +520,7 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
>      Error *local_err = NULL;
>      int i;
>  
> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> +    assert(type == RBD_MON_HOST);
>  
>      num_entries = qdict_array_entries(options, prefix);
>  
> @@ -573,10 +566,9 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
>                  value = strbuf;
>              }
>          } else {
> -            value = qemu_opt_get(opts, "auth");
> +            abort();
>          }
>  
> -
>          /* 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) {
> @@ -608,7 +600,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      QemuOpts *opts;
>      Error *local_err = NULL;
>      char *mon_host = NULL;
> -    char *auth_supported = NULL;
>      int r;
>  
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> @@ -619,14 +610,6 @@ 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) {
> @@ -678,13 +661,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          }
>      }
>  
> -    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;
> @@ -735,7 +711,6 @@ failed_shutdown:
>  failed_opts:
>      qemu_opts_del(opts);
>      g_free(mon_host);
> -    g_free(auth_supported);
>      return r;
>  }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5d2efe4..6a7ca0b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2601,27 +2601,6 @@
>  
>  
>  ##
> -# @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:
>  #
>  # @pool:               Ceph pool name.
> @@ -2639,8 +2618,6 @@
>  # @server:             Monitor host address and port.  This maps
>  #                      to the "mon_host" Ceph option.
>  #
> -# @auth-supported:     Authentication supported.
> -#
>  # @password-secret:    The ID of a QCryptoSecret object providing
>  #                      the password for the login.
>  #
> @@ -2653,7 +2630,6 @@
>              '*snapshot': 'str',
>              '*user': 'str',
>              '*server': ['InetSocketAddressBase'],
> -            '*auth-supported': ['RbdAuthMethod'],
>              '*password-secret': 'str' } }
>  
>  ##
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
> This reverts a part of commit 8a47e8e.  We're having second thoughts
> on the QAPI schema (and thus the external interface), and haven't
> reached consensus, yet.  Issues include:
> 
> * BlockdevOptionsRbd member @password-secret isn't actually a
>   password, it's a key generated by Ceph.
> 
> * We're not sure where member @password-secret belongs (see the
>   previous commit).
> 
> * How @password-secret interacts with settings from a configuration
>   file specified with @conf is undocumented.  I suspect it's untested,
>   too.

It's tested, at least to some extent -- it works on my setup; it will
override whatever is set in the conf or the keyring.  I can alternate
between /etc/ceph/keyring, or password-secret, or override the user present
in /etc/cep/keyring with a different key via password-secret.

So I'd suggest dropping that blurb from the commit message.

I agree with the overall reasoning though behind remove all parameters not
needed for now, so:

Reviewed-by: Jeff Cody <jcody@redhat.com>

> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure an authentication key with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6a7ca0b..2e60ab5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2618,9 +2618,6 @@
>  # @server:             Monitor host address and port.  This maps
>  #                      to the "mon_host" Ceph option.
>  #
> -# @password-secret:    The ID of a QCryptoSecret object providing
> -#                      the password for the login.
> -#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsRbd',
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Jeff Cody]

On Mon, Mar 27, 2017 at 10:32:40PM -0400, Jeff Cody wrote:
> On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
> > This reverts a part of commit 8a47e8e.  We're having second thoughts
> > on the QAPI schema (and thus the external interface), and haven't
> > reached consensus, yet.  Issues include:
> > 
> > * BlockdevOptionsRbd member @password-secret isn't actually a
> >   password, it's a key generated by Ceph.
> > 
> > * We're not sure where member @password-secret belongs (see the
> >   previous commit).
> > 
> > * How @password-secret interacts with settings from a configuration
> >   file specified with @conf is undocumented.  I suspect it's untested,
> >   too.
> 
> It's tested, at least to some extent -- it works on my setup; it will
> override whatever is set in the conf or the keyring.  I can alternate
> between /etc/ceph/keyring, or password-secret, or override the user present
> in /etc/cep/keyring with a different key via password-secret.
> 
> So I'd suggest dropping that blurb from the commit message.
> 
> I agree with the overall reasoning though behind remove all parameters not
> needed for now, so:
> 
> Reviewed-by: Jeff Cody <jcody@redhat.com>
>

Wait, hold up on that R-b :)

This only seems to revert the comment in the QAPI.  You meant to remove the
entry from the BlockdevOptionsRbd struct too, right?


> > 
> > Let's avoid painting ourselves into a corner now, and revert the
> > feature for 2.9.
> > 
> > Note that users can still configure an authentication key with a
> > configuration file.  They probably do that anyway if they use Ceph
> > outside QEMU as well.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  qapi/block-core.json | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 6a7ca0b..2e60ab5 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2618,9 +2618,6 @@
> >  # @server:             Monitor host address and port.  This maps
> >  #                      to the "mon_host" Ceph option.
> >  #
> > -# @password-secret:    The ID of a QCryptoSecret object providing
> > -#                      the password for the login.
> > -#
> >  # Since: 2.9
> >  ##
> >  { 'struct': 'BlockdevOptionsRbd',
> > -- 
> > 2.7.4
> > 

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Mar 27, 2017 at 03:26:26PM +0200, Markus Armbruster wrote:
>> qemu_rbd_open() neglects to check pool and image are present.
>> Reproducer:
>> 
>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>>     Segmentation fault (core dumped)
>
> This reproducer is wrong, I think.  Omitting the image should be caught
> earlier, but it is an error caught by the rbd_open() call.

Turns out the crash I observed was an artifact of my testing
instrumentation.

> What doesn't work is omitting the pool name, and that causes an abort()
> from rados_ioctx_create(), e.g.:
>
>
> $ qemu-system-x86_64 -nodefaults -drive driver=rbd,id=rbd,image=i,server.0.port=6789,server.0.host=192.168.15.180
> terminate called after throwing an instance of 'std::logic_error'
>   what():  basic_string::_M_construct null not valid
> Aborted (core dumped)
>
>
>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
>> 
>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
>> always sets both pool and image.
>> 
>> Doesn't affect -blockdev, because pool and image are mandatory in the
>> QAPI schema.
>> 
>> Fix by adding the missing checks.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> With an updated commit message:
>
> Reviewed-by: Jeff Cody <jcody@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Max Reitz <mreitz@redhat.com> writes:
>> >
>> >> On 27.03.2017 18:10, Max Reitz wrote:
>> >>> On 27.03.2017 15:26, Markus Armbruster wrote:
>> >>>> qemu_rbd_open() neglects to check pool and image are present.
>> >>>> Reproducer:
>> >>>>
>> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>> >>>>     Segmentation fault (core dumped)
>> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>> >>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
>> >>>>
>> >>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
>> >>>> always sets both pool and image.
>> >>>>
>> >>>> Doesn't affect -blockdev, because pool and image are mandatory in the
>> >>>> QAPI schema.
>> >>>>
>> >>>> Fix by adding the missing checks.
>> >>>>
>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> >>>> ---
>> >>>>  block/rbd.c | 10 +++++++---
>> >>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> >>> 
>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> >>
>> >> That said, don't we have a similar issue with qemu_rbd_create()? It too
>> >> doesn't check whether those options are given but I guess they're just
>> >> as mandatory.
>> >
>> > Looks like it.  I'll try to stick a fix into v4.
>> 
>> Hmm, ignorant question: how can I reach qemu_rbd_create() without going
>> through qemu_rbd_parse_filename()?
>
> You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call
> qemu_rbd_parse_filename().  And in qemu_rbd_parse_filename(), it will
> complain if pool is not provided (and that is what causes the abort, not the
> missing image parameter).  So I think we are safe, but a nicer error message
> for a missing 'image' parameter might be nice anyway.

I now see the "we are safe" part, but not the "want a nicer error
message for a missing 'image'" part.  How can 'image' be missing?

qemu_rbd_parse_filename() parses a pseudo-filename of the form

    rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...]

It fails if

* the pseudo-filename doesn't start with "rbd:"

* doesn't contain '/' ("Pool name is required")

* POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or
  too long (until the next commit)

* a KEY=VALUE doesn't contain '='

If it succeeds, "pool" and "image" are both set in @options.

Can you give me a reproducer for the error message you'd like me to
improve?

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Mar 27, 2017 at 10:32:40PM -0400, Jeff Cody wrote:
>> On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
>> > This reverts a part of commit 8a47e8e.  We're having second thoughts
>> > on the QAPI schema (and thus the external interface), and haven't
>> > reached consensus, yet.  Issues include:
>> > 
>> > * BlockdevOptionsRbd member @password-secret isn't actually a
>> >   password, it's a key generated by Ceph.
>> > 
>> > * We're not sure where member @password-secret belongs (see the
>> >   previous commit).
>> > 
>> > * How @password-secret interacts with settings from a configuration
>> >   file specified with @conf is undocumented.  I suspect it's untested,
>> >   too.
>> 
>> It's tested, at least to some extent -- it works on my setup; it will
>> override whatever is set in the conf or the keyring.  I can alternate
>> between /etc/ceph/keyring, or password-secret, or override the user present
>> in /etc/cep/keyring with a different key via password-secret.
>> 
>> So I'd suggest dropping that blurb from the commit message.

Of course.

>> I agree with the overall reasoning though behind remove all parameters not
>> needed for now, so:
>> 
>> Reviewed-by: Jeff Cody <jcody@redhat.com>
>>
>
> Wait, hold up on that R-b :)
>
> This only seems to revert the comment in the QAPI.  You meant to remove the
> entry from the BlockdevOptionsRbd struct too, right?

D'oh!  Suspect a rebase accident...  I'll drop the member and keep your
R-by.

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 03/11] rbd: Don't limit length of parameter values

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Mar 27, 2017 at 03:26:27PM +0200, Markus Armbruster wrote:
>> We laboriously enforce parameter values are between one and some
>
> s/are/that are/
>
> or maybe just s/are//

What about:

   We laboriously enforce that parameter values are between one and some

>> arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
>> librbd.h, and I'm not sure it applies.  Where the other limits come
>> from is unclear.
>> 
>> Drop the length checking.  The limits librbd actually imposes must be
>> checked by librbd anyway.
>> 
>> There's one minor complication: BDRVRBDState member name is a
>> fixed-size array.  Depends on the length limit.  Make it a pointer to
>> a dynamically allocated string.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Reviewed-by: Jeff Cody <jcody@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2017 08:26 AM, Markus Armbruster wrote:
>> This reverts half of commit 0a55679.  We're having second thoughts on
>> the QAPI schema (and thus the external interface), and haven't reached
>> consensus, yet.  Issues include:
>> 
>
>> Let's avoid painting ourselves into a corner now, and revert the
>> feature for 2.9.
>
> There may still be some tweaks to improve the commit message and/or code
> comments to clarify things that tripped me up until later in the
> subthread, but now that I understand the difference between pseudo-file
> format (where the key-value pair backdoor still works for libvirt's
> usage of -drive file=rbd:...) and QemuOpts format (-drive driver=rbd,...
> which didn't really exist in 2.8, and where we don't want to bake in
> something we don't like in 2.9), I agree with the move.

I'm inserting ...

>> Note that users can still configure authentication methods with a
>> configuration file.  They probably do that anyway if they use Ceph
>> outside QEMU as well.

... this hint right here:

   Further note that this doesn't affect use of key "auth-supported" in
   -drive file=rbd:...:key=value.

Good enough?

>> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
>> which is silly.  This will be cleaned up shortly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/rbd.c          | 31 +++----------------------------
>>  qapi/block-core.json | 24 ------------------------
>>  2 files changed, 3 insertions(+), 52 deletions(-)
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Jeff Cody]

On Tue, Mar 28, 2017 at 09:54:53AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Max Reitz <mreitz@redhat.com> writes:
> >> >
> >> >> On 27.03.2017 18:10, Max Reitz wrote:
> >> >>> On 27.03.2017 15:26, Markus Armbruster wrote:
> >> >>>> qemu_rbd_open() neglects to check pool and image are present.
> >> >>>> Reproducer:
> >> >>>>
> >> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
> >> >>>>     Segmentation fault (core dumped)
> >> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
> >> >>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
> >> >>>>
> >> >>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> >> >>>> always sets both pool and image.
> >> >>>>
> >> >>>> Doesn't affect -blockdev, because pool and image are mandatory in the
> >> >>>> QAPI schema.
> >> >>>>
> >> >>>> Fix by adding the missing checks.
> >> >>>>
> >> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> >>>> ---
> >> >>>>  block/rbd.c | 10 +++++++---
> >> >>>>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> >>> 
> >> >>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >> >>
> >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too
> >> >> doesn't check whether those options are given but I guess they're just
> >> >> as mandatory.
> >> >
> >> > Looks like it.  I'll try to stick a fix into v4.
> >> 
> >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going
> >> through qemu_rbd_parse_filename()?
> >
> > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call
> > qemu_rbd_parse_filename().  And in qemu_rbd_parse_filename(), it will
> > complain if pool is not provided (and that is what causes the abort, not the
> > missing image parameter).  So I think we are safe, but a nicer error message
> > for a missing 'image' parameter might be nice anyway.
> 
> I now see the "we are safe" part, but not the "want a nicer error
> message for a missing 'image'" part.  How can 'image' be missing?
> 
> qemu_rbd_parse_filename() parses a pseudo-filename of the form
> 
>     rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...]
> 
> It fails if
> 
> * the pseudo-filename doesn't start with "rbd:"
> 
> * doesn't contain '/' ("Pool name is required")
> 
> * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or
>   too long (until the next commit)
> 
> * a KEY=VALUE doesn't contain '='
> 
> If it succeeds, "pool" and "image" are both set in @options.
> 
> Can you give me a reproducer for the error message you'd like me to
> improve?


Sure: qemu-img info rbd:mypool/:mon_host=192.168.15.180

'image' is technically present in the options, but it is an empty string.

My thought was that it'd be nice to throw a similar error message for an
empty string for 'image'.  As it is, the rbd library catches it, so it isn't
catastrophic, but a nicer message would be helpful.

(My comment here assumes an empty string is not an acceptable image name for
rbd)

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Jeff Cody]

On Tue, Mar 28, 2017 at 07:56:06AM -0400, Jeff Cody wrote:
> On Tue, Mar 28, 2017 at 09:54:53AM +0200, Markus Armbruster wrote:
> > Jeff Cody <jcody@redhat.com> writes:
> > 
> > > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote:
> > >> Markus Armbruster <armbru@redhat.com> writes:
> > >> 
> > >> > Max Reitz <mreitz@redhat.com> writes:
> > >> >
> > >> >> On 27.03.2017 18:10, Max Reitz wrote:
> > >> >>> On 27.03.2017 15:26, Markus Armbruster wrote:
> > >> >>>> qemu_rbd_open() neglects to check pool and image are present.
> > >> >>>> Reproducer:
> > >> >>>>
> > >> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
> > >> >>>>     Segmentation fault (core dumped)
> > >> >>>>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
> > >> >>>>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null)
> > >> >>>>
> > >> >>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> > >> >>>> always sets both pool and image.
> > >> >>>>
> > >> >>>> Doesn't affect -blockdev, because pool and image are mandatory in the
> > >> >>>> QAPI schema.
> > >> >>>>
> > >> >>>> Fix by adding the missing checks.
> > >> >>>>
> > >> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > >> >>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> > >> >>>> ---
> > >> >>>>  block/rbd.c | 10 +++++++---
> > >> >>>>  1 file changed, 7 insertions(+), 3 deletions(-)
> > >> >>> 
> > >> >>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> > >> >>
> > >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too
> > >> >> doesn't check whether those options are given but I guess they're just
> > >> >> as mandatory.
> > >> >
> > >> > Looks like it.  I'll try to stick a fix into v4.
> > >> 
> > >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going
> > >> through qemu_rbd_parse_filename()?
> > >
> > > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call
> > > qemu_rbd_parse_filename().  And in qemu_rbd_parse_filename(), it will
> > > complain if pool is not provided (and that is what causes the abort, not the
> > > missing image parameter).  So I think we are safe, but a nicer error message
> > > for a missing 'image' parameter might be nice anyway.
> > 
> > I now see the "we are safe" part, but not the "want a nicer error
> > message for a missing 'image'" part.  How can 'image' be missing?
> > 
> > qemu_rbd_parse_filename() parses a pseudo-filename of the form
> > 
> >     rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...]
> > 
> > It fails if
> > 
> > * the pseudo-filename doesn't start with "rbd:"
> > 
> > * doesn't contain '/' ("Pool name is required")
> > 
> > * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or
> >   too long (until the next commit)
> > 
> > * a KEY=VALUE doesn't contain '='
> > 
> > If it succeeds, "pool" and "image" are both set in @options.
> > 
> > Can you give me a reproducer for the error message you'd like me to
> > improve?
> 
> 
> Sure: qemu-img info rbd:mypool/:mon_host=192.168.15.180
> 
> 'image' is technically present in the options, but it is an empty string.
> 
> My thought was that it'd be nice to throw a similar error message for an
> empty string for 'image'.  As it is, the rbd library catches it, so it isn't
> catastrophic, but a nicer message would be helpful.
> 
> (My comment here assumes an empty string is not an acceptable image name for
> rbd)

I'm not concerned enough by this to want a v5.  I wasn't clear about that.

-Jeff

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 08/11] rbd: Revert -blockdev and -drive parameter auth-supported

[Eric Blake]

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

On 03/28/2017 03:24 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/27/2017 08:26 AM, Markus Armbruster wrote:
>>> This reverts half of commit 0a55679.  We're having second thoughts on
>>> the QAPI schema (and thus the external interface), and haven't reached
>>> consensus, yet.  Issues include:
>>>
>>
>>> Let's avoid painting ourselves into a corner now, and revert the
>>> feature for 2.9.
>>
>> There may still be some tweaks to improve the commit message and/or code
>> comments to clarify things that tripped me up until later in the
>> subthread, but now that I understand the difference between pseudo-file
>> format (where the key-value pair backdoor still works for libvirt's
>> usage of -drive file=rbd:...) and QemuOpts format (-drive driver=rbd,...
>> which didn't really exist in 2.8, and where we don't want to bake in
>> something we don't like in 2.9), I agree with the move.
> 
> I'm inserting ...
> 
>>> Note that users can still configure authentication methods with a
>>> configuration file.  They probably do that anyway if they use Ceph
>>> outside QEMU as well.
> 
> ... this hint right here:
> 
>    Further note that this doesn't affect use of key "auth-supported" in
>    -drive file=rbd:...:key=value.
> 
> Good enough?

Yes, that does the trick.

-- 
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 RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Daniel P. Berrange]

On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
> This reverts a part of commit 8a47e8e.  We're having second thoughts
> on the QAPI schema (and thus the external interface), and haven't
> reached consensus, yet.  Issues include:
> 
> * BlockdevOptionsRbd member @password-secret isn't actually a
>   password, it's a key generated by Ceph.
> 
> * We're not sure where member @password-secret belongs (see the
>   previous commit).
> 
> * How @password-secret interacts with settings from a configuration
>   file specified with @conf is undocumented.  I suspect it's untested,
>   too.
> 
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
> 
> Note that users can still configure an authentication key with a
> configuration file.  They probably do that anyway if they use Ceph
> outside QEMU as well.

NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
since we rely on the password-secret facility working to support apps
like openstack which won't configure the global config file for RBD.

Not a regression though, since blockdev-add is new - just means we won't
be able to use the new feature yet :-(

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 RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Max Reitz]

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

On 03.04.2017 13:37, Daniel P. Berrange wrote:
> On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
>> This reverts a part of commit 8a47e8e.  We're having second thoughts
>> on the QAPI schema (and thus the external interface), and haven't
>> reached consensus, yet.  Issues include:
>>
>> * BlockdevOptionsRbd member @password-secret isn't actually a
>>   password, it's a key generated by Ceph.
>>
>> * We're not sure where member @password-secret belongs (see the
>>   previous commit).
>>
>> * How @password-secret interacts with settings from a configuration
>>   file specified with @conf is undocumented.  I suspect it's untested,
>>   too.
>>
>> Let's avoid painting ourselves into a corner now, and revert the
>> feature for 2.9.
>>
>> Note that users can still configure an authentication key with a
>> configuration file.  They probably do that anyway if they use Ceph
>> outside QEMU as well.
> 
> NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
> since we rely on the password-secret facility working to support apps
> like openstack which won't configure the global config file for RBD.
> 
> Not a regression though, since blockdev-add is new - just means we won't
> be able to use the new feature yet :-(

How does it make blockdev-add totally useless? The only thing you cannot
do is set passwords for rbd. Can this not be added as a new feature in
the future?

Max


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Daniel P. Berrange]

On Mon, Apr 03, 2017 at 02:42:48PM +0200, Max Reitz wrote:
> On 03.04.2017 13:37, Daniel P. Berrange wrote:
> > On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
> >> This reverts a part of commit 8a47e8e.  We're having second thoughts
> >> on the QAPI schema (and thus the external interface), and haven't
> >> reached consensus, yet.  Issues include:
> >>
> >> * BlockdevOptionsRbd member @password-secret isn't actually a
> >>   password, it's a key generated by Ceph.
> >>
> >> * We're not sure where member @password-secret belongs (see the
> >>   previous commit).
> >>
> >> * How @password-secret interacts with settings from a configuration
> >>   file specified with @conf is undocumented.  I suspect it's untested,
> >>   too.
> >>
> >> Let's avoid painting ourselves into a corner now, and revert the
> >> feature for 2.9.
> >>
> >> Note that users can still configure an authentication key with a
> >> configuration file.  They probably do that anyway if they use Ceph
> >> outside QEMU as well.
> > 
> > NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
> > since we rely on the password-secret facility working to support apps
> > like openstack which won't configure the global config file for RBD.
> > 
> > Not a regression though, since blockdev-add is new - just means we won't
> > be able to use the new feature yet :-(
> 
> How does it make blockdev-add totally useless? The only thing you cannot
> do is set passwords for rbd. Can this not be added as a new feature in
> the future?

Sure, if you want to run an rbd server without any auth its usable, just
that isn't something you really want todo from a security pov.

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 RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Jeff Cody]

On Mon, Apr 03, 2017 at 02:04:42PM +0100, Daniel P. Berrange wrote:
> On Mon, Apr 03, 2017 at 02:42:48PM +0200, Max Reitz wrote:
> > On 03.04.2017 13:37, Daniel P. Berrange wrote:
> > > On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
> > >> This reverts a part of commit 8a47e8e.  We're having second thoughts
> > >> on the QAPI schema (and thus the external interface), and haven't
> > >> reached consensus, yet.  Issues include:
> > >>
> > >> * BlockdevOptionsRbd member @password-secret isn't actually a
> > >>   password, it's a key generated by Ceph.
> > >>
> > >> * We're not sure where member @password-secret belongs (see the
> > >>   previous commit).
> > >>
> > >> * How @password-secret interacts with settings from a configuration
> > >>   file specified with @conf is undocumented.  I suspect it's untested,
> > >>   too.
> > >>
> > >> Let's avoid painting ourselves into a corner now, and revert the
> > >> feature for 2.9.
> > >>
> > >> Note that users can still configure an authentication key with a
> > >> configuration file.  They probably do that anyway if they use Ceph
> > >> outside QEMU as well.
> > > 
> > > NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
> > > since we rely on the password-secret facility working to support apps
> > > like openstack which won't configure the global config file for RBD.
> > > 
> > > Not a regression though, since blockdev-add is new - just means we won't
> > > be able to use the new feature yet :-(
> > 
> > How does it make blockdev-add totally useless? The only thing you cannot
> > do is set passwords for rbd. Can this not be added as a new feature in
> > the future?
> 
> Sure, if you want to run an rbd server without any auth its usable, just
> that isn't something you really want todo from a security pov.
>

What about using a keyring for rbd?

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Max Reitz]

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

On 03.04.2017 15:04, Daniel P. Berrange wrote:
> On Mon, Apr 03, 2017 at 02:42:48PM +0200, Max Reitz wrote:
>> On 03.04.2017 13:37, Daniel P. Berrange wrote:
>>> On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
>>>> This reverts a part of commit 8a47e8e.  We're having second thoughts
>>>> on the QAPI schema (and thus the external interface), and haven't
>>>> reached consensus, yet.  Issues include:
>>>>
>>>> * BlockdevOptionsRbd member @password-secret isn't actually a
>>>>   password, it's a key generated by Ceph.
>>>>
>>>> * We're not sure where member @password-secret belongs (see the
>>>>   previous commit).
>>>>
>>>> * How @password-secret interacts with settings from a configuration
>>>>   file specified with @conf is undocumented.  I suspect it's untested,
>>>>   too.
>>>>
>>>> Let's avoid painting ourselves into a corner now, and revert the
>>>> feature for 2.9.
>>>>
>>>> Note that users can still configure an authentication key with a
>>>> configuration file.  They probably do that anyway if they use Ceph
>>>> outside QEMU as well.
>>>
>>> NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
>>> since we rely on the password-secret facility working to support apps
>>> like openstack which won't configure the global config file for RBD.
>>>
>>> Not a regression though, since blockdev-add is new - just means we won't
>>> be able to use the new feature yet :-(
>>
>> How does it make blockdev-add totally useless? The only thing you cannot
>> do is set passwords for rbd. Can this not be added as a new feature in
>> the future?
> 
> Sure, if you want to run an rbd server without any auth its usable, just
> that isn't something you really want todo from a security pov.

Indeed, but that's at least an rbd-specific issues. You can still use
blockdev-add for other block drivers just fine.

...and I just noticed that I have read your response the wrong way. I
didn't notice the "for RBD" and just read "this makes blockdev-add
largely useless from libvirt's POV" which sounded wrong. OK, I get it
then, sorry. :-)

Max


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

Re: [Qemu-devel] [PATCH RFC v3 for-2.9 09/11] rbd: Revert -blockdev parameter password-secret

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Mar 27, 2017 at 03:26:33PM +0200, Markus Armbruster wrote:
>> This reverts a part of commit 8a47e8e.  We're having second thoughts
>> on the QAPI schema (and thus the external interface), and haven't
>> reached consensus, yet.  Issues include:
>> 
>> * BlockdevOptionsRbd member @password-secret isn't actually a
>>   password, it's a key generated by Ceph.
>> 
>> * We're not sure where member @password-secret belongs (see the
>>   previous commit).
>> 
>> * How @password-secret interacts with settings from a configuration
>>   file specified with @conf is undocumented.  I suspect it's untested,
>>   too.
>> 
>> Let's avoid painting ourselves into a corner now, and revert the
>> feature for 2.9.
>> 
>> Note that users can still configure an authentication key with a
>> configuration file.  They probably do that anyway if they use Ceph
>> outside QEMU as well.
>
> NB, this makes blockdev-add largely useless for RBD from libvirt's POV,
> since we rely on the password-secret facility working to support apps
> like openstack which won't configure the global config file for RBD.

Libvirt could still supply a *local* configuration file.

> Not a regression though, since blockdev-add is new - just means we won't
> be able to use the new feature yet :-(

Unfortunate if true, but QMP interfaces need to be stable, and this one
clearly needs more thought.