diff mbox

[v2,3/5] block/rbd: parse all options via bdrv_parse_filename

Message ID fa6a26e1642ad7e593f3a0776a4a021997493f24.1488220970.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody Feb. 27, 2017, 6:58 p.m. UTC 
Get rid of qemu_rbd_parsename in favor of bdrv_parse_filename.
This simplifies a lot of the parsing as well, as we can treat everything
a bit simpler since nonexistent options are simply NULL pointers instead
of empy strings.

An important item to note:

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

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

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

Comments

Eric Blake Feb. 27, 2017, 10:35 p.m. UTC | #1 
On 02/27/2017 12:58 PM, Jeff Cody wrote:
> Get rid of qemu_rbd_parsename in favor of bdrv_parse_filename.
> This simplifies a lot of the parsing as well, as we can treat everything
> a bit simpler since nonexistent options are simply NULL pointers instead
> of empy strings.

s/empy/empty/

> 
> An important item to note:
> 
> Ceph has many extra option values that can be specified as key/value
> pairs.  This was handled previously in the driver by extracting the
> values that the QEMU driver cared about, and then blindly passing all
> extra options to rbd after splitting them into key/value pairs, and
> cleaning up any special character escaping.
> 
> The practice is continued in this patch; there is an option
> "keyvalue-pairs" that is populated with all the key/value pairs that the
> QEMU driver does not care about.  These key/value pairs will override
> any settings in the 'conf' configuration file, just as they did before.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 298 ++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 148 insertions(+), 150 deletions(-)
> 

> +
> +    /* The following are essentially all key/value pairs, and we treat
> +     * 'id' and 'conf' a bit special.  Key/value pairs may be in any order. */
> +    while (p) {

> +        if (!strcmp(name, "conf")) {
> +            qdict_put(options, "conf", qstring_from_str(value));
> +        } else if (!strcmp(name, "id")) {
> +            qdict_put(options, "user" , qstring_from_str(value));
> +        } else {
> +            char *tmp = g_malloc0(max_keypair_size);
> +            /* only use a delimiter if it is not the first keypair found */
> +            /* These are sets of unknown key/value pairs we'll pass along
> +             * to ceph */
> +            if (keypairs[0]) {
> +                snprintf(tmp, max_keypair_size, ":%s=%s", name, value);
> +                pstrcat(keypairs, max_keypair_size, tmp);
> +            } else {
> +                snprintf(keypairs, max_keypair_size, "%s=%s", name, value);
> +            }
> +            g_free(tmp);
> +        }
> +    }
> +
> +    if (keypairs[0]) {
> +        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));

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

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

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

>  
>  static BlockDriver bdrv_rbd = {
> -    .format_name        = "rbd",
> -    .instance_size      = sizeof(BDRVRBDState),
> -    .bdrv_needs_filename = true,
> -    .bdrv_file_open     = qemu_rbd_open,
> -    .bdrv_close         = qemu_rbd_close,
> -    .bdrv_create        = qemu_rbd_create,
> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> -    .bdrv_get_info      = qemu_rbd_getinfo,
> -    .create_opts        = &qemu_rbd_create_opts,
> -    .bdrv_getlength     = qemu_rbd_getlength,
> -    .bdrv_truncate      = qemu_rbd_truncate,
> -    .protocol_name      = "rbd",
> +    .format_name            = "rbd",
> +    .instance_size          = sizeof(BDRVRBDState),
> +    .bdrv_parse_filename    = qemu_rbd_parse_filename,
> +    .bdrv_file_open         = qemu_rbd_open,
> +    .bdrv_close             = qemu_rbd_close,
> +    .bdrv_create            = qemu_rbd_create,
> +    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> +    .bdrv_get_info          = qemu_rbd_getinfo,
> +    .create_opts            = &qemu_rbd_create_opts,

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

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

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

But since adding comments, and fixing minor things like &, doesn't
change the real meat of this patch, I can live with:

Reviewed-by: Eric Blake <eblake@redhat.com>
Jeff Cody Feb. 27, 2017, 10:56 p.m. UTC | #2 
On Mon, Feb 27, 2017 at 04:35:58PM -0600, Eric Blake wrote:
> On 02/27/2017 12:58 PM, Jeff Cody wrote:
> > Get rid of qemu_rbd_parsename in favor of bdrv_parse_filename.
> > This simplifies a lot of the parsing as well, as we can treat everything
> > a bit simpler since nonexistent options are simply NULL pointers instead
> > of empy strings.
> 
> s/empy/empty/
>

Thanks

> > 
> > An important item to note:
> > 
> > Ceph has many extra option values that can be specified as key/value
> > pairs.  This was handled previously in the driver by extracting the
> > values that the QEMU driver cared about, and then blindly passing all
> > extra options to rbd after splitting them into key/value pairs, and
> > cleaning up any special character escaping.
> > 
> > The practice is continued in this patch; there is an option
> > "keyvalue-pairs" that is populated with all the key/value pairs that the
> > QEMU driver does not care about.  These key/value pairs will override
> > any settings in the 'conf' configuration file, just as they did before.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 298 ++++++++++++++++++++++++++++++------------------------------
> >  1 file changed, 148 insertions(+), 150 deletions(-)
> > 
> 
> > +
> > +    /* The following are essentially all key/value pairs, and we treat
> > +     * 'id' and 'conf' a bit special.  Key/value pairs may be in any order. */
> > +    while (p) {
> 
> > +        if (!strcmp(name, "conf")) {
> > +            qdict_put(options, "conf", qstring_from_str(value));
> > +        } else if (!strcmp(name, "id")) {
> > +            qdict_put(options, "user" , qstring_from_str(value));
> > +        } else {
> > +            char *tmp = g_malloc0(max_keypair_size);
> > +            /* only use a delimiter if it is not the first keypair found */
> > +            /* These are sets of unknown key/value pairs we'll pass along
> > +             * to ceph */
> > +            if (keypairs[0]) {
> > +                snprintf(tmp, max_keypair_size, ":%s=%s", name, value);
> > +                pstrcat(keypairs, max_keypair_size, tmp);
> > +            } else {
> > +                snprintf(keypairs, max_keypair_size, "%s=%s", name, value);
> > +            }
> > +            g_free(tmp);
> > +        }
> > +    }
> > +
> > +    if (keypairs[0]) {
> > +        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));
> 
> Uggh.  Why are we compressing this into a single string, instead of
> using a GList?  True, we aren't exposing it through QAPI, but I still
> wonder if a smarter representation than a flat string is warranted.
> 

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

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

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

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

> >  
> >  static BlockDriver bdrv_rbd = {

> > -    .instance_size      = sizeof(BDRVRBDState),
> > -    .bdrv_needs_filename = true,
> > -    .bdrv_file_open     = qemu_rbd_open,
> > -    .bdrv_close         = qemu_rbd_close,
> > -    .bdrv_create        = qemu_rbd_create,
> > -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> > -    .bdrv_get_info      = qemu_rbd_getinfo,
> > -    .create_opts        = &qemu_rbd_create_opts,
> > -    .bdrv_getlength     = qemu_rbd_getlength,
> > -    .bdrv_truncate      = qemu_rbd_truncate,
> > -    .protocol_name      = "rbd",
> > +    .format_name            = "rbd",
> > +    .instance_size          = sizeof(BDRVRBDState),
> > +    .bdrv_parse_filename    = qemu_rbd_parse_filename,
> > +    .bdrv_file_open         = qemu_rbd_open,
> > +    .bdrv_close             = qemu_rbd_close,
> > +    .bdrv_create            = qemu_rbd_create,
> > +    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> > +    .bdrv_get_info          = qemu_rbd_getinfo,
> > +    .create_opts            = &qemu_rbd_create_opts,
> 
> Pointless &; might as well remove it for consistency while touching it.
> 

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

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

Thanks!

-Jeff
Eric Blake Feb. 27, 2017, 11:15 p.m. UTC | #3 
On 02/27/2017 04:56 PM, Jeff Cody wrote:

>>>  static BlockDriver bdrv_rbd = {
> 
>>> -    .instance_size      = sizeof(BDRVRBDState),
>>> -    .bdrv_needs_filename = true,
>>> -    .bdrv_file_open     = qemu_rbd_open,
>>> -    .bdrv_close         = qemu_rbd_close,
>>> -    .bdrv_create        = qemu_rbd_create,
>>> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
>>> -    .bdrv_get_info      = qemu_rbd_getinfo,
>>> -    .create_opts        = &qemu_rbd_create_opts,
>>> -    .bdrv_getlength     = qemu_rbd_getlength,
>>> -    .bdrv_truncate      = qemu_rbd_truncate,
>>> -    .protocol_name      = "rbd",
>>> +    .format_name            = "rbd",
>>> +    .instance_size          = sizeof(BDRVRBDState),
>>> +    .bdrv_parse_filename    = qemu_rbd_parse_filename,
>>> +    .bdrv_file_open         = qemu_rbd_open,
>>> +    .bdrv_close             = qemu_rbd_close,
>>> +    .bdrv_create            = qemu_rbd_create,
>>> +    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
>>> +    .bdrv_get_info          = qemu_rbd_getinfo,
>>> +    .create_opts            = &qemu_rbd_create_opts,
>>
>> Pointless &; might as well remove it for consistency while touching it.
>>
> 
> I'm not sure I understand - we need the '&' here for the .create_opts
> initializer, unless I am overlooking something.

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

Patch

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