Re: [Qemu-devel] rbd: Possible regression in 2.9 RCs

[Jeff Cody]

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