| Message ID | 15b332e5432ad069441f7275a46080f465d789a0.1536704901.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/rbd: enable filename parsing on open | expand |
Jeff Cody <jcody@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@redhat.com> > Signed-off-by: Jeff Cody <jcody@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? ;) 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
On Sat, Sep 22, 2018 at 08:18:26AM +0200, Markus Armbruster wrote: > Jeff Cody <jcody@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@redhat.com> > > Signed-off-by: Jeff Cody <jcody@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
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; + } + /* 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."); } /* Remove the processed options from the QDict (the visitor processes