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

[Jeff Cody]

On Sat, Sep 22, 2018 at 08:18:26AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody at redhat.com> 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 <eblake at redhat.com>
> > Signed-off-by: Jeff Cody <jcody at redhat.com>
> > ---
> >  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?  ;)

Eric noticed this as well :)

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

Since the two requested changes are comments only and minor, and a PR has
already been sent, I went ahead and updated the patch and will send a v2
PR with these patches.  I left the r-b for this patch untouched.

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