[Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
stores the resulting QString in a QDict.

qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
a QList.

Drop both conversions, store the QList instead.

This affects output of qemu-img info.  Before this patch:

    $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
    [junk printed by Ceph library code...]
    image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
    [more output, not interesting here]

After this patch, we get

    image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}

The value of member "=keyvalue-pairs" changes from a string containing
a JSON array to that JSON array.  That's an improvement of sorts.  However:

* Should "=keyvalue-pairs" even be visible here?  It's supposed to be
  purely internal...

* Is this a stable interface we need to preserve, warts and all?

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 50 ++++++++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..ddc59e1c7a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -23,7 +23,6 @@
 #include "qemu/cutils.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
@@ -106,7 +105,7 @@ typedef struct BDRVRBDState {
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             BlockdevOptionsRbd *opts, bool cache,
-                            const char *keypairs, const char *secretid,
+                            QList *keypairs, const char *secretid,
                             Error **errp);
 
 static char *qemu_rbd_next_tok(char *src, char delim, char **p)
@@ -221,13 +220,11 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     }
 
     if (keypairs) {
-        qdict_put(options, "=keyvalue-pairs",
-                  qobject_to_json(QOBJECT(keypairs)));
+        qdict_put(options, "=keyvalue-pairs", keypairs);
     }
 
 done:
     g_free(buf);
-    qobject_unref(keypairs);
     return;
 }
 
@@ -281,42 +278,36 @@ static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
     return 0;
 }
 
-static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
+static int qemu_rbd_set_keypairs(rados_t cluster, QList *keypairs,
                                  Error **errp)
 {
-    QList *keypairs;
+    const QListEntry *entry1, *entry2;
     QString *name;
     QString *value;
     const char *key;
-    size_t remaining;
     int ret = 0;
 
-    if (!keypairs_json) {
+    if (!keypairs) {
         return ret;
     }
-    keypairs = qobject_to(QList,
-                          qobject_from_json(keypairs_json, &error_abort));
-    remaining = qlist_size(keypairs) / 2;
-    assert(remaining);
 
-    while (remaining--) {
-        name = qobject_to(QString, qlist_pop(keypairs));
-        value = qobject_to(QString, qlist_pop(keypairs));
+    entry1 = qlist_first(keypairs);
+    do {
+        entry2 = qlist_next(entry1);
+        name = qobject_to(QString, qlist_entry_obj(entry1));
+        value = qobject_to(QString, qlist_entry_obj(entry2));
         assert(name && value);
         key = qstring_get_str(name);
 
         ret = rados_conf_set(cluster, key, qstring_get_str(value));
-        qobject_unref(value);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "invalid conf option %s", key);
-            qobject_unref(name);
             ret = -EINVAL;
             break;
         }
-        qobject_unref(name);
-    }
+        entry1 = qlist_next(entry2);
+    } while(entry1);
 
-    qobject_unref(keypairs);
     return ret;
 }
 
@@ -370,7 +361,7 @@ static QemuOptsList runtime_opts = {
 
 /* FIXME Deprecate and remove keypairs or make it available in QMP. */
 static int qemu_rbd_do_create(BlockdevCreateOptions *options,
-                              const char *keypairs, const char *password_secret,
+                              QList *keypairs, const char *password_secret,
                               Error **errp)
 {
     BlockdevCreateOptionsRbd *opts = &options->u.rbd;
@@ -430,7 +421,8 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
     BlockdevCreateOptionsRbd *rbd_opts;
     BlockdevOptionsRbd *loc;
     Error *local_err = NULL;
-    const char *keypairs, *password_secret;
+    const char *password_secret;
+    QList *keypairs;
     QDict *options = NULL;
     int ret = 0;
 
@@ -470,7 +462,8 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
     loc->user     = g_strdup(qdict_get_try_str(options, "user"));
     loc->has_user = !!loc->user;
     loc->image    = g_strdup(qdict_get_try_str(options, "image"));
-    keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
+    /* non-string type, but it comes from qemu_rbd_parse_filename() */
+    keypairs      = qdict_get_qlist(options, "=keyvalue-pairs");
 
     ret = qemu_rbd_do_create(create_options, keypairs, password_secret, errp);
     if (ret < 0) {
@@ -567,7 +560,7 @@ static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             BlockdevOptionsRbd *opts, bool cache,
-                            const char *keypairs, const char *secretid,
+                            QList *keypairs, const char *secretid,
                             Error **errp)
 {
     char *mon_host = NULL;
@@ -663,10 +656,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     Visitor *v;
     const QDictEntry *e;
     Error *local_err = NULL;
-    char *keypairs, *secretid;
+    QList *keypairs;
+    char *secretid;
     int r;
 
-    keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+    keypairs = qobject_ref(qdict_get_qlist(options, "=keyvalue-pairs"));
     if (keypairs) {
         qdict_del(options, "=keyvalue-pairs");
     }
@@ -741,7 +735,7 @@ failed_open:
     rados_shutdown(s->cluster);
 out:
     qapi_free_BlockdevOptionsRbd(opts);
-    g_free(keypairs);
+    qobject_unref(keypairs);
     g_free(secretid);
     return r;
 }
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180725151011.25951-1-armbru@redhat.com
Subject: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
bea5b49489 rbd: Don't convert keypairs to JSON and back

=== OUTPUT BEGIN ===
Checking PATCH 1/1: rbd: Don't convert keypairs to JSON and back...
ERROR: space required before the open parenthesis '('
#120: FILE: block/rbd.c:309:
+    } while(entry1);

total: 1 errors, 0 warnings, 136 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Eric Blake]

On 07/25/2018 10:10 AM, Markus Armbruster wrote:
> qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
> stores the resulting QString in a QDict.
> 
> qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
> QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
> a QList.
> 
> Drop both conversions, store the QList instead.
> 
> This affects output of qemu-img info.  Before this patch:
> 
>      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>      [junk printed by Ceph library code...]
>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
>      [more output, not interesting here]
> 
> After this patch, we get
> 
>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
> 
> The value of member "=keyvalue-pairs" changes from a string containing
> a JSON array to that JSON array.  That's an improvement of sorts.  However:
> 
> * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>    purely internal...

I'd argue that since it is supposed to be internal (as evidenced by the 
leading '=' that does not name a normal variable), changing it doesn't 
hurt stability. But yes, it would be nicer if we could filter it 
entirely so that it does not appear in json: output, if it doesn't truly 
affect the contents that the guest would see.

> 
> * Is this a stable interface we need to preserve, warts and all?

I hope not.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/rbd.c | 50 ++++++++++++++++++++++----------------------------
>   1 file changed, 22 insertions(+), 28 deletions(-)

I'm not yet convinced if we want this patch for 3.0 without more 
comments from the RBD experts, nor do I see too much of an issue if this 
doesn't go in until 3.1.  But as to the code changes itself, I find them 
nice.

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] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Daniel P. Berrangé]

On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
> > qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
> > stores the resulting QString in a QDict.
> > 
> > qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
> > QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
> > a QList.
> > 
> > Drop both conversions, store the QList instead.
> > 
> > This affects output of qemu-img info.  Before this patch:
> > 
> >      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
> >      [junk printed by Ceph library code...]
> >      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
> >      [more output, not interesting here]
> > 
> > After this patch, we get
> > 
> >      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
> > 
> > The value of member "=keyvalue-pairs" changes from a string containing
> > a JSON array to that JSON array.  That's an improvement of sorts.  However:
> > 
> > * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
> >    purely internal...
> 
> I'd argue that since it is supposed to be internal (as evidenced by the
> leading '=' that does not name a normal variable), changing it doesn't hurt
> stability. But yes, it would be nicer if we could filter it entirely so that
> it does not appear in json: output, if it doesn't truly affect the contents
> that the guest would see.

If it appears in the json: output, then that means it could get written
into qcow2 headers as a backing file name, which would make it ABI
sensitive. This makes it even more important to filter it if it is supposed
to be internal only, with no ABI guarantee.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180725151011.25951-1-armbru@redhat.com
Subject: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
cbf4f43c2e rbd: Don't convert keypairs to JSON and back

=== OUTPUT BEGIN ===
Checking PATCH 1/1: rbd: Don't convert keypairs to JSON and back...
ERROR: space required before the open parenthesis '('
#121: FILE: block/rbd.c:309:
+    } while(entry1);

total: 1 errors, 0 warnings, 136 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Jeff Cody]

On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
> >qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
> >stores the resulting QString in a QDict.
> >
> >qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
> >QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
> >a QList.
> >
> >Drop both conversions, store the QList instead.
> >
> >This affects output of qemu-img info.  Before this patch:
> >
> >     $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
> >     [junk printed by Ceph library code...]
> >     image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
> >     [more output, not interesting here]
> >
> >After this patch, we get
> >
> >     image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
> >
> >The value of member "=keyvalue-pairs" changes from a string containing
> >a JSON array to that JSON array.  That's an improvement of sorts.  However:
> >
> >* Should "=keyvalue-pairs" even be visible here?  It's supposed to be
> >   purely internal...
> 
> I'd argue that since it is supposed to be internal (as evidenced by the
> leading '=' that does not name a normal variable), changing it doesn't hurt
> stability. But yes, it would be nicer if we could filter it entirely so that
> it does not appear in json: output, if it doesn't truly affect the contents
> that the guest would see.
> 
> >
> >* Is this a stable interface we need to preserve, warts and all?
> 
> I hope not.
> 
> >
> >Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >---
> >  block/rbd.c | 50 ++++++++++++++++++++++----------------------------
> >  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> I'm not yet convinced if we want this patch for 3.0 without more comments
> from the RBD experts, nor do I see too much of an issue if this doesn't go
> in until 3.1.  But as to the code changes itself, I find them nice.

Based on my IRC discussions with Markus, I believe the target for this patch
is indeed 3.1, not 3.0.

> 
> 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] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Jeff Cody]

On Wed, Jul 25, 2018 at 05:01:44PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
> > On 07/25/2018 10:10 AM, Markus Armbruster wrote:
> > > qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
> > > stores the resulting QString in a QDict.
> > > 
> > > qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
> > > QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
> > > a QList.
> > > 
> > > Drop both conversions, store the QList instead.
> > > 
> > > This affects output of qemu-img info.  Before this patch:
> > > 
> > >      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
> > >      [junk printed by Ceph library code...]
> > >      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
> > >      [more output, not interesting here]
> > > 
> > > After this patch, we get
> > > 
> > >      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
> > > 
> > > The value of member "=keyvalue-pairs" changes from a string containing
> > > a JSON array to that JSON array.  That's an improvement of sorts.  However:
> > > 
> > > * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
> > >    purely internal...
> > 
> > I'd argue that since it is supposed to be internal (as evidenced by the
> > leading '=' that does not name a normal variable), changing it doesn't hurt
> > stability. But yes, it would be nicer if we could filter it entirely so that
> > it does not appear in json: output, if it doesn't truly affect the contents
> > that the guest would see.
> 
> If it appears in the json: output, then that means it could get written
> into qcow2 headers as a backing file name, which would make it ABI
> sensitive. This makes it even more important to filter it if it is supposed
> to be internal only, with no ABI guarantee.
> 

It's been present for a couple releases (counting 3.0); is it safe to
assume that, although it could be present in the qcow2 headers, that it will
not break anything by altering it or removing it?

If so, and we are comfortable changing the output the way this patch does
(technically altering ABI anyway), we might as well go all the way and
filter it out completely.  That would be preferable to cleaning up the json
output of the internal key/value pairs, IMO.

-Jeff

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

On 2018-07-28 06:32, Jeff Cody wrote:
> On Wed, Jul 25, 2018 at 05:01:44PM +0100, Daniel P. Berrangé wrote:
>> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
>>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
>>>> qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
>>>> stores the resulting QString in a QDict.
>>>>
>>>> qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
>>>> QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
>>>> a QList.
>>>>
>>>> Drop both conversions, store the QList instead.
>>>>
>>>> This affects output of qemu-img info.  Before this patch:
>>>>
>>>>      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>>>>      [junk printed by Ceph library code...]
>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
>>>>      [more output, not interesting here]
>>>>
>>>> After this patch, we get
>>>>
>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
>>>>
>>>> The value of member "=keyvalue-pairs" changes from a string containing
>>>> a JSON array to that JSON array.  That's an improvement of sorts.  However:
>>>>
>>>> * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>>>>    purely internal...
>>>
>>> I'd argue that since it is supposed to be internal (as evidenced by the
>>> leading '=' that does not name a normal variable), changing it doesn't hurt
>>> stability. But yes, it would be nicer if we could filter it entirely so that
>>> it does not appear in json: output, if it doesn't truly affect the contents
>>> that the guest would see.
>>
>> If it appears in the json: output, then that means it could get written
>> into qcow2 headers as a backing file name, which would make it ABI
>> sensitive. This makes it even more important to filter it if it is supposed
>> to be internal only, with no ABI guarantee.
>>
> 
> It's been present for a couple releases (counting 3.0); is it safe to
> assume that, although it could be present in the qcow2 headers, that it will
> not break anything by altering it or removing it?

Did =keyvalue-pairs even work in json:{} filename?  If so, it will
continue to work even after filtering it.  If not, then filtering it
won't break existing files because they didn't work before either.

To me personally the issue is that if you can specify a plain filename,
bdrv_refresh_filename() should give you that plain filename back.  So
rbd's implementation of that is lacking.  Well, it just doesn't exist.

> If so, and we are comfortable changing the output the way this patch does
> (technically altering ABI anyway), we might as well go all the way and
> filter it out completely.  That would be preferable to cleaning up the json
> output of the internal key/value pairs, IMO.

Well, this filtering at least is done by my "Fix some filename
generation issues" series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
>> >qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
>> >stores the resulting QString in a QDict.
>> >
>> >qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
>> >QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
>> >a QList.
>> >
>> >Drop both conversions, store the QList instead.
>> >
>> >This affects output of qemu-img info.  Before this patch:
>> >
>> >     $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>> >     [junk printed by Ceph library code...]
>> >     image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
>> >     [more output, not interesting here]
>> >
>> >After this patch, we get
>> >
>> >     image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
>> >
>> >The value of member "=keyvalue-pairs" changes from a string containing
>> >a JSON array to that JSON array.  That's an improvement of sorts.  However:
>> >
>> >* Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>> >   purely internal...
>> 
>> I'd argue that since it is supposed to be internal (as evidenced by the
>> leading '=' that does not name a normal variable), changing it doesn't hurt
>> stability. But yes, it would be nicer if we could filter it entirely so that
>> it does not appear in json: output, if it doesn't truly affect the contents
>> that the guest would see.
>> 
>> >
>> >* Is this a stable interface we need to preserve, warts and all?
>> 
>> I hope not.
>> 
>> >
>> >Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >---
>> >  block/rbd.c | 50 ++++++++++++++++++++++----------------------------
>> >  1 file changed, 22 insertions(+), 28 deletions(-)
>> 
>> I'm not yet convinced if we want this patch for 3.0 without more comments
>> from the RBD experts, nor do I see too much of an issue if this doesn't go
>> in until 3.1.  But as to the code changes itself, I find them nice.
>
> Based on my IRC discussions with Markus, I believe the target for this patch
> is indeed 3.1, not 3.0.

Unless we conclude we want to change qemu-img info sooner rather than
later, which seems unlikely.

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2018-07-28 06:32, Jeff Cody wrote:
>> On Wed, Jul 25, 2018 at 05:01:44PM +0100, Daniel P. Berrangé wrote:
>>> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
>>>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
>>>>> qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
>>>>> stores the resulting QString in a QDict.
>>>>>
>>>>> qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
>>>>> QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
>>>>> a QList.
>>>>>
>>>>> Drop both conversions, store the QList instead.
>>>>>
>>>>> This affects output of qemu-img info.  Before this patch:
>>>>>
>>>>>      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>>>>>      [junk printed by Ceph library code...]
>>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
>>>>>      [more output, not interesting here]
>>>>>
>>>>> After this patch, we get
>>>>>
>>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
>>>>>
>>>>> The value of member "=keyvalue-pairs" changes from a string containing
>>>>> a JSON array to that JSON array.  That's an improvement of sorts.  However:
>>>>>
>>>>> * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>>>>>    purely internal...
>>>>
>>>> I'd argue that since it is supposed to be internal (as evidenced by the
>>>> leading '=' that does not name a normal variable), changing it doesn't hurt
>>>> stability. But yes, it would be nicer if we could filter it entirely so that
>>>> it does not appear in json: output, if it doesn't truly affect the contents
>>>> that the guest would see.
>>>
>>> If it appears in the json: output, then that means it could get written
>>> into qcow2 headers as a backing file name, which would make it ABI
>>> sensitive. This makes it even more important to filter it if it is supposed
>>> to be internal only, with no ABI guarantee.
>>>
>> 
>> It's been present for a couple releases (counting 3.0); is it safe to
>> assume that, although it could be present in the qcow2 headers, that it will
>> not break anything by altering it or removing it?
>
> Did =keyvalue-pairs even work in json:{} filename?  If so, it will
> continue to work even after filtering it.  If not, then filtering it
> won't break existing files because they didn't work before either.

I'm afraid it does work:

    $ gdb --args upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg 'json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}'
    GNU gdb (GDB) Fedora 8.1-25.fc28
    [...]
    (gdb) b qemu_rbd_open 
    Breakpoint 1 at 0x845f83: file /work/armbru/qemu/block/rbd.c, line 660.
    (gdb) r
    Starting program: /home/armbru/bin/upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg json:\{\"driver\":\ \"raw\",\ \"file\":\ \{\"pool\":\ \"rbd\",\ \"image\":\ \"testimg.raw\",\ \"conf\":\ \"/tmp/ceph.conf\",\ \"driver\":\ \"rbd\",\ \"=keyvalue-pairs\":\ \"\[\\\"mon_host\\\",\ \\\"192.168.15.180\\\",\ \\\"rbd_cache\\\",\ \\\"true\\\"\]\"\}\}
    [...]
    Thread 1 "upstream-qemu" hit Breakpoint 1, qemu_rbd_open (bs=0x555556a5a970, 
        options=0x555556a5ec40, flags=24578, errp=0x7fffffffd370)
        at /work/armbru/qemu/block/rbd.c:660
    660	{
    [...]
    (gdb) n
    661	    BDRVRBDState *s = bs->opaque;
    (gdb) 
    662	    BlockdevOptionsRbd *opts = NULL;
    (gdb) 
    665	    Error *local_err = NULL;
    (gdb) 
    669	    keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
    (gdb) 
    670	    if (keypairs) {
    (gdb) p keypairs 
    $1 = 0x5555569e54c0 "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"

It really, really, really should not work.

It doesn't work with anything that relies on QAPI to represent
configuration (such as QMP's blockdev-add), because BlockdevOptionsRbd
doesn't have it.

It works with -drive only with a pseudo-filename (more on that below),
even though -drive uses QemuOpts and QDict rather than QAPI, because the
(carefully chosen) name "=keyvalue-pairs" is impossible to use with
QemuOpts.

However, we missed the json:... backdoor :(

Block device configuration has become waaaaay too baroque.  I can't keep
enough of it in my mind at the same time to change it safely.  I believe
none of us can.

> To me personally the issue is that if you can specify a plain filename,
> bdrv_refresh_filename() should give you that plain filename back.  So
> rbd's implementation of that is lacking.  Well, it just doesn't exist.

I'm not even sure I understand what you're talking about.

>> If so, and we are comfortable changing the output the way this patch does
>> (technically altering ABI anyway), we might as well go all the way and
>> filter it out completely.  That would be preferable to cleaning up the json
>> output of the internal key/value pairs, IMO.
>
> Well, this filtering at least is done by my "Fix some filename
> generation issues" series.

Likewise.

Back to rbd.  =keyvalue-pairs exists only to implement the part after
':' in pseudo-filenames
rbd:poolname/devicename[@snapshotname][:option1=value1[:option2=value2...]]

Lets you pass arbitrary configuration to rados_conf_set().  We pass it
before we pass configuration the rbd driver computes (such as
rbd_cache), which should get conflicting key-value pairs silently
ignored.

We treat "id" and "conf" specially.  "id" gets passed to rados_create(),
not rados_conf_set().  "conf" names a configuration file, i.e. it's yet
another way to pass arbitrary configuration, this time via
rados_conf_read_file().  We call that before passing the non-special
key-value pairs to rados_conf_set(), which should get conflicting
settings in the conf file silently ignored.

We provide the equivalent to "id" and "conf" in QAPI, but we refused to
provide key-value pairs.

Same for -drive without a pseudo-filename.

Unfortunately, our attempt to confine the unloved key-value pair feature
to pseudo-filenames has failed: it escaped through the json: backdoor.

Can we get rid of the key-value pair feature?

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Jeff Cody]

On Wed, Aug 15, 2018 at 10:12:12AM +0200, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
> > On 2018-07-28 06:32, Jeff Cody wrote:
> >> On Wed, Jul 25, 2018 at 05:01:44PM +0100, Daniel P. Berrangé wrote:
> >>> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
> >>>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
> >>>>> qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
> >>>>> stores the resulting QString in a QDict.
> >>>>>
> >>>>> qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
> >>>>> QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
> >>>>> a QList.
> >>>>>
> >>>>> Drop both conversions, store the QList instead.
> >>>>>
> >>>>> This affects output of qemu-img info.  Before this patch:
> >>>>>
> >>>>>      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
> >>>>>      [junk printed by Ceph library code...]
> >>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
> >>>>>      [more output, not interesting here]
> >>>>>
> >>>>> After this patch, we get
> >>>>>
> >>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
> >>>>>
> >>>>> The value of member "=keyvalue-pairs" changes from a string containing
> >>>>> a JSON array to that JSON array.  That's an improvement of sorts.  However:
> >>>>>
> >>>>> * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
> >>>>>    purely internal...
> >>>>
> >>>> I'd argue that since it is supposed to be internal (as evidenced by the
> >>>> leading '=' that does not name a normal variable), changing it doesn't hurt
> >>>> stability. But yes, it would be nicer if we could filter it entirely so that
> >>>> it does not appear in json: output, if it doesn't truly affect the contents
> >>>> that the guest would see.
> >>>
> >>> If it appears in the json: output, then that means it could get written
> >>> into qcow2 headers as a backing file name, which would make it ABI
> >>> sensitive. This makes it even more important to filter it if it is supposed
> >>> to be internal only, with no ABI guarantee.
> >>>
> >> 
> >> It's been present for a couple releases (counting 3.0); is it safe to
> >> assume that, although it could be present in the qcow2 headers, that it will
> >> not break anything by altering it or removing it?
> >
> > Did =keyvalue-pairs even work in json:{} filename?  If so, it will
> > continue to work even after filtering it.  If not, then filtering it
> > won't break existing files because they didn't work before either.
> 
> I'm afraid it does work:
> 
>     $ gdb --args upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg 'json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}'
>     GNU gdb (GDB) Fedora 8.1-25.fc28
>     [...]
>     (gdb) b qemu_rbd_open 
>     Breakpoint 1 at 0x845f83: file /work/armbru/qemu/block/rbd.c, line 660.
>     (gdb) r
>     Starting program: /home/armbru/bin/upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg json:\{\"driver\":\ \"raw\",\ \"file\":\ \{\"pool\":\ \"rbd\",\ \"image\":\ \"testimg.raw\",\ \"conf\":\ \"/tmp/ceph.conf\",\ \"driver\":\ \"rbd\",\ \"=keyvalue-pairs\":\ \"\[\\\"mon_host\\\",\ \\\"192.168.15.180\\\",\ \\\"rbd_cache\\\",\ \\\"true\\\"\]\"\}\}
>     [...]
>     Thread 1 "upstream-qemu" hit Breakpoint 1, qemu_rbd_open (bs=0x555556a5a970, 
>         options=0x555556a5ec40, flags=24578, errp=0x7fffffffd370)
>         at /work/armbru/qemu/block/rbd.c:660
>     660	{
>     [...]
>     (gdb) n
>     661	    BDRVRBDState *s = bs->opaque;
>     (gdb) 
>     662	    BlockdevOptionsRbd *opts = NULL;
>     (gdb) 
>     665	    Error *local_err = NULL;
>     (gdb) 
>     669	    keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>     (gdb) 
>     670	    if (keypairs) {
>     (gdb) p keypairs 
>     $1 = 0x5555569e54c0 "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"
> 
> It really, really, really should not work.
> 
> It doesn't work with anything that relies on QAPI to represent
> configuration (such as QMP's blockdev-add), because BlockdevOptionsRbd
> doesn't have it.
> 
> It works with -drive only with a pseudo-filename (more on that below),
> even though -drive uses QemuOpts and QDict rather than QAPI, because the
> (carefully chosen) name "=keyvalue-pairs" is impossible to use with
> QemuOpts.
> 
> However, we missed the json:... backdoor :(
> 
> Block device configuration has become waaaaay too baroque.  I can't keep
> enough of it in my mind at the same time to change it safely.  I believe
> none of us can.
> 
> > To me personally the issue is that if you can specify a plain filename,
> > bdrv_refresh_filename() should give you that plain filename back.  So
> > rbd's implementation of that is lacking.  Well, it just doesn't exist.
> 
> I'm not even sure I understand what you're talking about.
> 
> >> If so, and we are comfortable changing the output the way this patch does
> >> (technically altering ABI anyway), we might as well go all the way and
> >> filter it out completely.  That would be preferable to cleaning up the json
> >> output of the internal key/value pairs, IMO.
> >
> > Well, this filtering at least is done by my "Fix some filename
> > generation issues" series.
> 
> Likewise.
> 
> Back to rbd.  =keyvalue-pairs exists only to implement the part after
> ':' in pseudo-filenames
> rbd:poolname/devicename[@snapshotname][:option1=value1[:option2=value2...]]
> 
> Lets you pass arbitrary configuration to rados_conf_set().  We pass it
> before we pass configuration the rbd driver computes (such as
> rbd_cache), which should get conflicting key-value pairs silently
> ignored.
> 
> We treat "id" and "conf" specially.  "id" gets passed to rados_create(),
> not rados_conf_set().  "conf" names a configuration file, i.e. it's yet
> another way to pass arbitrary configuration, this time via
> rados_conf_read_file().  We call that before passing the non-special
> key-value pairs to rados_conf_set(), which should get conflicting
> settings in the conf file silently ignored.
> 
> We provide the equivalent to "id" and "conf" in QAPI, but we refused to
> provide key-value pairs.
> 
> Same for -drive without a pseudo-filename.
> 
> Unfortunately, our attempt to confine the unloved key-value pair feature
> to pseudo-filenames has failed: it escaped through the json: backdoor.
> 
> Can we get rid of the key-value pair feature?

I'm concerned about just removing the key-value pair feature, because it has
been around for quite a while now. The risk that it is being used as a way
to configure the (many) different rbd options that we do not directly
support is, I fear, too high.

But as you mentioned on irc, perhaps a deprecation period would work.
During this period we could work on adding whatever options make sense that
are currently not supported, and document that other unsupported options are
only supported via rbd config file.

But in this case, I think it is still best to try and figure out a
reasonable way to filter the json: output, so that the troublesome key/value
pairs are not present during the whole deprecation period.

But then, if we have the ability to suppress the key/value pair in the json
output, is it still necessary to deprecate it as well?  From a design
standpoint, it will remove some hacky code, so I think it still would make
sense to deprecate too.

-Jeff

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

On 2018-08-15 10:12, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:

[...]

>> To me personally the issue is that if you can specify a plain filename,
>> bdrv_refresh_filename() should give you that plain filename back.  So
>> rbd's implementation of that is lacking.  Well, it just doesn't exist.
> 
> I'm not even sure I understand what you're talking about.

We have this bdrv_refresh_filename() thing which can do this:

$ qemu-img info \
    "json:{'driver':'raw',
           'file':{'driver':'nbd','host':'localhost'}}"
image: nbd://localhost:10809
[...]

So it can reconstruct a plain filename even if you specify it as options
instead of just using a plain filename.


Now here's my fault: I thought it might be necessary for a driver to
implement that function (which rbd doesn't) so that you'd get a nice
filename back (instead of just json:{} garbled things).   But you don't.
 For protocol drivers, you'll just get the initial filename back.  (So
my comment was just wrong.)

So what I was thinking about was some case where you specified a normal
plain filename and qemu would give you back json:{}.  (If rbd
implemented bdrv_refresh_filename(), that wouldn't happen, because it
would reconstruct a nice normal filename.)  It turns out, I don't think
that can happen so easily.  You'll just get your filename back.

Because here's what I'm thinking: If someone uses an option that is
undocumented and starts with =, well, too bad.  If someone uses a normal
filename, but gets back a json:{} filename...  Then they are free to use
that anywhere, and their use of "=" is legitimized.


Now that issue kind of reappears when you open an RBD volume, and then
e.g. take a blockdev-snapshot.  Then your overlay has an overridden
backing file (one that may be different from what its image header says)
and its filename may well become a json:{} one (to arrange for the
overridden backing file options).  Of course, if you opened the RBD
volume with a filename with some of the options warranting
=keyvalue-pairs, then your json:{} filename will contain those options
under =keyvalue-pairs.

So...  I'm not quite sure what I want to say?  I think there are edge
cases where the user may not have put any weird option into qemu, but
they do get a json:{} filename with =keyvalue-pairs out of it.  And I
think users are free to use json:{} filenames qemu spews at them, and we
can't blame them for it.

>>> If so, and we are comfortable changing the output the way this patch does
>>> (technically altering ABI anyway), we might as well go all the way and
>>> filter it out completely.  That would be preferable to cleaning up the json
>>> output of the internal key/value pairs, IMO.
>>
>> Well, this filtering at least is done by my "Fix some filename
>> generation issues" series.
> 
> Likewise.

The series overhauls quite a bit of the bdrv_refresh_filename()
infrastructure.  That function is also responsible for generating
json:{} filenames.

One thing it introduces is a BlockDriver field where a driver can
specify which of the runtime options are actually important.  The rest
is omitted from the generated json:{} filename.

I may have taken the liberty not to include =keyvalue-pairs in RBD's
"strong runtime options" list.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Wed, Aug 15, 2018 at 10:12:12AM +0200, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>> > On 2018-07-28 06:32, Jeff Cody wrote:
>> >> On Wed, Jul 25, 2018 at 05:01:44PM +0100, Daniel P. Berrangé wrote:
>> >>> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
>> >>>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
>> >>>>> qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
>> >>>>> stores the resulting QString in a QDict.
>> >>>>>
>> >>>>> qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
>> >>>>> QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
>> >>>>> a QList.
>> >>>>>
>> >>>>> Drop both conversions, store the QList instead.
>> >>>>>
>> >>>>> This affects output of qemu-img info.  Before this patch:
>> >>>>>
>> >>>>>      $ qemu-img info rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>> >>>>>      [junk printed by Ceph library code...]
>> >>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}
>> >>>>>      [more output, not interesting here]
>> >>>>>
>> >>>>> After this patch, we get
>> >>>>>
>> >>>>>      image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
>> >>>>>
>> >>>>> The value of member "=keyvalue-pairs" changes from a string containing
>> >>>>> a JSON array to that JSON array.  That's an improvement of sorts.  However:
>> >>>>>
>> >>>>> * Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>> >>>>>    purely internal...
>> >>>>
>> >>>> I'd argue that since it is supposed to be internal (as evidenced by the
>> >>>> leading '=' that does not name a normal variable), changing it doesn't hurt
>> >>>> stability. But yes, it would be nicer if we could filter it entirely so that
>> >>>> it does not appear in json: output, if it doesn't truly affect the contents
>> >>>> that the guest would see.
>> >>>
>> >>> If it appears in the json: output, then that means it could get written
>> >>> into qcow2 headers as a backing file name, which would make it ABI
>> >>> sensitive. This makes it even more important to filter it if it is supposed
>> >>> to be internal only, with no ABI guarantee.
>> >>>
>> >> 
>> >> It's been present for a couple releases (counting 3.0); is it safe to
>> >> assume that, although it could be present in the qcow2 headers, that it will
>> >> not break anything by altering it or removing it?
>> >
>> > Did =keyvalue-pairs even work in json:{} filename?  If so, it will
>> > continue to work even after filtering it.  If not, then filtering it
>> > won't break existing files because they didn't work before either.
>> 
>> I'm afraid it does work:
>> 
>>     $ gdb --args upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg 'json:{"driver": "raw", "file": {"pool": "rbd", "image": "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"}}'
>>     GNU gdb (GDB) Fedora 8.1-25.fc28
>>     [...]
>>     (gdb) b qemu_rbd_open 
>>     Breakpoint 1 at 0x845f83: file /work/armbru/qemu/block/rbd.c, line 660.
>>     (gdb) r
>>     Starting program: /home/armbru/bin/upstream-qemu -nodefaults -S -display vnc=:0 -readconfig test.cfg json:\{\"driver\":\ \"raw\",\ \"file\":\ \{\"pool\":\ \"rbd\",\ \"image\":\ \"testimg.raw\",\ \"conf\":\ \"/tmp/ceph.conf\",\ \"driver\":\ \"rbd\",\ \"=keyvalue-pairs\":\ \"\[\\\"mon_host\\\",\ \\\"192.168.15.180\\\",\ \\\"rbd_cache\\\",\ \\\"true\\\"\]\"\}\}
>>     [...]
>>     Thread 1 "upstream-qemu" hit Breakpoint 1, qemu_rbd_open (bs=0x555556a5a970, 
>>         options=0x555556a5ec40, flags=24578, errp=0x7fffffffd370)
>>         at /work/armbru/qemu/block/rbd.c:660
>>     660	{
>>     [...]
>>     (gdb) n
>>     661	    BDRVRBDState *s = bs->opaque;
>>     (gdb) 
>>     662	    BlockdevOptionsRbd *opts = NULL;
>>     (gdb) 
>>     665	    Error *local_err = NULL;
>>     (gdb) 
>>     669	    keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>>     (gdb) 
>>     670	    if (keypairs) {
>>     (gdb) p keypairs 
>>     $1 = 0x5555569e54c0 "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", \"true\"]"
>> 
>> It really, really, really should not work.
>> 
>> It doesn't work with anything that relies on QAPI to represent
>> configuration (such as QMP's blockdev-add), because BlockdevOptionsRbd
>> doesn't have it.
>> 
>> It works with -drive only with a pseudo-filename (more on that below),
>> even though -drive uses QemuOpts and QDict rather than QAPI, because the
>> (carefully chosen) name "=keyvalue-pairs" is impossible to use with
>> QemuOpts.
>> 
>> However, we missed the json:... backdoor :(
>> 
>> Block device configuration has become waaaaay too baroque.  I can't keep
>> enough of it in my mind at the same time to change it safely.  I believe
>> none of us can.
>> 
>> > To me personally the issue is that if you can specify a plain filename,
>> > bdrv_refresh_filename() should give you that plain filename back.  So
>> > rbd's implementation of that is lacking.  Well, it just doesn't exist.
>> 
>> I'm not even sure I understand what you're talking about.
>> 
>> >> If so, and we are comfortable changing the output the way this patch does
>> >> (technically altering ABI anyway), we might as well go all the way and
>> >> filter it out completely.  That would be preferable to cleaning up the json
>> >> output of the internal key/value pairs, IMO.
>> >
>> > Well, this filtering at least is done by my "Fix some filename
>> > generation issues" series.
>> 
>> Likewise.
>> 
>> Back to rbd.  =keyvalue-pairs exists only to implement the part after
>> ':' in pseudo-filenames
>> rbd:poolname/devicename[@snapshotname][:option1=value1[:option2=value2...]]
>> 
>> Lets you pass arbitrary configuration to rados_conf_set().  We pass it
>> before we pass configuration the rbd driver computes (such as
>> rbd_cache), which should get conflicting key-value pairs silently
>> ignored.
>> 
>> We treat "id" and "conf" specially.  "id" gets passed to rados_create(),
>> not rados_conf_set().  "conf" names a configuration file, i.e. it's yet
>> another way to pass arbitrary configuration, this time via
>> rados_conf_read_file().  We call that before passing the non-special
>> key-value pairs to rados_conf_set(), which should get conflicting
>> settings in the conf file silently ignored.
>> 
>> We provide the equivalent to "id" and "conf" in QAPI, but we refused to
>> provide key-value pairs.
>> 
>> Same for -drive without a pseudo-filename.
>> 
>> Unfortunately, our attempt to confine the unloved key-value pair feature
>> to pseudo-filenames has failed: it escaped through the json: backdoor.
>> 
>> Can we get rid of the key-value pair feature?
>
> I'm concerned about just removing the key-value pair feature, because it has
> been around for quite a while now. The risk that it is being used as a way
> to configure the (many) different rbd options that we do not directly
> support is, I fear, too high.
>
> But as you mentioned on irc, perhaps a deprecation period would work.
> During this period we could work on adding whatever options make sense that
> are currently not supported, and document that other unsupported options are
> only supported via rbd config file.

Deprecating a special way to configure things that is *only* available
in pseudo-filenames feels like a no-brainer to me.  If it's too ugly to
be provided in QAPI and QemuOpts, then it's too ugly to live on.

If there's a demand for being able to specify certain options directly
rather than via configuration file, we can add them.  I don't see a
dependency between that and deprecating key-value pairs in
pseudo-filenames, though.

> But in this case, I think it is still best to try and figure out a
> reasonable way to filter the json: output, so that the troublesome key/value
> pairs are not present during the whole deprecation period.

We attempted to hide them, but failed.  If you can fix that, show us
your patches :)

> But then, if we have the ability to suppress the key/value pair in the json
> output, is it still necessary to deprecate it as well?  From a design
> standpoint, it will remove some hacky code, so I think it still would make
> sense to deprecate too.

Maintainer's choice, I think.

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2018-08-15 10:12, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>
> [...]
>
>>> To me personally the issue is that if you can specify a plain filename,
>>> bdrv_refresh_filename() should give you that plain filename back.  So
>>> rbd's implementation of that is lacking.  Well, it just doesn't exist.
>> 
>> I'm not even sure I understand what you're talking about.
>
> We have this bdrv_refresh_filename() thing which can do this:
>
> $ qemu-img info \
>     "json:{'driver':'raw',
>            'file':{'driver':'nbd','host':'localhost'}}"
> image: nbd://localhost:10809
> [...]
>
> So it can reconstruct a plain filename even if you specify it as options
> instead of just using a plain filename.
>
>
> Now here's my fault: I thought it might be necessary for a driver to
> implement that function (which rbd doesn't) so that you'd get a nice
> filename back (instead of just json:{} garbled things).   But you don't.
>  For protocol drivers, you'll just get the initial filename back.  (So
> my comment was just wrong.)
>
> So what I was thinking about was some case where you specified a normal
> plain filename and qemu would give you back json:{}.  (If rbd
> implemented bdrv_refresh_filename(), that wouldn't happen, because it
> would reconstruct a nice normal filename.)  It turns out, I don't think
> that can happen so easily.  You'll just get your filename back.
>
> Because here's what I'm thinking: If someone uses an option that is
> undocumented and starts with =, well, too bad.  If someone uses a normal
> filename, but gets back a json:{} filename...  Then they are free to use
> that anywhere, and their use of "=" is legitimized.
>
>
> Now that issue kind of reappears when you open an RBD volume, and then
> e.g. take a blockdev-snapshot.  Then your overlay has an overridden
> backing file (one that may be different from what its image header says)
> and its filename may well become a json:{} one (to arrange for the
> overridden backing file options).  Of course, if you opened the RBD
> volume with a filename with some of the options warranting
> =keyvalue-pairs, then your json:{} filename will contain those options
> under =keyvalue-pairs.
>
> So...  I'm not quite sure what I want to say?  I think there are edge
> cases where the user may not have put any weird option into qemu, but
> they do get a json:{} filename with =keyvalue-pairs out of it.  And I
> think users are free to use json:{} filenames qemu spews at them, and we
> can't blame them for it.

Makes sense.

More reason to deprecate key-value pairs in pseudo-filenames.

The only alternative would be to also provide them in QAPI
BlockdevOptionsRbd.  I find that as distasteful as ever, but if the
block maintainers decide we need it, I'll hold my nose.

>>>> If so, and we are comfortable changing the output the way this patch does
>>>> (technically altering ABI anyway), we might as well go all the way and
>>>> filter it out completely.  That would be preferable to cleaning up the json
>>>> output of the internal key/value pairs, IMO.
>>>
>>> Well, this filtering at least is done by my "Fix some filename
>>> generation issues" series.
>> 
>> Likewise.
>
> The series overhauls quite a bit of the bdrv_refresh_filename()
> infrastructure.  That function is also responsible for generating
> json:{} filenames.
>
> One thing it introduces is a BlockDriver field where a driver can
> specify which of the runtime options are actually important.  The rest
> is omitted from the generated json:{} filename.
>
> I may have taken the liberty not to include =keyvalue-pairs in RBD's
> "strong runtime options" list.

I see.

Permit me to digress a bit.

I understand one application for generating a json: filename is for
putting it into an image file header (say as a COW's backing image).

Having image file headers point to other images is not as simple as it
may look at first glance.  The basic case of image format plus plain
filename (not containing '/') is straightforward enough.  But if I make
the filename absolute (with a leading '/'), the image becomes less easy
to move to another machine.

Similarly, certain Ceph configuration bits may make no sense on another
machine, and putting them into an image file header may not be a good
idea.

Rather vague, I'm afraid.

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6277 bytes --]

On 2018-08-16 08:40, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2018-08-15 10:12, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>
>> [...]
>>
>>>> To me personally the issue is that if you can specify a plain filename,
>>>> bdrv_refresh_filename() should give you that plain filename back.  So
>>>> rbd's implementation of that is lacking.  Well, it just doesn't exist.
>>>
>>> I'm not even sure I understand what you're talking about.
>>
>> We have this bdrv_refresh_filename() thing which can do this:
>>
>> $ qemu-img info \
>>     "json:{'driver':'raw',
>>            'file':{'driver':'nbd','host':'localhost'}}"
>> image: nbd://localhost:10809
>> [...]
>>
>> So it can reconstruct a plain filename even if you specify it as options
>> instead of just using a plain filename.
>>
>>
>> Now here's my fault: I thought it might be necessary for a driver to
>> implement that function (which rbd doesn't) so that you'd get a nice
>> filename back (instead of just json:{} garbled things).   But you don't.
>>  For protocol drivers, you'll just get the initial filename back.  (So
>> my comment was just wrong.)
>>
>> So what I was thinking about was some case where you specified a normal
>> plain filename and qemu would give you back json:{}.  (If rbd
>> implemented bdrv_refresh_filename(), that wouldn't happen, because it
>> would reconstruct a nice normal filename.)  It turns out, I don't think
>> that can happen so easily.  You'll just get your filename back.
>>
>> Because here's what I'm thinking: If someone uses an option that is
>> undocumented and starts with =, well, too bad.  If someone uses a normal
>> filename, but gets back a json:{} filename...  Then they are free to use
>> that anywhere, and their use of "=" is legitimized.
>>
>>
>> Now that issue kind of reappears when you open an RBD volume, and then
>> e.g. take a blockdev-snapshot.  Then your overlay has an overridden
>> backing file (one that may be different from what its image header says)
>> and its filename may well become a json:{} one (to arrange for the
>> overridden backing file options).  Of course, if you opened the RBD
>> volume with a filename with some of the options warranting
>> =keyvalue-pairs, then your json:{} filename will contain those options
>> under =keyvalue-pairs.
>>
>> So...  I'm not quite sure what I want to say?  I think there are edge
>> cases where the user may not have put any weird option into qemu, but
>> they do get a json:{} filename with =keyvalue-pairs out of it.  And I
>> think users are free to use json:{} filenames qemu spews at them, and we
>> can't blame them for it.
> 
> Makes sense.
> 
> More reason to deprecate key-value pairs in pseudo-filenames.
> 
> The only alternative would be to also provide them in QAPI
> BlockdevOptionsRbd.  I find that as distasteful as ever, but if the
> block maintainers decide we need it, I'll hold my nose.
> 
>>>>> If so, and we are comfortable changing the output the way this patch does
>>>>> (technically altering ABI anyway), we might as well go all the way and
>>>>> filter it out completely.  That would be preferable to cleaning up the json
>>>>> output of the internal key/value pairs, IMO.
>>>>
>>>> Well, this filtering at least is done by my "Fix some filename
>>>> generation issues" series.
>>>
>>> Likewise.
>>
>> The series overhauls quite a bit of the bdrv_refresh_filename()
>> infrastructure.  That function is also responsible for generating
>> json:{} filenames.
>>
>> One thing it introduces is a BlockDriver field where a driver can
>> specify which of the runtime options are actually important.  The rest
>> is omitted from the generated json:{} filename.
>>
>> I may have taken the liberty not to include =keyvalue-pairs in RBD's
>> "strong runtime options" list.
> 
> I see.
> 
> Permit me to digress a bit.
> 
> I understand one application for generating a json: filename is for
> putting it into an image file header (say as a COW's backing image).

Yes.

(And it's not completely pointless, as there are options you may want to
specify, but cannot do so in a plain filename.  Like host-key-check for
https.)

(And technically you need a string filename to point to when doing
block-commit (Kevin sent patches to remedy this, though), so that could
be called an application as well.)

> Having image file headers point to other images is not as simple as it
> may look at first glance.  The basic case of image format plus plain
> filename (not containing '/') is straightforward enough.  But if I make
> the filename absolute (with a leading '/'), the image becomes less easy
> to move to another machine.

That assumes that we correctly implement relative backing file names.
Believe me, we don't.

For example, say you did this:

$ qemu-img create -f qcow2 foo/bot.qcow2 1M
$ qemu-img create -f qcow2 -b bot.qcow2 foo/mid.qcow2
$ qemu-img create -f qcow2 -b mid.qcow2 foo/top.qcow2

Now try committing top to mid.

You're right, it's impossible right now.

(Not via -b mid.qcow2, nor via -b foo/mid.qcow2, nor via
$PWD/foo/mid.qcow2.  Short of CD-ing to foo/ and then committing to
mid.qcow2, it's just impossible.)

((I've send patches to fix this, but still, it's not like we even got
the case right that's "straightforward enough". :-)))

> Similarly, certain Ceph configuration bits may make no sense on another
> machine, and putting them into an image file header may not be a good
> idea.

On one hand, sure.  Adding json:{} filenames really wasn't one of my
finest hours.  It looked like a simple enough idea at the time, but
they're just not really useful and just keep on biting us.

On another, well, but they may indeed make sense on another machine.
It's like specifying an ssh URL (or absolute mount point on a local
machine, like you describe above).  It may make sense to some (say, you
have a global "content server" or something), so, well.

I mean, our ideal model is that the user just configures the whole
backing chain at runtime and nothing is our fault.  At least that's my
ideal model.  It's always the management layer's fault. O:-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2018-08-16 08:40, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 2018-08-15 10:12, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>> [...]
>>>
>>>>> To me personally the issue is that if you can specify a plain filename,
>>>>> bdrv_refresh_filename() should give you that plain filename back.  So
>>>>> rbd's implementation of that is lacking.  Well, it just doesn't exist.
>>>>
>>>> I'm not even sure I understand what you're talking about.
>>>
>>> We have this bdrv_refresh_filename() thing which can do this:
>>>
>>> $ qemu-img info \
>>>     "json:{'driver':'raw',
>>>            'file':{'driver':'nbd','host':'localhost'}}"
>>> image: nbd://localhost:10809
>>> [...]
>>>
>>> So it can reconstruct a plain filename even if you specify it as options
>>> instead of just using a plain filename.
>>>
>>>
>>> Now here's my fault: I thought it might be necessary for a driver to
>>> implement that function (which rbd doesn't) so that you'd get a nice
>>> filename back (instead of just json:{} garbled things).   But you don't.
>>>  For protocol drivers, you'll just get the initial filename back.  (So
>>> my comment was just wrong.)
>>>
>>> So what I was thinking about was some case where you specified a normal
>>> plain filename and qemu would give you back json:{}.  (If rbd
>>> implemented bdrv_refresh_filename(), that wouldn't happen, because it
>>> would reconstruct a nice normal filename.)  It turns out, I don't think
>>> that can happen so easily.  You'll just get your filename back.
>>>
>>> Because here's what I'm thinking: If someone uses an option that is
>>> undocumented and starts with =, well, too bad.  If someone uses a normal
>>> filename, but gets back a json:{} filename...  Then they are free to use
>>> that anywhere, and their use of "=" is legitimized.
>>>
>>>
>>> Now that issue kind of reappears when you open an RBD volume, and then
>>> e.g. take a blockdev-snapshot.  Then your overlay has an overridden
>>> backing file (one that may be different from what its image header says)
>>> and its filename may well become a json:{} one (to arrange for the
>>> overridden backing file options).  Of course, if you opened the RBD
>>> volume with a filename with some of the options warranting
>>> =keyvalue-pairs, then your json:{} filename will contain those options
>>> under =keyvalue-pairs.
>>>
>>> So...  I'm not quite sure what I want to say?  I think there are edge
>>> cases where the user may not have put any weird option into qemu, but
>>> they do get a json:{} filename with =keyvalue-pairs out of it.  And I
>>> think users are free to use json:{} filenames qemu spews at them, and we
>>> can't blame them for it.
>> 
>> Makes sense.
>> 
>> More reason to deprecate key-value pairs in pseudo-filenames.
>> 
>> The only alternative would be to also provide them in QAPI
>> BlockdevOptionsRbd.  I find that as distasteful as ever, but if the
>> block maintainers decide we need it, I'll hold my nose.
>> 
>>>>>> If so, and we are comfortable changing the output the way this patch does
>>>>>> (technically altering ABI anyway), we might as well go all the way and
>>>>>> filter it out completely.  That would be preferable to cleaning up the json
>>>>>> output of the internal key/value pairs, IMO.
>>>>>
>>>>> Well, this filtering at least is done by my "Fix some filename
>>>>> generation issues" series.
>>>>
>>>> Likewise.
>>>
>>> The series overhauls quite a bit of the bdrv_refresh_filename()
>>> infrastructure.  That function is also responsible for generating
>>> json:{} filenames.
>>>
>>> One thing it introduces is a BlockDriver field where a driver can
>>> specify which of the runtime options are actually important.  The rest
>>> is omitted from the generated json:{} filename.
>>>
>>> I may have taken the liberty not to include =keyvalue-pairs in RBD's
>>> "strong runtime options" list.
>> 
>> I see.
>> 
>> Permit me to digress a bit.
>> 
>> I understand one application for generating a json: filename is for
>> putting it into an image file header (say as a COW's backing image).
>
> Yes.
>
> (And it's not completely pointless, as there are options you may want to
> specify, but cannot do so in a plain filename.  Like host-key-check for
> https.)

Understood.

> (And technically you need a string filename to point to when doing
> block-commit (Kevin sent patches to remedy this, though), so that could
> be called an application as well.)
>
>> Having image file headers point to other images is not as simple as it
>> may look at first glance.  The basic case of image format plus plain
>> filename (not containing '/') is straightforward enough.  But if I make
>> the filename absolute (with a leading '/'), the image becomes less easy
>> to move to another machine.
>
> That assumes that we correctly implement relative backing file names.
> Believe me, we don't.
>
> For example, say you did this:
>
> $ qemu-img create -f qcow2 foo/bot.qcow2 1M
> $ qemu-img create -f qcow2 -b bot.qcow2 foo/mid.qcow2
> $ qemu-img create -f qcow2 -b mid.qcow2 foo/top.qcow2
>
> Now try committing top to mid.
>
> You're right, it's impossible right now.
>
> (Not via -b mid.qcow2, nor via -b foo/mid.qcow2, nor via
> $PWD/foo/mid.qcow2.  Short of CD-ing to foo/ and then committing to
> mid.qcow2, it's just impossible.)
>
> ((I've send patches to fix this, but still, it's not like we even got
> the case right that's "straightforward enough". :-)))

Wait, I carefully defined "straightforward enough" as "filename not
containing '/'".  Did we manage to screw that up, too?

>> Similarly, certain Ceph configuration bits may make no sense on another
>> machine, and putting them into an image file header may not be a good
>> idea.
>
> On one hand, sure.  Adding json:{} filenames really wasn't one of my
> finest hours.  It looked like a simple enough idea at the time, but
> they're just not really useful and just keep on biting us.
>
> On another, well, but they may indeed make sense on another machine.
> It's like specifying an ssh URL (or absolute mount point on a local
> machine, like you describe above).  It may make sense to some (say, you
> have a global "content server" or something), so, well.
>
> I mean, our ideal model is that the user just configures the whole
> backing chain at runtime and nothing is our fault.  At least that's my
> ideal model.  It's always the management layer's fault. O:-)

Hi management layer, I got another buck for you!

I'm currently leaning towards regarding an image header's references to
other images as a convenience feature for users.  Saves restating the
"obvious" (appreciated), until the obvious becomes wrong, possibly
creating confusion (generally less appreciated).  I think having them is
a perfectly reasonable tradeoff overall at least for simple cases.
However, I suspect the more complex things get, the more the value of
"obvious" diminishes, and the more likely confusion becomes.  Mix of
observation and speculation, not a call for action.

Re: [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3604 bytes --]

On 2018-08-20 08:38, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2018-08-16 08:40, Markus Armbruster wrote:

[...]

>> (And technically you need a string filename to point to when doing
>> block-commit (Kevin sent patches to remedy this, though), so that could
>> be called an application as well.)
>>
>>> Having image file headers point to other images is not as simple as it
>>> may look at first glance.  The basic case of image format plus plain
>>> filename (not containing '/') is straightforward enough.  But if I make
>>> the filename absolute (with a leading '/'), the image becomes less easy
>>> to move to another machine.
>>
>> That assumes that we correctly implement relative backing file names.
>> Believe me, we don't.
>>
>> For example, say you did this:
>>
>> $ qemu-img create -f qcow2 foo/bot.qcow2 1M
>> $ qemu-img create -f qcow2 -b bot.qcow2 foo/mid.qcow2
>> $ qemu-img create -f qcow2 -b mid.qcow2 foo/top.qcow2
>>
>> Now try committing top to mid.
>>
>> You're right, it's impossible right now.
>>
>> (Not via -b mid.qcow2, nor via -b foo/mid.qcow2, nor via
>> $PWD/foo/mid.qcow2.  Short of CD-ing to foo/ and then committing to
>> mid.qcow2, it's just impossible.)
>>
>> ((I've send patches to fix this, but still, it's not like we even got
>> the case right that's "straightforward enough". :-)))
> 
> Wait, I carefully defined "straightforward enough" as "filename not
> containing '/'".  Did we manage to screw that up, too?

Well, you were talking about the backing filename.  In the example given
above, none of the backing filenames contain a '/'. O:-)

>>> Similarly, certain Ceph configuration bits may make no sense on another
>>> machine, and putting them into an image file header may not be a good
>>> idea.
>>
>> On one hand, sure.  Adding json:{} filenames really wasn't one of my
>> finest hours.  It looked like a simple enough idea at the time, but
>> they're just not really useful and just keep on biting us.
>>
>> On another, well, but they may indeed make sense on another machine.
>> It's like specifying an ssh URL (or absolute mount point on a local
>> machine, like you describe above).  It may make sense to some (say, you
>> have a global "content server" or something), so, well.
>>
>> I mean, our ideal model is that the user just configures the whole
>> backing chain at runtime and nothing is our fault.  At least that's my
>> ideal model.  It's always the management layer's fault. O:-)
> 
> Hi management layer, I got another buck for you!
> 
> I'm currently leaning towards regarding an image header's references to
> other images as a convenience feature for users.  Saves restating the
> "obvious" (appreciated), until the obvious becomes wrong, possibly
> creating confusion (generally less appreciated).  I think having them is
> a perfectly reasonable tradeoff overall at least for simple cases.
> However, I suspect the more complex things get, the more the value of
> "obvious" diminishes, and the more likely confusion becomes.  Mix of
> observation and speculation, not a call for action.

That's how I'd like to look at it, too, but I'm not sure how reasonable
it is.  Well, it is reasonable as long as we can clearly define for
which use cases it simply isn't enough (*cough* the ones where you need
json:{} *cough*), but I suppose that's too late now.  With json:{} you
unfortunately can shoehorn everything into your image header...
(Which was the point, but you know, path to hell and good intentions and
so on.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]