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

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> 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.
>
> We do not support mixed modern usage alongside legacy keyvalue pair
> usage.
>
> A deprecation warning has been added, although care should be taken
> when actually deprecating since the impact is not limited to
> commandline or qapi usage, but also opening existing images.
>
> Reviewed-by: Eric Blake <address@hidden>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/rbd.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index b199450f9f..5090e4f662 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
> BlockdevOptionsRbd **opts,
>      return 0;
>  }
>  
> +static int qemu_rbd_attempt_legacy_options(QDict *options,
> +                                           BlockdevOptionsRbd **opts,
> +                                           char **keypairs)
> +{
> +    char *filename;
> +    int r;
> +
> +    filename = g_strdup(qdict_get_try_str(options, "filename"));
> +    if (!filename) {
> +        return -EINVAL;
> +    }
> +    qdict_del(options, "filename");
> +
> +    qemu_rbd_parse_filename(filename, options, NULL);
> +
> +    /* keypairs freed by caller */
> +    *keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +    if (*keypairs) {
> +        qdict_del(options, "=keyvalue-pairs");
> +    }
> +
> +    r = qemu_rbd_convert_options(options, opts, NULL);
> +
> +    g_free(filename);
> +    return r;
> +}
> +
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>                           Error **errp)
>  {
> @@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>      r = qemu_rbd_convert_options(options, &opts, &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto out;
> +        /* If keypairs are present, that means some options are present in
> +         * the modern option format.  Don't attempt to parse legacy option
> +         * formats, as we won't support mixed usage. */
> +        if (keypairs) {
> +            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. */
> +        r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
> +        if (r < 0) {
> +            error_propagate(errp, local_err);
> +            goto out;

This reports the error from qemu_rbd_convert_options(), as you commit
message explains.  Would explaining it in a comment help future readers?

> +        }
> +        /* Take care whenever deciding to actually deprecate; once this 
> ability
> +         * is removed, we will not be able to open any images with 
> legacy-styled
> +         * backing image strings. */
> +        error_report("RBD options encoded in the filename as keyvalue pairs "
> +                     "is deprecated.  Future versions may cease to parse "
> +                     "these options in the future.");

"Future versions may ... in the future": you're serious about this
happening only in the future, aren't you?  ;)

Quote error_report()'s contract: "The resulting message should be a
single phrase, with no newline or trailing punctuation."

Let's scratch everything from the first period on.  

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