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

Jeff Cody <jcody(a)redhat.com&gt; 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(a)redhat.com&gt; > Signed-off-by: Jeff Cody <jcody(a)redhat.com&gt; > --- > 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