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

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

[Jeff Cody]

This series adds blockdev-add for rbd.

However, there is an area that will likely need to change.  In the RBD
driver, all options for Ceph are supported, but the qemu driver is not
explicitly aware of all the options.

There are a few options that the QEMU driver cares about and handles, while
the other options are merely split out until their string key/value pairs
and passed along to Ceph.  In patches 3 and 4, this patch series does
the same practice, and parses all the extra options as a long string.

For QAPI, this is not acceptable - so I presume the best way to handle it
is with an array schema of structs.  I'll reply to the specific patches in
the problem areas, and see what the preference is from an API perspective
 to handle this.




Jeff Cody (4):
  block/rbd: don't copy strings in qemu_rbd_next_tok()
  block/rbd: code movement
  block/rbd: parse all options via bdrv_parse_filename
  block/rbd: Add blockdev-add support

 block/rbd.c          | 435 +++++++++++++++++++++++++++++----------------------
 qapi/block-core.json |  47 +++++-
 2 files changed, 289 insertions(+), 193 deletions(-)

-- 
2.9.3

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

[Jeff Cody]

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

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

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

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

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

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

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> This patch is prep work for parsing options for .bdrv_parse_filename,
> and using QDict options.
>
> The function qemu_rbd_next_tok() searched for various key/value pairs,
> and copied them into buffers.  This will soon be an unnecessary extra
> step, so we will now return found strings by reference only, and
> offload the responsibility for safely handling/coping these strings to
> the caller.
>
> This also cleans up error handling some, as the callers now rely on
> the Error object to determine if there is a parse error.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 99 +++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 22e8e69..3f1a9de 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -102,10 +102,10 @@ typedef struct BDRVRBDState {
>      char *snap;
>  } BDRVRBDState;
>  
> -static int qemu_rbd_next_tok(char *dst, int dst_len,
> -                             char *src, char delim,
> -                             const char *name,
> -                             char **p, Error **errp)
> +static char *qemu_rbd_next_tok(int max_len,
> +                               char *src, char delim,
> +                               const char *name,
> +                               char **p, Error **errp)
>  {
>      int l;
>      char *end;

       *p = NULL;

       if (delim != '\0') {
           for (end = src; *end; ++end) {
               if (*end == delim) {
                   break;
               }
               if (*end == '\\' && end[1] != '\0') {
                   end++;
               }
           }
           if (*end == delim) {
               *p = end + 1;
               *end = '\0';
>          }
>      }
>      l = strlen(src);

Not this patch's problem: this is a rather thoughtless way to say

       l = end - src;

> -    if (l >= dst_len) {
> +    if (l >= max_len) {
>          error_setg(errp, "%s too long", name);
> -        return -EINVAL;
> +        return NULL;
>      } else if (l == 0) {
>          error_setg(errp, "%s too short", name);
> -        return -EINVAL;
> +        return NULL;
>      }
>  
> -    pstrcpy(dst, dst_len, src);
> -
> -    return 0;
> +    return src;
>  }

Note for later:

1. This function always dereferences @src.
2. If @delim, it sets *@p to point behind @src plus the delimiter,
   else to NULL
3. It returns NULL exactly when it sets an error.
4. It returns NULL and sets an error when @src is empty.

Not this patch's problem, but I have to say it: whoever wrote this code
should either write simpler functions or get into the habit of writing
function contract comments.  Because having to document your
embarrassingly complicated shit is great motivation to simplify (pardon
my french).

>  
>  static void qemu_rbd_unescape(char *src)
> @@ -162,7 +160,9 @@ static int qemu_rbd_parsename(const char *filename,

This is a parser.  As so often, it is a parser without any hint on what
it's supposed to parse, let alone a grammar.  Experience tells that
these are wrong more often than not, and exposing me to yet another one
is a surefire way to make me grumpy.  Not your fault, of course.

>  {
>      const char *start;
>      char *p, *buf;
> -    int ret;
> +    int ret = 0;
> +    char *found_str;
> +    Error *local_err = NULL;
>  
>      if (!strstart(filename, "rbd:", &start)) {
>          error_setg(errp, "File name must start with 'rbd:'");
           return -EINVAL;
       }

       buf = g_strdup(start);
       p = buf;

This assignment to @p ...

>      *snap = '\0';
>      *conf = '\0';
>  
> -    ret = qemu_rbd_next_tok(pool, pool_len, p,
> -                            '/', "pool name", &p, errp);
> -    if (ret < 0 || !p) {
> +    found_str = qemu_rbd_next_tok(pool_len, p,
> +                                 '/', "pool name", &p, &local_err);

... is dead, because qemu_rbd_next_tok() assigns to it unconditionally.

The call extracts the part up to the first unescaped '/' or the end of
the string.

> +    if (local_err) {
> +        goto done;
> +    }
> +    if (!p) {

We extracted to end of string, i.e. we didn't find '/'.

>          ret = -EINVAL;
> +        error_setg(errp, "Pool name is required");
>          goto done;
>      }
> -    qemu_rbd_unescape(pool);
> +    qemu_rbd_unescape(found_str);
> +    g_strlcpy(pool, found_str, pool_len);

Before, we copy, then unescape the copy.

After, we unescape in place, then copy.

Unescaping can't lengthen the string.  Therefore, this is safe as long
as nothing else uses this part of @buf.

>  
>      if (strchr(p, '@')) {
> -        ret = qemu_rbd_next_tok(name, name_len, p,
> -                                '@', "object name", &p, errp);
> -        if (ret < 0) {
> +        found_str = qemu_rbd_next_tok(name_len, p,
> +                                     '@', "object name", &p, &local_err);

Extracts from first unescaped '/' to next unescaped '@' or end of string.

@p can't be null.

> +        if (local_err) {
>              goto done;
>          }
> -        ret = qemu_rbd_next_tok(snap, snap_len, p,
> -                                ':', "snap name", &p, errp);
> -        qemu_rbd_unescape(snap);
> +        qemu_rbd_unescape(found_str);
> +        g_strlcpy(name, found_str, name_len);
> +
> +        found_str = qemu_rbd_next_tok(snap_len, p,
> +                                      ':', "snap name", &p, &local_err);

>From that '@' to next ':' or end of string.

> +        if (local_err) {
> +            goto done;
> +        }
> +        qemu_rbd_unescape(found_str);
> +        g_strlcpy(snap, found_str, snap_len);

This confused me, but I figured it out: you're moving the @name unescape
from after the conditional into both its arms.  This is the first copy.

>      } else {
> -        ret = qemu_rbd_next_tok(name, name_len, p,
> -                                ':', "object name", &p, errp);
> +        found_str = qemu_rbd_next_tok(name_len, p,
> +                                     ':', "object name", &p, &local_err);

>From first '/' to next ':' or end of string.

Indentation off by one.

> +        if (local_err) {
> +            goto done;
> +        }
> +        qemu_rbd_unescape(found_str);

This is the second copy of the moved unescape.

> +        g_strlcpy(name, found_str, name_len);
>      }
> -    qemu_rbd_unescape(name);

This is where the copies come from.

> -    if (ret < 0 || !p) {
> +    if (!p) {

We didn't find ':'.

>          goto done;
>      }
>  
> -    ret = qemu_rbd_next_tok(conf, conf_len, p,
> -                            '\0', "configuration", &p, errp);
> +    found_str = qemu_rbd_next_tok(conf_len, p,
> +                                 '\0', "configuration", &p, &local_err);

>From that ':' to end of string.

> +    if (local_err) {
> +        goto done;
> +    }
> +    g_strlcpy(conf, found_str, conf_len);

No unescape?  Strange... but your patch doesn't change anything.

>  
>  done:
> +    if (local_err) {
> +        ret = -EINVAL;
> +        error_propagate(errp, local_err);
> +    }
>      g_free(buf);
>      return ret;
>  }

Okay, now someone tell me what it's supposed to parse, and I tell you
whether it works.

Unescaping in place looks safe.

> @@ -262,17 +286,18 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,

Great, another parser for an unknown language.  I'll pass.

>                               Error **errp)
>  {
>      char *p, *buf;
> -    char name[RBD_MAX_CONF_NAME_SIZE];
> -    char value[RBD_MAX_CONF_VAL_SIZE];
> +    char *name;
> +    char *value;
> +    Error *local_err = NULL;
>      int ret = 0;
>  
>      buf = g_strdup(conf);
>      p = buf;

Again, dead assignment to @p.

>  
>      while (p) {
> -        ret = qemu_rbd_next_tok(name, sizeof(name), p,
> -                                '=', "conf option name", &p, errp);
> -        if (ret < 0) {
> +        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
> +                                 '=', "conf option name", &p, &local_err);
> +        if (local_err) {
>              break;
>          }
>          qemu_rbd_unescape(name);

Again, you change to unescape in place.

> @@ -283,9 +308,9 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
>              break;
>          }
>  
> -        ret = qemu_rbd_next_tok(value, sizeof(value), p,
> -                                ':', "conf option value", &p, errp);
> -        if (ret < 0) {
> +        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
> +                                  ':', "conf option value", &p, &local_err);
> +        if (local_err) {
>              break;
>          }
>          qemu_rbd_unescape(value);

Likewise.

> @@ -313,6 +338,10 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
>          }
>      }
>  
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +    }
>      g_free(buf);
>      return ret;
>  }

Again, unescaping in place looks safe.

Preferably with the two dead assignments dropped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Jeff Cody]

On Mon, Feb 27, 2017 at 05:39:45PM +0100, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > This patch is prep work for parsing options for .bdrv_parse_filename,
> > and using QDict options.
> >
> > The function qemu_rbd_next_tok() searched for various key/value pairs,
> > and copied them into buffers.  This will soon be an unnecessary extra
> > step, so we will now return found strings by reference only, and
> > offload the responsibility for safely handling/coping these strings to
> > the caller.
> >
> > This also cleans up error handling some, as the callers now rely on
> > the Error object to determine if there is a parse error.
> >
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 99 +++++++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 64 insertions(+), 35 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 22e8e69..3f1a9de 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -102,10 +102,10 @@ typedef struct BDRVRBDState {
> >      char *snap;
> >  } BDRVRBDState;
> >  
> > -static int qemu_rbd_next_tok(char *dst, int dst_len,
> > -                             char *src, char delim,
> > -                             const char *name,
> > -                             char **p, Error **errp)
> > +static char *qemu_rbd_next_tok(int max_len,
> > +                               char *src, char delim,
> > +                               const char *name,
> > +                               char **p, Error **errp)
> >  {
> >      int l;
> >      char *end;
> 
>        *p = NULL;
> 
>        if (delim != '\0') {
>            for (end = src; *end; ++end) {
>                if (*end == delim) {
>                    break;
>                }
>                if (*end == '\\' && end[1] != '\0') {
>                    end++;
>                }
>            }
>            if (*end == delim) {
>                *p = end + 1;
>                *end = '\0';
> >          }
> >      }
> >      l = strlen(src);
> 
> Not this patch's problem: this is a rather thoughtless way to say
> 
>        l = end - src;
> 
> > -    if (l >= dst_len) {
> > +    if (l >= max_len) {
> >          error_setg(errp, "%s too long", name);
> > -        return -EINVAL;
> > +        return NULL;
> >      } else if (l == 0) {
> >          error_setg(errp, "%s too short", name);
> > -        return -EINVAL;
> > +        return NULL;
> >      }
> >  
> > -    pstrcpy(dst, dst_len, src);
> > -
> > -    return 0;
> > +    return src;
> >  }
> 
> Note for later:
> 
> 1. This function always dereferences @src.
> 2. If @delim, it sets *@p to point behind @src plus the delimiter,
>    else to NULL
> 3. It returns NULL exactly when it sets an error.
> 4. It returns NULL and sets an error when @src is empty.
> 
> Not this patch's problem, but I have to say it: whoever wrote this code
> should either write simpler functions or get into the habit of writing
> function contract comments.  Because having to document your
> embarrassingly complicated shit is great motivation to simplify (pardon
> my french).
>

Heh.  I had to read and re-read this function multiple times to get a feel
for what it was doing.


> >  
> >  static void qemu_rbd_unescape(char *src)
> > @@ -162,7 +160,9 @@ static int qemu_rbd_parsename(const char *filename,
> 
> This is a parser.  As so often, it is a parser without any hint on what
> it's supposed to parse, let alone a grammar.  Experience tells that
> these are wrong more often than not, and exposing me to yet another one
> is a surefire way to make me grumpy.  Not your fault, of course.
> 
> >  {
> >      const char *start;
> >      char *p, *buf;
> > -    int ret;
> > +    int ret = 0;
> > +    char *found_str;
> > +    Error *local_err = NULL;
> >  
> >      if (!strstart(filename, "rbd:", &start)) {
> >          error_setg(errp, "File name must start with 'rbd:'");
>            return -EINVAL;
>        }
> 
>        buf = g_strdup(start);
>        p = buf;
> 
> This assignment to @p ...
> 
> >      *snap = '\0';
> >      *conf = '\0';
> >  
> > -    ret = qemu_rbd_next_tok(pool, pool_len, p,
> > -                            '/', "pool name", &p, errp);
> > -    if (ret < 0 || !p) {
> > +    found_str = qemu_rbd_next_tok(pool_len, p,
> > +                                 '/', "pool name", &p, &local_err);
> 
> ... is dead, because qemu_rbd_next_tok() assigns to it unconditionally.
> 

While that is true, @p is also used as the src argument to
qemu_rbd_next_tok() in addition (second arg).  We could just pass in @buf
for that argument, but using @p keeps it consistent with the other calls.



> The call extracts the part up to the first unescaped '/' or the end of
> the string.
> 
> > +    if (local_err) {
> > +        goto done;
> > +    }
> > +    if (!p) {
> 
> We extracted to end of string, i.e. we didn't find '/'.
> 
> >          ret = -EINVAL;
> > +        error_setg(errp, "Pool name is required");
> >          goto done;
> >      }
> > -    qemu_rbd_unescape(pool);
> > +    qemu_rbd_unescape(found_str);
> > +    g_strlcpy(pool, found_str, pool_len);
> 
> Before, we copy, then unescape the copy.
> 
> After, we unescape in place, then copy.
> 
> Unescaping can't lengthen the string.  Therefore, this is safe as long
> as nothing else uses this part of @buf.
> 
> >  
> >      if (strchr(p, '@')) {
> > -        ret = qemu_rbd_next_tok(name, name_len, p,
> > -                                '@', "object name", &p, errp);
> > -        if (ret < 0) {
> > +        found_str = qemu_rbd_next_tok(name_len, p,
> > +                                     '@', "object name", &p, &local_err);
> 
> Extracts from first unescaped '/' to next unescaped '@' or end of string.
> 
> @p can't be null.
> 
> > +        if (local_err) {
> >              goto done;
> >          }
> > -        ret = qemu_rbd_next_tok(snap, snap_len, p,
> > -                                ':', "snap name", &p, errp);
> > -        qemu_rbd_unescape(snap);
> > +        qemu_rbd_unescape(found_str);
> > +        g_strlcpy(name, found_str, name_len);
> > +
> > +        found_str = qemu_rbd_next_tok(snap_len, p,
> > +                                      ':', "snap name", &p, &local_err);
> 
> From that '@' to next ':' or end of string.
> 
> > +        if (local_err) {
> > +            goto done;
> > +        }
> > +        qemu_rbd_unescape(found_str);
> > +        g_strlcpy(snap, found_str, snap_len);
> 
> This confused me, but I figured it out: you're moving the @name unescape
> from after the conditional into both its arms.  This is the first copy.
> 
> >      } else {
> > -        ret = qemu_rbd_next_tok(name, name_len, p,
> > -                                ':', "object name", &p, errp);
> > +        found_str = qemu_rbd_next_tok(name_len, p,
> > +                                     ':', "object name", &p, &local_err);
> 
> From first '/' to next ':' or end of string.
> 
> Indentation off by one.
> 

Those pesky single quotes are almost invisible...


> > +        if (local_err) {
> > +            goto done;
> > +        }
> > +        qemu_rbd_unescape(found_str);
> 
> This is the second copy of the moved unescape.
> 
> > +        g_strlcpy(name, found_str, name_len);
> >      }
> > -    qemu_rbd_unescape(name);
> 
> This is where the copies come from.
> 
> > -    if (ret < 0 || !p) {
> > +    if (!p) {
> 
> We didn't find ':'.
> 
> >          goto done;
> >      }
> >  
> > -    ret = qemu_rbd_next_tok(conf, conf_len, p,
> > -                            '\0', "configuration", &p, errp);
> > +    found_str = qemu_rbd_next_tok(conf_len, p,
> > +                                 '\0', "configuration", &p, &local_err);
> 
> From that ':' to end of string.
> 
> > +    if (local_err) {
> > +        goto done;
> > +    }
> > +    g_strlcpy(conf, found_str, conf_len);
> 
> No unescape?  Strange... but your patch doesn't change anything.
> 

This part is essentially chunked off to the end of the string, as you noted
above.  This chunk is then parsed (and unescaped) in qemu_rbd_set_conf().

> >  
> >  done:
> > +    if (local_err) {
> > +        ret = -EINVAL;
> > +        error_propagate(errp, local_err);
> > +    }
> >      g_free(buf);
> >      return ret;
> >  }
> 
> Okay, now someone tell me what it's supposed to parse, and I tell you
> whether it works.
> 
> Unescaping in place looks safe.
> 
> > @@ -262,17 +286,18 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
> 
> Great, another parser for an unknown language.  I'll pass.
> 
> >                               Error **errp)
> >  {
> >      char *p, *buf;
> > -    char name[RBD_MAX_CONF_NAME_SIZE];
> > -    char value[RBD_MAX_CONF_VAL_SIZE];
> > +    char *name;
> > +    char *value;
> > +    Error *local_err = NULL;
> >      int ret = 0;
> >  
> >      buf = g_strdup(conf);
> >      p = buf;
> 
> Again, dead assignment to @p.
> 

I'm inclined to leave it for the same reason as the first instance.

> >  
> >      while (p) {
> > -        ret = qemu_rbd_next_tok(name, sizeof(name), p,
> > -                                '=', "conf option name", &p, errp);
> > -        if (ret < 0) {
> > +        name = qemu_rbd_next_tok(RBD_MAX_CONF_NAME_SIZE, p,
> > +                                 '=', "conf option name", &p, &local_err);
> > +        if (local_err) {
> >              break;
> >          }
> >          qemu_rbd_unescape(name);
> 
> Again, you change to unescape in place.
> 
> > @@ -283,9 +308,9 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
> >              break;
> >          }
> >  
> > -        ret = qemu_rbd_next_tok(value, sizeof(value), p,
> > -                                ':', "conf option value", &p, errp);
> > -        if (ret < 0) {
> > +        value = qemu_rbd_next_tok(RBD_MAX_CONF_VAL_SIZE, p,
> > +                                  ':', "conf option value", &p, &local_err);
> > +        if (local_err) {
> >              break;
> >          }
> >          qemu_rbd_unescape(value);
> 
> Likewise.
> 
> > @@ -313,6 +338,10 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
> >          }
> >      }
> >  
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        ret = -EINVAL;
> > +    }
> >      g_free(buf);
> >      return ret;
> >  }
> 
> Again, unescaping in place looks safe.
> 
> Preferably with the two dead assignments dropped:

How strongly do you feel about those?

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

[Qemu-devel] [PATCH 2/4] block/rbd: code movement

[Jeff Cody]

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

diff --git a/block/rbd.c b/block/rbd.c
index 3f1a9de..c8d4eb1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -357,6 +357,51 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
     }
 }
 
+static QemuOptsList runtime_opts = {
+    .name = "rbd",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "Specification of the rbd image",
+        },
+        {
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret providing the password",
+        },
+        {
+            .name = "conf",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "pool",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "image",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "snapshot",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            /* you might be tempted to call this 'id' to match
+             * the ceph documentation, but then it'll get gobbled
+             * up in the block layer before it gets to the image driver */
+            .name = "rbd-id",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "keyvalue-pairs",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     Error *local_err = NULL;
@@ -500,25 +545,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-/* TODO Convert to fine grained options */
-static QemuOptsList runtime_opts = {
-    .name = "rbd",
-    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
-    .desc = {
-        {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-            .help = "Specification of the rbd image",
-        },
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
-        { /* end of list */ }
-    },
-};
-
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/4] block/rbd: code movement

[Daniel P. Berrange]

Describing this as "code movement" when the added & removed chunks are not
identical is a bit misleading.

Can you expand the commit message to explain why the extra options are
being added

On Mon, Feb 27, 2017 at 02:30:39AM -0500, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 64 +++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 3f1a9de..c8d4eb1 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -357,6 +357,51 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
>      }
>  }
>  
> +static QemuOptsList runtime_opts = {
> +    .name = "rbd",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "filename",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Specification of the rbd image",
> +        },
> +        {
> +            .name = "password-secret",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
> +        {
> +            .name = "conf",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "pool",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "image",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "snapshot",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            /* you might be tempted to call this 'id' to match
> +             * the ceph documentation, but then it'll get gobbled
> +             * up in the block layer before it gets to the image driver */
> +            .name = "rbd-id",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "keyvalue-pairs",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>  {
>      Error *local_err = NULL;
> @@ -500,25 +545,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>      qemu_aio_unref(acb);
>  }
>  
> -/* TODO Convert to fine grained options */
> -static QemuOptsList runtime_opts = {
> -    .name = "rbd",
> -    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> -    .desc = {
> -        {
> -            .name = "filename",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Specification of the rbd image",
> -        },
> -        {
> -            .name = "password-secret",
> -            .type = QEMU_OPT_STRING,
> -            .help = "ID of secret providing the password",
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>                           Error **errp)
>  {
> -- 
> 2.9.3
> 

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 2/4] block/rbd: code movement

[Jeff Cody]

On Mon, Feb 27, 2017 at 09:28:56AM +0000, Daniel P. Berrange wrote:
> 
> Describing this as "code movement" when the added & removed chunks are not
> identical is a bit misleading.
> 
> Can you expand the commit message to explain why the extra options are
> being added

Ah, sorry - yes.  The lack of a commit message was a mistake when squashing
patches.

> 
> On Mon, Feb 27, 2017 at 02:30:39AM -0500, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 64 +++++++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 45 insertions(+), 19 deletions(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 3f1a9de..c8d4eb1 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -357,6 +357,51 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
> >      }
> >  }
> >  
> > +static QemuOptsList runtime_opts = {
> > +    .name = "rbd",
> > +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > +    .desc = {
> > +        {
> > +            .name = "filename",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "Specification of the rbd image",
> > +        },
> > +        {
> > +            .name = "password-secret",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "ID of secret providing the password",
> > +        },
> > +        {
> > +            .name = "conf",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "pool",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "image",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "snapshot",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            /* you might be tempted to call this 'id' to match
> > +             * the ceph documentation, but then it'll get gobbled
> > +             * up in the block layer before it gets to the image driver */
> > +            .name = "rbd-id",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> > +            .name = "keyvalue-pairs",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> >  static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
> >  {
> >      Error *local_err = NULL;
> > @@ -500,25 +545,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
> >      qemu_aio_unref(acb);
> >  }
> >  
> > -/* TODO Convert to fine grained options */
> > -static QemuOptsList runtime_opts = {
> > -    .name = "rbd",
> > -    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > -    .desc = {
> > -        {
> > -            .name = "filename",
> > -            .type = QEMU_OPT_STRING,
> > -            .help = "Specification of the rbd image",
> > -        },
> > -        {
> > -            .name = "password-secret",
> > -            .type = QEMU_OPT_STRING,
> > -            .help = "ID of secret providing the password",
> > -        },
> > -        { /* end of list */ }
> > -    },
> > -};
> > -
> >  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >                           Error **errp)
> >  {
> > -- 
> > 2.9.3
> > 
> 
> 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/ :|

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

[Jeff Cody]

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

An important item to note:

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

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

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

diff --git a/block/rbd.c b/block/rbd.c
index c8d4eb1..2eee502 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, "rbd-id" , 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) {
@@ -315,26 +330,11 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
         }
         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;
         }
     }
 
@@ -408,27 +408,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);
@@ -436,35 +425,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, "rbd-id");
+    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;
     }
@@ -495,6 +504,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;
 }
 
@@ -549,15 +562,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);
@@ -568,44 +576,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, "rbd-id");
+    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) {
@@ -1059,18 +1059,18 @@ static QemuOptsList qemu_rbd_create_opts = {
 };
 
 static BlockDriver bdrv_rbd = {
-    .format_name        = "rbd",
-    .instance_size      = sizeof(BDRVRBDState),
-    .bdrv_needs_filename = true,
-    .bdrv_file_open     = qemu_rbd_open,
-    .bdrv_close         = qemu_rbd_close,
-    .bdrv_create        = qemu_rbd_create,
-    .bdrv_has_zero_init = bdrv_has_zero_init_1,
-    .bdrv_get_info      = qemu_rbd_getinfo,
-    .create_opts        = &qemu_rbd_create_opts,
-    .bdrv_getlength     = qemu_rbd_getlength,
-    .bdrv_truncate      = qemu_rbd_truncate,
-    .protocol_name      = "rbd",
+    .format_name            = "rbd",
+    .instance_size          = sizeof(BDRVRBDState),
+    .bdrv_parse_filename    = qemu_rbd_parse_filename,
+    .bdrv_file_open         = qemu_rbd_open,
+    .bdrv_close             = qemu_rbd_close,
+    .bdrv_create            = qemu_rbd_create,
+    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .bdrv_get_info          = qemu_rbd_getinfo,
+    .create_opts            = &qemu_rbd_create_opts,
+    .bdrv_getlength         = qemu_rbd_getlength,
+    .bdrv_truncate          = qemu_rbd_truncate,
+    .protocol_name          = "rbd",
 
     .bdrv_aio_readv         = qemu_rbd_aio_readv,
     .bdrv_aio_writev        = qemu_rbd_aio_writev,
-- 
2.9.3

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

[Jeff Cody]

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f82d35..08a1419 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2111,6 +2111,7 @@
 # @replication: Since 2.8
 # @ssh: Since 2.8
 # @iscsi: Since 2.9
+# @rbd: Since 2.9
 #
 # Since: 2.0
 ##
@@ -2120,7 +2121,7 @@
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
             'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
-            'vpc', 'vvfat' ] }
+            'vpc', 'vvfat', 'rbd' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2376,7 +2377,6 @@
             'path': 'str',
             '*user': 'str' } }
 
-
 ##
 # @BlkdebugEvent:
 #
@@ -2666,6 +2666,47 @@
             '*timeout': 'int' } }
 
 ##
+# @BlockdevOptionsRbd:
+#
+# @pool:               Ceph pool name
+#
+# @image:              Image name in the Ceph pool
+#
+# @conf:               # optional path to Ceph configuration file.  Values
+#                      in the configuration file will be overridden by
+#                      options specified via QAPI.
+#
+# @snapshot:           #optional Ceph snapshot name
+#
+# @rbd-id:             #optional Ceph id name
+#
+# @password-secret:    #optional The ID of a QCryptoSecret object providing
+#                       the password for the login.
+#
+# @keyvalue-pairs:     #optional  string containing key/value pairs for
+#                      additional Ceph configuration, not including "id" or "conf"
+#                      options. This can be used to specify any of the options
+#                      that Ceph supports.  The format is of the form:
+#                           key1=value1:key2=value2:[...]
+#
+#                      Special characters such as ":" and "=" can be escaped
+#                      with a '\' character, which means the QAPI needs an
+#                      extra '\' character to pass the needed escape character.
+#                      For example:
+#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsRbd',
+  'data': { 'pool': 'str',
+            'image': 'str',
+            '*conf': 'str',
+            '*snapshot': 'str',
+            '*rbd-id': 'str',
+            '*password-secret': 'str',
+            '*keyvalue-pairs': 'str' } }
+
+##
 # @ReplicationMode:
 #
 # An enumeration of replication modes.
@@ -2863,7 +2904,7 @@
       'qed':        'BlockdevOptionsGenericCOWFormat',
       'quorum':     'BlockdevOptionsQuorum',
       'raw':        'BlockdevOptionsRaw',
-# TODO rbd: Wait for structured options
+      'rbd':        'BlockdevOptionsRbd',
       'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
       'ssh':        'BlockdevOptionsSsh',
-- 
2.9.3

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

[Jeff Cody]

On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5f82d35..08a1419 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2111,6 +2111,7 @@
>  # @replication: Since 2.8
>  # @ssh: Since 2.8
>  # @iscsi: Since 2.9
> +# @rbd: Since 2.9
>  #
>  # Since: 2.0
>  ##
> @@ -2120,7 +2121,7 @@
>              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
>              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
>              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> -            'vpc', 'vvfat' ] }
> +            'vpc', 'vvfat', 'rbd' ] }
>  
>  ##
>  # @BlockdevOptionsFile:
> @@ -2376,7 +2377,6 @@
>              'path': 'str',
>              '*user': 'str' } }
>  
> -
>  ##
>  # @BlkdebugEvent:
>  #
> @@ -2666,6 +2666,47 @@
>              '*timeout': 'int' } }
>  
>  ##
> +# @BlockdevOptionsRbd:
> +#
> +# @pool:               Ceph pool name
> +#
> +# @image:              Image name in the Ceph pool
> +#
> +# @conf:               # optional path to Ceph configuration file.  Values
> +#                      in the configuration file will be overridden by
> +#                      options specified via QAPI.
> +#
> +# @snapshot:           #optional Ceph snapshot name
> +#
> +# @rbd-id:             #optional Ceph id name
> +#
> +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> +#                       the password for the login.
> +#
> +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> +#                      additional Ceph configuration, not including "id" or "conf"
> +#                      options. This can be used to specify any of the options
> +#                      that Ceph supports.  The format is of the form:
> +#                           key1=value1:key2=value2:[...]
> +#
> +#                      Special characters such as ":" and "=" can be escaped
> +#                      with a '\' character, which means the QAPI needs an
> +#                      extra '\' character to pass the needed escape character.
> +#                      For example:
> +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> +#

This is the key / value pair issue mentioned in the cover letter.  Encoding
all the options as a string like this is ugly.  What is the preference on
how to handle these via QAPI, when the actual key and value pairs could be
anything?   Talking with Markus on IRC, one option he mentioned was an array
of a generic struct of 'key' and 'value' pairs.

Do the libvirt folks have any interface preferences here?


> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsRbd',
> +  'data': { 'pool': 'str',
> +            'image': 'str',
> +            '*conf': 'str',
> +            '*snapshot': 'str',
> +            '*rbd-id': 'str',
> +            '*password-secret': 'str',
> +            '*keyvalue-pairs': 'str' } }
> +
> +##
>  # @ReplicationMode:
>  #
>  # An enumeration of replication modes.
> @@ -2863,7 +2904,7 @@
>        'qed':        'BlockdevOptionsGenericCOWFormat',
>        'quorum':     'BlockdevOptionsQuorum',
>        'raw':        'BlockdevOptionsRaw',
> -# TODO rbd: Wait for structured options
> +      'rbd':        'BlockdevOptionsRbd',
>        'replication':'BlockdevOptionsReplication',
>  # TODO sheepdog: Wait for structured options
>        'ssh':        'BlockdevOptionsSsh',
> -- 
> 2.9.3
> 

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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 02:36:13AM -0500, Jeff Cody wrote:
> On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 5f82d35..08a1419 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2111,6 +2111,7 @@
> >  # @replication: Since 2.8
> >  # @ssh: Since 2.8
> >  # @iscsi: Since 2.9
> > +# @rbd: Since 2.9
> >  #
> >  # Since: 2.0
> >  ##
> > @@ -2120,7 +2121,7 @@
> >              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> >              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> >              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > -            'vpc', 'vvfat' ] }
> > +            'vpc', 'vvfat', 'rbd' ] }
> >  
> >  ##
> >  # @BlockdevOptionsFile:
> > @@ -2376,7 +2377,6 @@
> >              'path': 'str',
> >              '*user': 'str' } }
> >  
> > -
> >  ##
> >  # @BlkdebugEvent:
> >  #
> > @@ -2666,6 +2666,47 @@
> >              '*timeout': 'int' } }
> >  
> >  ##
> > +# @BlockdevOptionsRbd:
> > +#
> > +# @pool:               Ceph pool name
> > +#
> > +# @image:              Image name in the Ceph pool
> > +#
> > +# @conf:               # optional path to Ceph configuration file.  Values
> > +#                      in the configuration file will be overridden by
> > +#                      options specified via QAPI.
> > +#
> > +# @snapshot:           #optional Ceph snapshot name
> > +#
> > +# @rbd-id:             #optional Ceph id name
> > +#
> > +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> > +#                       the password for the login.
> > +#
> > +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> > +#                      additional Ceph configuration, not including "id" or "conf"
> > +#                      options. This can be used to specify any of the options
> > +#                      that Ceph supports.  The format is of the form:
> > +#                           key1=value1:key2=value2:[...]
> > +#
> > +#                      Special characters such as ":" and "=" can be escaped
> > +#                      with a '\' character, which means the QAPI needs an
> > +#                      extra '\' character to pass the needed escape character.
> > +#                      For example:
> > +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> > +#
> 
> This is the key / value pair issue mentioned in the cover letter.  Encoding
> all the options as a string like this is ugly.  What is the preference on
> how to handle these via QAPI, when the actual key and value pairs could be
> anything?   Talking with Markus on IRC, one option he mentioned was an array
> of a generic struct of 'key' and 'value' pairs.
> 
> Do the libvirt folks have any interface preferences here?

IMHO, we should formally model each option that we need to be able to provide
and *not* provide any generic passthrough feature in QAPI.

Particularly for the server hostname/port, we should have the same QAPI
modelling approach that we did for other network protocols.


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 4/4] block/rbd: Add blockdev-add support

[Jeff Cody]

On Mon, Feb 27, 2017 at 09:31:21AM +0000, Daniel P. Berrange wrote:
> On Mon, Feb 27, 2017 at 02:36:13AM -0500, Jeff Cody wrote:
> > On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > ---
> > >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 44 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 5f82d35..08a1419 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -2111,6 +2111,7 @@
> > >  # @replication: Since 2.8
> > >  # @ssh: Since 2.8
> > >  # @iscsi: Since 2.9
> > > +# @rbd: Since 2.9
> > >  #
> > >  # Since: 2.0
> > >  ##
> > > @@ -2120,7 +2121,7 @@
> > >              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> > >              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> > >              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > > -            'vpc', 'vvfat' ] }
> > > +            'vpc', 'vvfat', 'rbd' ] }
> > >  
> > >  ##
> > >  # @BlockdevOptionsFile:
> > > @@ -2376,7 +2377,6 @@
> > >              'path': 'str',
> > >              '*user': 'str' } }
> > >  
> > > -
> > >  ##
> > >  # @BlkdebugEvent:
> > >  #
> > > @@ -2666,6 +2666,47 @@
> > >              '*timeout': 'int' } }
> > >  
> > >  ##
> > > +# @BlockdevOptionsRbd:
> > > +#
> > > +# @pool:               Ceph pool name
> > > +#
> > > +# @image:              Image name in the Ceph pool
> > > +#
> > > +# @conf:               # optional path to Ceph configuration file.  Values
> > > +#                      in the configuration file will be overridden by
> > > +#                      options specified via QAPI.
> > > +#
> > > +# @snapshot:           #optional Ceph snapshot name
> > > +#
> > > +# @rbd-id:             #optional Ceph id name
> > > +#
> > > +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> > > +#                       the password for the login.
> > > +#
> > > +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> > > +#                      additional Ceph configuration, not including "id" or "conf"
> > > +#                      options. This can be used to specify any of the options
> > > +#                      that Ceph supports.  The format is of the form:
> > > +#                           key1=value1:key2=value2:[...]
> > > +#
> > > +#                      Special characters such as ":" and "=" can be escaped
> > > +#                      with a '\' character, which means the QAPI needs an
> > > +#                      extra '\' character to pass the needed escape character.
> > > +#                      For example:
> > > +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> > > +#
> > 
> > This is the key / value pair issue mentioned in the cover letter.  Encoding
> > all the options as a string like this is ugly.  What is the preference on
> > how to handle these via QAPI, when the actual key and value pairs could be
> > anything?   Talking with Markus on IRC, one option he mentioned was an array
> > of a generic struct of 'key' and 'value' pairs.
> > 
> > Do the libvirt folks have any interface preferences here?
> 
> IMHO, we should formally model each option that we need to be able to provide
> and *not* provide any generic passthrough feature in QAPI.
> 
> Particularly for the server hostname/port, we should have the same QAPI
> modelling approach that we did for other network protocols.
> 
>

That is a sane position to take, but the problem is I really have no idea
all the options to include or not include here.

However, maybe it doesn't matter, at least for 2.9 - for the QAPI command,
we could drop the extra arguments completely (i.e., just drop the
keyvalue-pairs argument, above).  The extra options could still be set via a
config file passed in via 'conf', and in release > 2.9 we can gradually (or
not-so-gradually) add in additional options directly supported via QAPI.

The filename parsing would remain the same, for backwards compatibility, of
course.

Does this sound reasonable to you?


-Jeff

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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 08:18:59AM -0500, Jeff Cody wrote:
> On Mon, Feb 27, 2017 at 09:31:21AM +0000, Daniel P. Berrange wrote:
> > On Mon, Feb 27, 2017 at 02:36:13AM -0500, Jeff Cody wrote:
> > > On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > > ---
> > > >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 44 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index 5f82d35..08a1419 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -2111,6 +2111,7 @@
> > > >  # @replication: Since 2.8
> > > >  # @ssh: Since 2.8
> > > >  # @iscsi: Since 2.9
> > > > +# @rbd: Since 2.9
> > > >  #
> > > >  # Since: 2.0
> > > >  ##
> > > > @@ -2120,7 +2121,7 @@
> > > >              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> > > >              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> > > >              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > > > -            'vpc', 'vvfat' ] }
> > > > +            'vpc', 'vvfat', 'rbd' ] }
> > > >  
> > > >  ##
> > > >  # @BlockdevOptionsFile:
> > > > @@ -2376,7 +2377,6 @@
> > > >              'path': 'str',
> > > >              '*user': 'str' } }
> > > >  
> > > > -
> > > >  ##
> > > >  # @BlkdebugEvent:
> > > >  #
> > > > @@ -2666,6 +2666,47 @@
> > > >              '*timeout': 'int' } }
> > > >  
> > > >  ##
> > > > +# @BlockdevOptionsRbd:
> > > > +#
> > > > +# @pool:               Ceph pool name
> > > > +#
> > > > +# @image:              Image name in the Ceph pool
> > > > +#
> > > > +# @conf:               # optional path to Ceph configuration file.  Values
> > > > +#                      in the configuration file will be overridden by
> > > > +#                      options specified via QAPI.
> > > > +#
> > > > +# @snapshot:           #optional Ceph snapshot name
> > > > +#
> > > > +# @rbd-id:             #optional Ceph id name
> > > > +#
> > > > +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> > > > +#                       the password for the login.
> > > > +#
> > > > +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> > > > +#                      additional Ceph configuration, not including "id" or "conf"
> > > > +#                      options. This can be used to specify any of the options
> > > > +#                      that Ceph supports.  The format is of the form:
> > > > +#                           key1=value1:key2=value2:[...]
> > > > +#
> > > > +#                      Special characters such as ":" and "=" can be escaped
> > > > +#                      with a '\' character, which means the QAPI needs an
> > > > +#                      extra '\' character to pass the needed escape character.
> > > > +#                      For example:
> > > > +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> > > > +#
> > > 
> > > This is the key / value pair issue mentioned in the cover letter.  Encoding
> > > all the options as a string like this is ugly.  What is the preference on
> > > how to handle these via QAPI, when the actual key and value pairs could be
> > > anything?   Talking with Markus on IRC, one option he mentioned was an array
> > > of a generic struct of 'key' and 'value' pairs.
> > > 
> > > Do the libvirt folks have any interface preferences here?
> > 
> > IMHO, we should formally model each option that we need to be able to provide
> > and *not* provide any generic passthrough feature in QAPI.
> > 
> > Particularly for the server hostname/port, we should have the same QAPI
> > modelling approach that we did for other network protocols.
> > 
> >
> 
> That is a sane position to take, but the problem is I really have no idea
> all the options to include or not include here.

Libvirt relies on the following

 - id             - to provide the username
 - mon_host       - to provide the list of host+ports
 - auth_supported - to provide the list of authentication schemes to try
 - conf           - to proide the ceph config file


> However, maybe it doesn't matter, at least for 2.9 - for the QAPI command,
> we could drop the extra arguments completely (i.e., just drop the
> keyvalue-pairs argument, above).  The extra options could still be set via a
> config file passed in via 'conf', and in release > 2.9 we can gradually (or
> not-so-gradually) add in additional options directly supported via QAPI.
> 
> The filename parsing would remain the same, for backwards compatibility, of
> course.
> 
> Does this sound reasonable to you?

If we support the pieces libvirt needs, then I've no objection to dropping
the rest.

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 4/4] block/rbd: Add blockdev-add support

[Jeff Cody]

On Mon, Feb 27, 2017 at 01:30:46PM +0000, Daniel P. Berrange wrote:
> On Mon, Feb 27, 2017 at 08:18:59AM -0500, Jeff Cody wrote:
> > On Mon, Feb 27, 2017 at 09:31:21AM +0000, Daniel P. Berrange wrote:
> > > On Mon, Feb 27, 2017 at 02:36:13AM -0500, Jeff Cody wrote:
> > > > On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > > > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > > > ---
> > > > >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 44 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > index 5f82d35..08a1419 100644
> > > > > --- a/qapi/block-core.json
> > > > > +++ b/qapi/block-core.json
> > > > > @@ -2111,6 +2111,7 @@
> > > > >  # @replication: Since 2.8
> > > > >  # @ssh: Since 2.8
> > > > >  # @iscsi: Since 2.9
> > > > > +# @rbd: Since 2.9
> > > > >  #
> > > > >  # Since: 2.0
> > > > >  ##
> > > > > @@ -2120,7 +2121,7 @@
> > > > >              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> > > > >              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> > > > >              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > > > > -            'vpc', 'vvfat' ] }
> > > > > +            'vpc', 'vvfat', 'rbd' ] }
> > > > >  
> > > > >  ##
> > > > >  # @BlockdevOptionsFile:
> > > > > @@ -2376,7 +2377,6 @@
> > > > >              'path': 'str',
> > > > >              '*user': 'str' } }
> > > > >  
> > > > > -
> > > > >  ##
> > > > >  # @BlkdebugEvent:
> > > > >  #
> > > > > @@ -2666,6 +2666,47 @@
> > > > >              '*timeout': 'int' } }
> > > > >  
> > > > >  ##
> > > > > +# @BlockdevOptionsRbd:
> > > > > +#
> > > > > +# @pool:               Ceph pool name
> > > > > +#
> > > > > +# @image:              Image name in the Ceph pool
> > > > > +#
> > > > > +# @conf:               # optional path to Ceph configuration file.  Values
> > > > > +#                      in the configuration file will be overridden by
> > > > > +#                      options specified via QAPI.
> > > > > +#
> > > > > +# @snapshot:           #optional Ceph snapshot name
> > > > > +#
> > > > > +# @rbd-id:             #optional Ceph id name
> > > > > +#
> > > > > +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> > > > > +#                       the password for the login.
> > > > > +#
> > > > > +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> > > > > +#                      additional Ceph configuration, not including "id" or "conf"
> > > > > +#                      options. This can be used to specify any of the options
> > > > > +#                      that Ceph supports.  The format is of the form:
> > > > > +#                           key1=value1:key2=value2:[...]
> > > > > +#
> > > > > +#                      Special characters such as ":" and "=" can be escaped
> > > > > +#                      with a '\' character, which means the QAPI needs an
> > > > > +#                      extra '\' character to pass the needed escape character.
> > > > > +#                      For example:
> > > > > +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"
> > > > > +#
> > > > 
> > > > This is the key / value pair issue mentioned in the cover letter.  Encoding
> > > > all the options as a string like this is ugly.  What is the preference on
> > > > how to handle these via QAPI, when the actual key and value pairs could be
> > > > anything?   Talking with Markus on IRC, one option he mentioned was an array
> > > > of a generic struct of 'key' and 'value' pairs.
> > > > 
> > > > Do the libvirt folks have any interface preferences here?
> > > 
> > > IMHO, we should formally model each option that we need to be able to provide
> > > and *not* provide any generic passthrough feature in QAPI.
> > > 
> > > Particularly for the server hostname/port, we should have the same QAPI
> > > modelling approach that we did for other network protocols.
> > > 
> > >
> > 
> > That is a sane position to take, but the problem is I really have no idea
> > all the options to include or not include here.
> 
> Libvirt relies on the following
> 
>  - id             - to provide the username
>  - mon_host       - to provide the list of host+ports
>  - auth_supported - to provide the list of authentication schemes to try
>  - conf           - to proide the ceph config file
> 
> 
> > However, maybe it doesn't matter, at least for 2.9 - for the QAPI command,
> > we could drop the extra arguments completely (i.e., just drop the
> > keyvalue-pairs argument, above).  The extra options could still be set via a
> > config file passed in via 'conf', and in release > 2.9 we can gradually (or
> > not-so-gradually) add in additional options directly supported via QAPI.
> > 
> > The filename parsing would remain the same, for backwards compatibility, of
> > course.
> > 
> > Does this sound reasonable to you?
> 
> If we support the pieces libvirt needs, then I've no objection to dropping
> the rest.
> 

I'll add in mon_host and auth_supported, then.  Then we'll rely on the
config file for unlisted options, and revisit new options in later releases.

Thanks,
Jeff

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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5f82d35..08a1419 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2111,6 +2111,7 @@
>  # @replication: Since 2.8
>  # @ssh: Since 2.8
>  # @iscsi: Since 2.9
> +# @rbd: Since 2.9
>  #
>  # Since: 2.0
>  ##
> @@ -2120,7 +2121,7 @@
>              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
>              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
>              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> -            'vpc', 'vvfat' ] }
> +            'vpc', 'vvfat', 'rbd' ] }
>  
>  ##
>  # @BlockdevOptionsFile:
> @@ -2376,7 +2377,6 @@
>              'path': 'str',
>              '*user': 'str' } }
>  
> -
>  ##
>  # @BlkdebugEvent:
>  #
> @@ -2666,6 +2666,47 @@
>              '*timeout': 'int' } }
>  
>  ##
> +# @BlockdevOptionsRbd:
> +#
> +# @pool:               Ceph pool name
> +#
> +# @image:              Image name in the Ceph pool
> +#
> +# @conf:               # optional path to Ceph configuration file.  Values
> +#                      in the configuration file will be overridden by
> +#                      options specified via QAPI.
> +#
> +# @snapshot:           #optional Ceph snapshot name
> +#
> +# @rbd-id:             #optional Ceph id name

BTW, I think I'd suggest 'user' or 'username' for this, since that is the more
common terminology we seem to use for other block drivers

> +#
> +# @password-secret:    #optional The ID of a QCryptoSecret object providing
> +#                       the password for the login.
> +#
> +# @keyvalue-pairs:     #optional  string containing key/value pairs for
> +#                      additional Ceph configuration, not including "id" or "conf"
> +#                      options. This can be used to specify any of the options
> +#                      that Ceph supports.  The format is of the form:
> +#                           key1=value1:key2=value2:[...]
> +#
> +#                      Special characters such as ":" and "=" can be escaped
> +#                      with a '\' character, which means the QAPI needs an
> +#                      extra '\' character to pass the needed escape character.
> +#                      For example:
> +#                            "keyvalue-pairs": "mon_host=127.0.0.1\\:6321"

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 4/4] block/rbd: Add blockdev-add support

[Jeff Cody]

On Mon, Feb 27, 2017 at 01:45:47PM +0000, Daniel P. Berrange wrote:
> On Mon, Feb 27, 2017 at 02:30:41AM -0500, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 5f82d35..08a1419 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2111,6 +2111,7 @@
> >  # @replication: Since 2.8
> >  # @ssh: Since 2.8
> >  # @iscsi: Since 2.9
> > +# @rbd: Since 2.9
> >  #
> >  # Since: 2.0
> >  ##
> > @@ -2120,7 +2121,7 @@
> >              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
> >              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
> >              'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
> > -            'vpc', 'vvfat' ] }
> > +            'vpc', 'vvfat', 'rbd' ] }
> >  
> >  ##
> >  # @BlockdevOptionsFile:
> > @@ -2376,7 +2377,6 @@
> >              'path': 'str',
> >              '*user': 'str' } }
> >  
> > -
> >  ##
> >  # @BlkdebugEvent:
> >  #
> > @@ -2666,6 +2666,47 @@
> >              '*timeout': 'int' } }
> >  
> >  ##
> > +# @BlockdevOptionsRbd:
> > +#
> > +# @pool:               Ceph pool name
> > +#
> > +# @image:              Image name in the Ceph pool
> > +#
> > +# @conf:               # optional path to Ceph configuration file.  Values
> > +#                      in the configuration file will be overridden by
> > +#                      options specified via QAPI.
> > +#
> > +# @snapshot:           #optional Ceph snapshot name
> > +#
> > +# @rbd-id:             #optional Ceph id name
> 
> BTW, I think I'd suggest 'user' or 'username' for this, since that is the more
> common terminology we seem to use for other block drivers
>

OK, I will go with 'user' instead of 'rbd-id'.

I think that fits with the usage terminology in rados_create()
documentation as well:

    int rados_create(rados_t * cluster, const char *const id)

    [...]

    Parameters
       * cluster: where to store the handle
       * id: the user to connect as (i.e. admin, not client.admin)

-Jeff