[Qemu-devel] [PATCH 0/2] block/rbd: enable filename parsing on open

[Qemu-devel] [PATCH 0/2] block/rbd: enable filename parsing on open

[Jeff Cody]

This series enables filename parsing on open, on the old key/value pair.
Recent changes to new option formats for rbd broke some images.  See
Patch 2 for more details.

Jeff Cody (2):
  block/rbd: pull out qemu_rbd_convert_options
  block/rbd: Attempt to parse legacy filenames

 block/rbd.c | 66 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 15 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options

[Jeff Cody]

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

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

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..a8e79d01d2 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
     return r;
 }
 
+static int qemu_rbd_convert_options(BlockDriverState *bs, QDict *options,
+                                    BlockdevOptionsRbd **opts, Error **errp)
+{
+    Visitor *v;
+    Error *local_err = NULL;
+
+    /* Convert the remaining options into a QAPI object */
+    v = qobject_input_visitor_new_flat_confused(options, errp);
+    if (!v) {
+        return -EINVAL;
+    }
+
+    visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+    visit_free(v);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     BlockdevOptionsRbd *opts = NULL;
-    Visitor *v;
     const QDictEntry *e;
     Error *local_err = NULL;
     char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         qdict_del(options, "password-secret");
     }
 
-    /* Convert the remaining options into a QAPI object */
-    v = qobject_input_visitor_new_flat_confused(options, errp);
-    if (!v) {
-        r = -EINVAL;
-        goto out;
-    }
-
-    visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-    visit_free(v);
-
+    r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        r = -EINVAL;
         goto out;
     }
 
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

This approach has a potential drawback: if for some reason there are
some options supplied the new way, and some the old way, we may not
catch all the old options if they are not required options (since it
won't cause the initial failure).

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

diff --git a/block/rbd.c b/block/rbd.c
index a8e79d01d2..bce86b8bde 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     BlockdevOptionsRbd *opts = NULL;
     const QDictEntry *e;
     Error *local_err = NULL;
-    char *keypairs, *secretid;
+    char *keypairs, *secretid, *filename;
     int r;
 
     keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
     if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
+        /* If the initial attempt to convert and process the options failed,
+         * we may be attempting to open an image file that has the rbd options
+         * specified in the older format consisting of all key/value pairs
+         * encoded in the filename.  Go ahead and attempt to parse the
+         * filename, and see if we can pull out the required options */
+        Error *parse_err = NULL;
+
+        filename = g_strdup(qdict_get_try_str(options, "filename"));
+        qdict_del(options, "filename");
+
+        qemu_rbd_parse_filename(filename, options, NULL);
+
+        g_free(keypairs);
+        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+        if (keypairs) {
+            qdict_del(options, "=keyvalue-pairs");
+        }
+
+        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
+        if (parse_err) {
+            /* if the second attempt failed, pass along the original error
+             * message for the current format */
+            error_propagate(errp, local_err);
+            error_free(parse_err);
+            goto out;
+        }
     }
 
     /* Remove the processed options from the QDict (the visitor processes
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options

[Eric Blake]

On 9/11/18 12:15 AM, Jeff Cody wrote:
> Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
> into a helper function.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/rbd.c | 36 ++++++++++++++++++++++++------------
>   1 file changed, 24 insertions(+), 12 deletions(-)
> 

> -
> +    r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        r = -EINVAL;
>           goto out;
>       }

You could simplify this to:

r = qemu_rbd_convert_options(bs, options, &opts, errp);
if (r < 0) {
     goto out;
}

Either way, this refactoring looks correct.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 01:15:49AM -0400, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
> 
> This leaves a bit of an ugly conundrum, and a hacky solution.
> 
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
> 
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      BlockdevOptionsRbd *opts = NULL;
>      const QDictEntry *e;
>      Error *local_err = NULL;
> -    char *keypairs, *secretid;
> +    char *keypairs, *secretid, *filename;
>      int r;
>  
>      keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto out;
> +        /* If the initial attempt to convert and process the options failed,
> +         * we may be attempting to open an image file that has the rbd options
> +         * specified in the older format consisting of all key/value pairs
> +         * encoded in the filename.  Go ahead and attempt to parse the
> +         * filename, and see if we can pull out the required options */
> +        Error *parse_err = NULL;
> +
> +        filename = g_strdup(qdict_get_try_str(options, "filename"));

I'm leaking filename, I'll clean that up (along with any other comments) in
v2.

> +        qdict_del(options, "filename");
> +
> +        qemu_rbd_parse_filename(filename, options, NULL);
> +
> +        g_free(keypairs);
> +        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +        if (keypairs) {
> +            qdict_del(options, "=keyvalue-pairs");
> +        }
> +
> +        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +        if (parse_err) {
> +            /* if the second attempt failed, pass along the original error
> +             * message for the current format */
> +            error_propagate(errp, local_err);
> +            error_free(parse_err);
> +            goto out;
> +        }
>      }
>  
>      /* Remove the processed options from the QDict (the visitor processes
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Eric Blake]

On 9/11/18 12:15 AM, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
> 
> This leaves a bit of an ugly conundrum, and a hacky solution.
> 
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
> 
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).

No one should be mixing new and old, though.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/rbd.c | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>       BlockdevOptionsRbd *opts = NULL;
>       const QDictEntry *e;
>       Error *local_err = NULL;
> -    char *keypairs, *secretid;
> +    char *keypairs, *secretid, *filename;
>       int r;
>   
>       keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>   
>       r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>       if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto out;

Oh, my comment about simplifying this in 1/2 is probably moot, now that 
you are doing a lot more based on local_err rather than just blindly 
propagating it.

> +        /* If the initial attempt to convert and process the options failed,
> +         * we may be attempting to open an image file that has the rbd options
> +         * specified in the older format consisting of all key/value pairs
> +         * encoded in the filename.  Go ahead and attempt to parse the
> +         * filename, and see if we can pull out the required options */
> +        Error *parse_err = NULL;
> +
> +        filename = g_strdup(qdict_get_try_str(options, "filename"));

You already spotted your leak.

> +        qdict_del(options, "filename");
> +
> +        qemu_rbd_parse_filename(filename, options, NULL);
> +
> +        g_free(keypairs);

Wait. Why are you unilaterally freeing any previously-parsed keypairs in 
favor of the ones parsed out of the filename?  I'd rather that we insist 
on only old behavior, or only new, and not some mix.  Thus, if we 
already detected keypairs at all, we should declare this situation as an 
error, rather than throwing them away.

> +        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +        if (keypairs) {
> +            qdict_del(options, "=keyvalue-pairs");
> +        }
> +
> +        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +        if (parse_err) {
> +            /* if the second attempt failed, pass along the original error
> +             * message for the current format */
> +            error_propagate(errp, local_err);
> +            error_free(parse_err);
> +            goto out;
> +        }
>       }

The idea of trying two parses makes sense, but I'm hoping v2 better 
handles the case of detecting bad attempts to mix-and-match behavior. 
Furthermore, is there an iotests that you can modify (or add) as a 
regression test for this working the way we want?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options

[John Snow]

On 09/11/2018 01:15 AM, Jeff Cody wrote:
> Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
> into a helper function.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index ca8e5bbace..a8e79d01d2 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -655,12 +655,34 @@ failed_opts:
>      return r;
>  }
>  
> +static int qemu_rbd_convert_options(BlockDriverState *bs, QDict *options,
> +                                    BlockdevOptionsRbd **opts, Error **errp)
> +{
> +    Visitor *v;
> +    Error *local_err = NULL;
> +
> +    /* Convert the remaining options into a QAPI object */
> +    v = qobject_input_visitor_new_flat_confused(options, errp);
> +    if (!v) {
> +        return -EINVAL;
> +    }
> +
> +    visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
> +    visit_free(v);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>                           Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      BlockdevOptionsRbd *opts = NULL;
> -    Visitor *v;
>      const QDictEntry *e;
>      Error *local_err = NULL;
>      char *keypairs, *secretid;
> @@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          qdict_del(options, "password-secret");
>      }
>  
> -    /* Convert the remaining options into a QAPI object */
> -    v = qobject_input_visitor_new_flat_confused(options, errp);
> -    if (!v) {
> -        r = -EINVAL;
> -        goto out;
> -    }
> -
> -    visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
> -    visit_free(v);
> -
> +    r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        r = -EINVAL;
>          goto out;
>      }
>  
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[John Snow]

On 09/11/2018 01:15 AM, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
> 
> This leaves a bit of an ugly conundrum, and a hacky solution.
> 
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
> 
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      BlockdevOptionsRbd *opts = NULL;
>      const QDictEntry *e;
>      Error *local_err = NULL;
> -    char *keypairs, *secretid;
> +    char *keypairs, *secretid, *filename;
>      int r;
>  
>      keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto out;
> +        /* If the initial attempt to convert and process the options failed,
> +         * we may be attempting to open an image file that has the rbd options
> +         * specified in the older format consisting of all key/value pairs
> +         * encoded in the filename.  Go ahead and attempt to parse the
> +         * filename, and see if we can pull out the required options */

Is it worth splitting out the legacy parsing routine here into its own
function, given that we will generally depend on it not being invoked?
i.e., for readability, it doesn't need to distract us.

> +        Error *parse_err = NULL;
> +
> +        filename = g_strdup(qdict_get_try_str(options, "filename"));
> +        qdict_del(options, "filename");
> +
> +        qemu_rbd_parse_filename(filename, options, NULL);
> +
> +        g_free(keypairs);

As Eric already noticed, better to just return with error if this is
already set.

> +        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +        if (keypairs) {
> +            qdict_del(options, "=keyvalue-pairs");
> +        }
> +
> +        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +        if (parse_err) {
> +            /* if the second attempt failed, pass along the original error
> +             * message for the current format */
> +            error_propagate(errp, local_err);
> +            error_free(parse_err);
> +            goto out;
> +        }

If it does succeed, though, ought we emit a deprecated warning that the
old specification syntax is not supported?

Once we load the image, will the header get rewritten into a compliant
format?

--js

>      }
>  
>      /* Remove the processed options from the QDict (the visitor processes
> 

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 01:03:44PM -0500, Eric Blake wrote:
> On 9/11/18 12:15 AM, Jeff Cody wrote:
> >When we converted rbd to get rid of the older key/value-centric
> >encoding format, we broke compatibility with image files with backing
> >file strings encoded in the old format.
> >
> >This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> >If the initial attempt to parse the "proper" options fails, it assumes
> >that we may have an older key/value encoded filename.  Fall back to
> >attempting to parse the filename, and extract the required options from
> >it.  If that fails, pass along the original error message.
> >
> >This approach has a potential drawback: if for some reason there are
> >some options supplied the new way, and some the old way, we may not
> >catch all the old options if they are not required options (since it
> >won't cause the initial failure).
> 
> No one should be mixing new and old, though.
> 
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >---
> >  block/rbd.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index a8e79d01d2..bce86b8bde 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >      BlockdevOptionsRbd *opts = NULL;
> >      const QDictEntry *e;
> >      Error *local_err = NULL;
> >-    char *keypairs, *secretid;
> >+    char *keypairs, *secretid, *filename;
> >      int r;
> >      keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >      r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> >      if (local_err) {
> >-        error_propagate(errp, local_err);
> >-        goto out;
> 
> Oh, my comment about simplifying this in 1/2 is probably moot, now that you
> are doing a lot more based on local_err rather than just blindly propagating
> it.
> 
> >+        /* If the initial attempt to convert and process the options failed,
> >+         * we may be attempting to open an image file that has the rbd options
> >+         * specified in the older format consisting of all key/value pairs
> >+         * encoded in the filename.  Go ahead and attempt to parse the
> >+         * filename, and see if we can pull out the required options */
> >+        Error *parse_err = NULL;
> >+
> >+        filename = g_strdup(qdict_get_try_str(options, "filename"));
> 
> You already spotted your leak.
> 
> >+        qdict_del(options, "filename");
> >+
> >+        qemu_rbd_parse_filename(filename, options, NULL);
> >+
> >+        g_free(keypairs);
> 
> Wait. Why are you unilaterally freeing any previously-parsed keypairs in
> favor of the ones parsed out of the filename?  I'd rather that we insist on
> only old behavior, or only new, and not some mix.  Thus, if we already
> detected keypairs at all, we should declare this situation as an error,
> rather than throwing them away.
> 

Good point.  I'll flag (local_err && keypairs) as an error.

I also just realized I need to check for NULL filename, and error out as
well in that case.

> >+        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >+        if (keypairs) {
> >+            qdict_del(options, "=keyvalue-pairs");
> >+        }
> >+
> >+        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> >+        if (parse_err) {
> >+            /* if the second attempt failed, pass along the original error
> >+             * message for the current format */
> >+            error_propagate(errp, local_err);
> >+            error_free(parse_err);
> >+            goto out;
> >+        }
> >      }
> 
> The idea of trying two parses makes sense, but I'm hoping v2 better handles
> the case of detecting bad attempts to mix-and-match behavior. Furthermore,
> is there an iotests that you can modify (or add) as a regression test for
> this working the way we want?

Hmm... yes, that should be doable.  I'm not sure if I can do it without the
'success' path being a timeout when there is no ceph server present, though.
I'll see what I can do.

-Jeff

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
> 
> 
> On 09/11/2018 01:15 AM, Jeff Cody wrote:
> > When we converted rbd to get rid of the older key/value-centric
> > encoding format, we broke compatibility with image files with backing
> > file strings encoded in the old format.
> > 
> > This leaves a bit of an ugly conundrum, and a hacky solution.
> > 
> > If the initial attempt to parse the "proper" options fails, it assumes
> > that we may have an older key/value encoded filename.  Fall back to
> > attempting to parse the filename, and extract the required options from
> > it.  If that fails, pass along the original error message.
> > 
> > This approach has a potential drawback: if for some reason there are
> > some options supplied the new way, and some the old way, we may not
> > catch all the old options if they are not required options (since it
> > won't cause the initial failure).
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/rbd.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index a8e79d01d2..bce86b8bde 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >      BlockdevOptionsRbd *opts = NULL;
> >      const QDictEntry *e;
> >      Error *local_err = NULL;
> > -    char *keypairs, *secretid;
> > +    char *keypairs, *secretid, *filename;
> >      int r;
> >  
> >      keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> >      if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        goto out;
> > +        /* If the initial attempt to convert and process the options failed,
> > +         * we may be attempting to open an image file that has the rbd options
> > +         * specified in the older format consisting of all key/value pairs
> > +         * encoded in the filename.  Go ahead and attempt to parse the
> > +         * filename, and see if we can pull out the required options */
> 
> Is it worth splitting out the legacy parsing routine here into its own
> function, given that we will generally depend on it not being invoked?
> i.e., for readability, it doesn't need to distract us.
> 

Yeah, that would probably be good.

> > +        Error *parse_err = NULL;
> > +
> > +        filename = g_strdup(qdict_get_try_str(options, "filename"));
> > +        qdict_del(options, "filename");
> > +
> > +        qemu_rbd_parse_filename(filename, options, NULL);
> > +
> > +        g_free(keypairs);
> 
> As Eric already noticed, better to just return with error if this is
> already set.
> 
> > +        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > +        if (keypairs) {
> > +            qdict_del(options, "=keyvalue-pairs");
> > +        }
> > +
> > +        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> > +        if (parse_err) {
> > +            /* if the second attempt failed, pass along the original error
> > +             * message for the current format */
> > +            error_propagate(errp, local_err);
> > +            error_free(parse_err);
> > +            goto out;
> > +        }
> 
> If it does succeed, though, ought we emit a deprecated warning that the
> old specification syntax is not supported?
>

I don't know.  Without this support, we can't open some existing images.  At
what point would we actually remove that support?

> Once we load the image, will the header get rewritten into a compliant
> format?
> 

Hmm - I think in some code paths, but not all.  I don't think the answer is
'yes' universally, alas.

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[John Snow]

On 09/11/2018 02:37 PM, Jeff Cody wrote:
> On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
>>
>>
>> On 09/11/2018 01:15 AM, Jeff Cody wrote:
>>> When we converted rbd to get rid of the older key/value-centric
>>> encoding format, we broke compatibility with image files with backing
>>> file strings encoded in the old format.
>>>
>>> This leaves a bit of an ugly conundrum, and a hacky solution.
>>>
>>> If the initial attempt to parse the "proper" options fails, it assumes
>>> that we may have an older key/value encoded filename.  Fall back to
>>> attempting to parse the filename, and extract the required options from
>>> it.  If that fails, pass along the original error message.
>>>
>>> This approach has a potential drawback: if for some reason there are
>>> some options supplied the new way, and some the old way, we may not
>>> catch all the old options if they are not required options (since it
>>> won't cause the initial failure).
>>>
>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>> ---
>>>  block/rbd.c | 30 +++++++++++++++++++++++++++---
>>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/rbd.c b/block/rbd.c
>>> index a8e79d01d2..bce86b8bde 100644
>>> --- a/block/rbd.c
>>> +++ b/block/rbd.c
>>> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>>      BlockdevOptionsRbd *opts = NULL;
>>>      const QDictEntry *e;
>>>      Error *local_err = NULL;
>>> -    char *keypairs, *secretid;
>>> +    char *keypairs, *secretid, *filename;
>>>      int r;
>>>  
>>>      keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>>> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>>  
>>>      r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>>>      if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        goto out;
>>> +        /* If the initial attempt to convert and process the options failed,
>>> +         * we may be attempting to open an image file that has the rbd options
>>> +         * specified in the older format consisting of all key/value pairs
>>> +         * encoded in the filename.  Go ahead and attempt to parse the
>>> +         * filename, and see if we can pull out the required options */
>>
>> Is it worth splitting out the legacy parsing routine here into its own
>> function, given that we will generally depend on it not being invoked?
>> i.e., for readability, it doesn't need to distract us.
>>
> 
> Yeah, that would probably be good.
> 
>>> +        Error *parse_err = NULL;
>>> +
>>> +        filename = g_strdup(qdict_get_try_str(options, "filename"));
>>> +        qdict_del(options, "filename");
>>> +
>>> +        qemu_rbd_parse_filename(filename, options, NULL);
>>> +
>>> +        g_free(keypairs);
>>
>> As Eric already noticed, better to just return with error if this is
>> already set.
>>
>>> +        keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>>> +        if (keypairs) {
>>> +            qdict_del(options, "=keyvalue-pairs");
>>> +        }
>>> +
>>> +        r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
>>> +        if (parse_err) {
>>> +            /* if the second attempt failed, pass along the original error
>>> +             * message for the current format */
>>> +            error_propagate(errp, local_err);
>>> +            error_free(parse_err);
>>> +            goto out;
>>> +        }
>>
>> If it does succeed, though, ought we emit a deprecated warning that the
>> old specification syntax is not supported?
>>
> 
> I don't know.  Without this support, we can't open some existing images.  At
> what point would we actually remove that support?
> 

Yeah, probably never -- but there are some versions of QEMU in the wild
that now simply don't support this format, so some urging to switch is
probably not harmful was my thought.

(At least, that was my read. When did we switch RBD formats? Just for
3.1? In that case, no warning is necessary.)

>> Once we load the image, will the header get rewritten into a compliant
>> format?
>>
> 
> Hmm - I think in some code paths, but not all.  I don't think the answer is
> 'yes' universally, alas.
> 

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Kevin Wolf]

Am 11.09.2018 um 20:37 hat Jeff Cody geschrieben:
> On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
> > Once we load the image, will the header get rewritten into a compliant
> > format?
> 
> Hmm - I think in some code paths, but not all.  I don't think the answer is
> 'yes' universally, alas.

Can't we explicitly call BdrvChildRole.update_filename() for all parents
when we open a legacy filename? We'd just need to add the callback to
child_file, which would propagate it to the parents of the format layer,
and then just opening the image with a legacy backing file link once
would fix the problem for this image.

Kevin

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Wed, Sep 12, 2018 at 12:38:56PM +0200, Kevin Wolf wrote:
> Am 11.09.2018 um 20:37 hat Jeff Cody geschrieben:
> > On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
> > > Once we load the image, will the header get rewritten into a compliant
> > > format?
> > 
> > Hmm - I think in some code paths, but not all.  I don't think the answer is
> > 'yes' universally, alas.
> 
> Can't we explicitly call BdrvChildRole.update_filename() for all parents
> when we open a legacy filename? We'd just need to add the callback to
> child_file, which would propagate it to the parents of the format layer,
> and then just opening the image with a legacy backing file link once
> would fix the problem for this image.
>

Yes, that is a good idea.  I will spin a v5 with that added.

Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Wed, Sep 12, 2018 at 08:42:15AM -0400, Jeff Cody wrote:
> On Wed, Sep 12, 2018 at 12:38:56PM +0200, Kevin Wolf wrote:
> > Am 11.09.2018 um 20:37 hat Jeff Cody geschrieben:
> > > On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
> > > > Once we load the image, will the header get rewritten into a compliant
> > > > format?
> > > 
> > > Hmm - I think in some code paths, but not all.  I don't think the answer is
> > > 'yes' universally, alas.
> > 
> > Can't we explicitly call BdrvChildRole.update_filename() for all parents
> > when we open a legacy filename? We'd just need to add the callback to
> > child_file, which would propagate it to the parents of the format layer,
> > and then just opening the image with a legacy backing file link once
> > would fix the problem for this image.
> >
> 
> Yes, that is a good idea.  I will spin a v5 with that added.

On second thought; how about we address updating the legacy filename header
in a separate patch series?

-Jeff