On Sat, Sep 22, 2018 at 08:18:26AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Jeff Cody <jcody@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
