[Qemu-devel] [PATCH RFC v2 0/9] rbd: Clean up API and code

[Qemu-devel] [PATCH RFC v2 0/9] rbd: Clean up API and code

[Markus Armbruster]

This is RFC because it needs more testing and an additional patch to
clean up around RbdAuthMethod.  I'm sending it out now in the hope of
getting some review.

Markus Armbruster (9):
  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: Rewrite the code to extract list-valued options
  rbd: Reject invalid authentication methods

 block/rbd.c          | 333 ++++++++++++++++++---------------------------------
 qapi-schema.json     |  21 ++--
 qapi/block-core.json |   2 +-
 3 files changed, 131 insertions(+), 225 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH RFC v2 1/9] 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>
---
 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 v2 1/9] rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

[Eric Blake]

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

On 03/24/2017 12:44 PM, 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.

Are now rejected when using -blockdev-add or QMP, but are still
(silently) ignored when using -drive [fixed later in series].

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

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

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


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

[Qemu-devel] [PATCH RFC v2 2/9] 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>
---
 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 v2 2/9] rbd: Fix to cleanly reject -drive without pool or image

[Eric Blake]

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

On 03/24/2017 12:44 PM, 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>
> ---
>  block/rbd.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH RFC v2 3/9] 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>
---
 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 v2 3/9] rbd: Don't limit length of parameter values

[Eric Blake]

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

On 03/24/2017 12:44 PM, 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.

And if librbd is NOT doing proper length checks, the bug lies there, not
in qemu.

> 
> 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>
> ---
>  block/rbd.c | 91 ++++++++++---------------------------------------------------
>  1 file changed, 14 insertions(+), 77 deletions(-)

Wow - you weren't kidding about the checks being laborious.

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

Getting rid of the forced length means we can't fail - nice.


> @@ -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));

Unrelated to your patch, and doesn't hold you up, but I'd love to
eventually get in my patch that shortens this pattern to:

qdict_put_str(options, "image", found_str);

(I still need to find time to write up a Coccinelle script to automate
the task; it's 2.10 material now)

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

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


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

[Qemu-devel] [PATCH RFC v2 4/9] 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>
---
 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 v2 4/9] rbd: Clean up after the previous commit

[Eric Blake]

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

On 03/24/2017 12:44 PM, Markus Armbruster wrote:
> This code in qemu_rbd_parse_filename()
> 
>     found_str = qemu_rbd_next_tok(p, '\0', &p);

This says to take a string starting at p, modify it in place, update p
to point past the delimiter (well, to nowhere since the delimiter of
'\0' means it is the last token parsed), and return the original
starting into found_str....

>     p = found_str;

...and this says to revert the change to p (why we had to pass &p
instead of NULL is beyond me).  Furthermore, reading the modifications
that qemu_rbd_next_tok() does in place (namely, looking for \ escape
sequences), it doesn't do any when delimiter is '\0'.  Prior to patch 3,
it was therefore useful as an idiom for length checking - but as you
killed even that aspect, I agree with your assessment that it is now a
no-op.

> 
> has no effect.  Drop it, and simplify qemu_rbd_next_tok().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH RFC v2 5/9] 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>
---
 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 v2 5/9] rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

[Eric Blake]

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

On 03/24/2017 12:44 PM, 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.

We could also use the hack "x-keyvalue-pairs" to make it obvious that it
is unsupported as an external interface (we've used the hack "x-image"
in blkverify/blkdebug), or even "=x-keyvalue-pairs" (combining your hack
with the blkverify hack).  In fact, if your leading '=' hack is nice
enough, we could retrofit blkverify and blkdebug to use it instead of
(or in addition to) the leading "x-" hack.

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

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

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


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

[Qemu-devel] [PATCH RFC v2 6/9] 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>
---
 block/rbd.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 2632533..e8db165 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",
+        },
+        /*
+         * Legacy keys accepted by qemu_rbd_parse_filename(), not in
+         * the QAPI schema
+         */
         {
             /*
              * HACK: name starts with '=' so that qemu_opts_parse()
@@ -338,6 +341,12 @@ 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 v2 6/9] rbd: Clean up runtime_opts, fix -drive to reject filename

[Eric Blake]

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

On 03/24/2017 12:44 PM, Markus Armbruster wrote:
> runtime_opts is used for three different purposes:

> 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>
> ---
>  block/rbd.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH RFC v2 7/9] 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 e8db165..6397626 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -375,7 +375,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");
@@ -406,19 +405,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) {
@@ -469,7 +460,6 @@ shutdown:
 
 exit:
     QDECREF(options);
-    qemu_opts_del(rbd_opts);
     return ret;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH RFC v2 8/9] rbd: Rewrite the code to extract list-valued options

[Markus Armbruster]

We have two list-values options:

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

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

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

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

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

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

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

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

Permits cleaning up runtime_opts, which fixes -drive to reject several
bogus parameters instead of silently ignoring them: host, port, auth;
server.*.P where P isn't host; port auth-supported.*.P where P isn't
auth.

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

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

Re: [Qemu-devel] [PATCH RFC v2 8/9] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

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

On 03/24/2017 12:44 PM, Markus Armbruster wrote:
> We have two list-values options:
> 
> * "server" is a list of InetSocketAddress.  We use members "host" and
>   "port", and silently ignore the rest.
> 
> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>   "auth".
> 
> Since qemu_rbd_open() takes options as a flattened QDict, options has
> keys of the form server.%d.host, server.%d.port and
> auth-supported.%d.auth, where %d counts up from zero.
> 

> 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.
> 
> Permits cleaning up runtime_opts, which fixes -drive to reject several
> bogus parameters instead of silently ignoring them: host, port, auth;
> server.*.P where P isn't host; port auth-supported.*.P where P isn't

s/host; port/host or port;/

> auth.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 150 +++++++++++++++++++++---------------------------------------
>  1 file changed, 52 insertions(+), 98 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 6397626..e962641 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  
> +#include <rbd/librbd.h>
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "block/block_int.h"
> @@ -20,8 +21,6 @@
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/qstring.h"
>  
> -#include <rbd/librbd.h>
> -

Still no mention of the include cleanup in the commit message. Not the
end of the world (I don't think it's worth splitting into a separate
patch, and the lack of commit message mention isn't fatal).

> @@ -635,7 +590,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      secretid = qemu_opt_get(opts, "password-secret");
> -
>      pool           = qemu_opt_get(opts, "pool");

Spurious change that should belong to an earlier commit?

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

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


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

[Qemu-devel] [PATCH RFC v2 9/9] rbd: Reject invalid authentication methods

[Markus Armbruster]

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

diff --git a/block/rbd.c b/block/rbd.c
index e962641..19fd820 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -15,6 +15,7 @@
 
 #include <rbd/librbd.h>
 #include "qapi/error.h"
+#include "qapi/util.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
 #include "crypto/secret.h"
@@ -506,11 +507,17 @@ static char *qemu_rbd_auth(QDict *options, Error **errp)
         }
 
         vals[i] = qstring_get_str(qobject_to_qstring(val));
+        if (qapi_enum_parse(RbdAuthSupport_lookup, vals[i],
+                            RBD_AUTH_SUPPORT__MAX, -1, errp) < 0) {
+            rados_str = NULL;
+            goto out;
+        }
         qdict_del(options, keybuf);
     }
     vals[i] = NULL;
 
     rados_str = i ? g_strjoinv(";", (char **)vals) : NULL;
+out:
     g_free(vals);
     return rados_str;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v2 9/9] rbd: Reject invalid authentication methods

[Eric Blake]

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

On 03/24/2017 12:44 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

In short, since we have a QAPI enum of valid types, we should verify
that all members of the list match that enum.

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