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

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Wed, Aug 15, 2018 at 10:12:12AM +0200, Markus Armbruster wrote:
>> Max Reitz <address@hidden> 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/address@hidden: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.