Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

[Connor Kuehl <ckuehl@redhat.com>]

On 4/1/21 12:24 PM, Max Reitz wrote:
> On 01.04.21 17:52, Connor Kuehl wrote:
>> That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
>>
>> Furthermore, this was causing some confusion in the parsing logic to
>> where the caller might test for the presence of a character to split on
>> like so:
>>
>> if (strchr(image_name, '/')) {
>>          found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
>>     [..]
>>
>> When qemu_rbd_next_tok() performs unescaping as a side effect, the
>> parser can get confused thinking that it can split this string, but
>> really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
>> "finds" the delimiter and consumes the rest of the token input stream.
> 
> I don’t fully understand.  I understand the strchr() problem and the 
> thing you explain next.  But I would have thought that’s a problem of 
> using strchr(), i.e. that we’d need a custom function to do strchr() but 
> consider escaped characters.  If it’s just about true/false like in your 
> example, it could be a new version of qemu_rbd_next_tok() that does not 
> modify the string.

I went back and forth a lot on the different ways this can be fixed 
before sending this, and I agree the most consistent fix here would be 
to add an escape-aware strchr. Initially, adding another libc-like 
function with more side effects wasn't as appealing to me as removing 
the side effects entirely to separate mechanism vs. policy. Your example 
below convinced me that this patch would split the token in unexpected 
ways. I'll send a v2 :-)

Thanks,

Connor

> [..]
> Should it?  I would have fully expected it to not be split and the 
> parser complains that the input is invalid.  Or, let’s say, 
> "foo\/bar/baz” should be split into “foo\/bar” and “baz”.