Jeff Cody <jc...@redhat.com> writes: > On Wed, Aug 15, 2018 at 10:12:12AM +0200, Markus Armbruster wrote: >> 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? > > 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.