On Thu, Mar 30, 2017 at 09:08:20PM -0500, Eric Blake wrote: > On 03/30/2017 08:22 PM, Eric Blake wrote: > > On 03/30/2017 08:05 PM, Alexandru Avadanii wrote: > >> c7cacb3e7a2e9fdf929c993b98268e4179147cbb is the first bad commit > >> block/rbd: parse all options via bdrv_parse_filename > > > > Yep, my bisect finished about 2 minutes after your email on the same > > spot. I'm working on a patch. I can reproduce the problem with a mere: > > > > ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio > > -drive > > 'file=rbd:volumes/volume-ea141b5c-cdb3-4765-910d-e7008b209a70:id=compute:key=AQAVkvxXAAAAABAA9ZxWFYdRmV+DSwKr7BKKXg==:auth_supported=cephx\;none:mon_host=192.168.1.2\:6789,format=raw,if=none,id=drive-virtio-disk0,serial=ea141b5c-cdb3-4765-910d-e7008b209a70,cache=writeback' > > > > the good behavior (on my setup) just hangs trying to connect to a > > non-existent machine, the bad behavior gets rather-badly misparsed > > (splitting the escaped : in the host:port portion as if the port were > > the next key-value pair) resulting in an instant error message. I don't > > have an actual RBD setup for testing the fix, but will cc you on the > > patch that I propose once I have something. > > I found the culprit, but the patch is taking me longer. > > We are unescaping key="mon_host", value="192.168.1.2\:6789" when we > first parse the key/value pairs in qemu_rbd_parse_filename(), then we > slam the unescaped values back into a single string with ':' separators > resulting in "mon_host=192.168.1.2:6789" for stuffing through the QDict, > then we pass that string BACK through qemu_rbd_next_tok() inside > qemu_rbd_set_keypairs(), and since we no longer have the \: escape, we > are trying to treat 6789 as a new key on the second pass. Moral of the > story: don't parse stuff twice. > > My patch will be to use a QList instead of a QString for the hidden > "=keyvalue-pairs" object that we use to pass things around between > parsing the filename and actually passing parameters to RBD, matching > the fact that we already have this telling comment: > > /* FIXME: This is pretty ugly, and not the right way to do this. > * These should be contained in a structure, and then > * passed explicitly as individual key/value pairs to > * rados. Consider this legacy code that needs to be > * updated. */
Ugh. Yep, I just got done coming to the same conclusion, only to hop on the email list to see that you did as well. A QList sounds like a good choice. Jeff
