Max Reitz <mre...@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?