[Qemu-devel] [PATCH v2 0/3] block/rbd: enable filename parsing on open

[Qemu-devel] [PATCH v2 0/3] block/rbd: enable filename parsing on open

[Jeff Cody]

Changes from v1:

Patch 1: Don't pass unused BlockDriverState to helper function

Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
         Add deprecation warning [John]
         Pull legacy parsing code into function [John]
         Fixed filename leak

Patch 3: New; iotest 231. [Eric]


iotest failure on current master:

 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.  Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': error connecting: No such file or directory
+qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': Parameter 'pool' is missing
 unable to get monitor info from DNS SRV with service name: ceph-mon
 no monitors specified to connect to.
 qemu-img: Could not open 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}': error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests



Jeff Cody (3):
  block/rbd: pull out qemu_rbd_convert_options
  block/rbd: Attempt to parse legacy filenames
  block/rbd: add iotest for rbd legacy keyvalue filename parsing

 block/rbd.c                | 89 ++++++++++++++++++++++++++++++++------
 tests/qemu-iotests/231     | 62 ++++++++++++++++++++++++++
 tests/qemu-iotests/231.out |  9 ++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 147 insertions(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/3] block/rbd: pull out qemu_rbd_convert_options

[Jeff Cody]

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
     return r;
 }
 
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+                                    Error **errp)
+{
+    Visitor *v;
+    Error *local_err = NULL;
+
+    /* Convert the remaining options into a QAPI object */
+    v = qobject_input_visitor_new_flat_confused(options, errp);
+    if (!v) {
+        return -EINVAL;
+    }
+
+    visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+    visit_free(v);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     BlockdevOptionsRbd *opts = NULL;
-    Visitor *v;
     const QDictEntry *e;
     Error *local_err = NULL;
     char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         qdict_del(options, "password-secret");
     }
 
-    /* Convert the remaining options into a QAPI object */
-    v = qobject_input_visitor_new_flat_confused(options, errp);
-    if (!v) {
-        r = -EINVAL;
-        goto out;
-    }
-
-    visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-    visit_free(v);
-
+    r = qemu_rbd_convert_options(options, &opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        r = -EINVAL;
         goto out;
     }
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

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.

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;
+        }
+        /* 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
-- 
2.17.1

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

[Eric Blake]

On 9/11/18 3:43 PM, Jeff Cody wrote:
> 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.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/rbd.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 51 insertions(+), 2 deletions(-)

Where's the patch to qemu/qemu-deprecated.texi to mention the new 
message and our advice on upgrading images to avoid triggering it?

> 
> 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);

Intentionally ignoring errors here,

> +
> +    /* 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);

and here. I guess we'll see how the caller is expecting things to behave.

> +
> +    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.");

Okay, so you're making a best-effort fallback to scrape out any 
remaining keyvalue pairs. If there was no filename (and hence no 
keyvalue pairs populated), we know up front that we lack enough 
information, so attempt_legacy_options() returns failure right away; if 
there was a filename, we don't know if we got all the required options 
(so ignoring errors was okay) - that will be up to later code to 
decipher, after we emit our warning that we already relied on legacy 
options (if the later code has all it needed, then only the warning is 
emitted; if later code fails, the user now has the warning in addition 
to the later failure message to help them resolve their images).

A bit confusing how it all fits together, but it seems to work out in 
the end.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Jeff Cody]

On Tue, Sep 11, 2018 at 04:05:45PM -0500, Eric Blake wrote:
> On 9/11/18 3:43 PM, Jeff Cody wrote:
> >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.
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >---
> >  block/rbd.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> Where's the patch to qemu/qemu-deprecated.texi to mention the new message
> and our advice on upgrading images to avoid triggering it?
> 

Argh, thanks - forgot that.  I'll spin a v3 with that as patch 4.

> >
> >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);
> 
> Intentionally ignoring errors here,
> 
> >+
> >+    /* 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);
> 
> and here. I guess we'll see how the caller is expecting things to behave.
> 
> >+
> >+    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.");
> 
> Okay, so you're making a best-effort fallback to scrape out any remaining
> keyvalue pairs. If there was no filename (and hence no keyvalue pairs
> populated), we know up front that we lack enough information, so
> attempt_legacy_options() returns failure right away; if there was a
> filename, we don't know if we got all the required options (so ignoring
> errors was okay) - that will be up to later code to decipher, after we emit
> our warning that we already relied on legacy options (if the later code has
> all it needed, then only the warning is emitted; if later code fails, the
> user now has the warning in addition to the later failure message to help
> them resolve their images).
> 
> A bit confusing how it all fits together, but it seems to work out in the
> end.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Thanks for the review!

[Qemu-devel] [PATCH v2 3/3] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Jeff Cody]

This is a small test that will check for the ability to parse
both legacy and modern options for rbd.

The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server.  The error
messages in the success path show that the arguments were parsed.

The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/231     | 62 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/231.out |  9 ++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 0000000000..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph.  This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jcody@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+    sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.  Just want it
+# to fail in the right way.
+$QEMU_IMG info "json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 2>&1 | _filter_conf
+$QEMU_IMG info "json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}" 2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 0000000000..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.  Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}': error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
 226 auto quick
 227 auto quick
 229 auto quick
+231 auto quick
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/3] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Eric Blake]

On 9/11/18 3:43 PM, Jeff Cody wrote:
> This is a small test that will check for the ability to parse
> both legacy and modern options for rbd.
> 
> The way the test is set up is for failure to occur, but without
> having to wait to timeout on a non-existent rbd server.  The error
> messages in the success path show that the arguments were parsed.
> 
> The failure behavior prior to the patch series that has this test, is
> qemu-img complaining about mandatory options (e.g. 'pool') not being
> provided.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   tests/qemu-iotests/231     | 62 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/231.out |  9 ++++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 72 insertions(+)
>   create mode 100755 tests/qemu-iotests/231
>   create mode 100644 tests/qemu-iotests/231.out

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org