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

[Markus Armbruster <armbru@redhat.com>]

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?