Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper

[Max Reitz]

On 09.04.21 16:05, Connor Kuehl wrote:
> On 4/6/21 9:24 AM, Max Reitz wrote:
> > On 01.04.21 23:01, Connor Kuehl wrote:
> > > [..]
> > > diff --git a/block/rbd.c b/block/rbd.c
> > > index 9071a00e3f..c0e4d4a952 100644
> > > --- a/block/rbd.c
> > > +++ b/block/rbd.c
> > > @@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char 
> > > delim, char **p)
> > >       return src;
> > >   }
> > > +static char *qemu_rbd_strchr(char *src, char delim)
> > > +{
> > > +    char *p;
> > > +
> > > +    for (p = src; *p; ++p) {
> > > +        if (*p == delim) {
> > > +            return p;
> > > +        }
> > > +        if (*p == '\\') {
> > > +            ++p;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> >
> > So I thought you could make qemu_rbd_do_next_tok() to do this.  (I 
> > didn’t say you should, but bear with me.)  That would be possible by 
> > giving it a new parameter (e.g. @find), and if that is set, return 
> > @end if *end == delim after the loop, and NULL otherwise.
> >
> > Now, if you add wrapper functions to make it nice, there’s not much 
> > more difference in lines added compared to just adding a new function, 
> > but it does mean your function should basically be the same as 
> > qemu_rbd_next_tok(), except that no splitting happens, that there is 
> > no *p, and that @end is returned instead of @src.
>
> Do you have a strong preference for this? I agree that 
> qemu_rbd_next_tok() could grow this functionality, but I think it'd be 
> simpler to keep it separate in the form of qemu_rbd_strchr().

Oh, no, no.  I mostly said this so it would be clear why both functions 
should basically have the same structure, i.e. why a difference in 
structure might be a sign that something’s wrong.

Sorry if I came across as too verbose.

> > So there is one difference, and that is that qemu_rbd_next_tok() has 
> > this condition to skip escaped characters:
> >
> >      if (*end == '\\' && end[1] != '\0') {
> >
> > where qemu_rbd_strchr() has only:
> >
> >      if (*p == '\\') {
> >
> > And I think qemu_rbd_next_tok() is right; if the string in question 
> > has a trailing backslash, qemu_rbd_strchr() will ignore the final NUL 
> > and continue searching past the end of the string.
>
> Aha, good catch. I'll fix this up.

Thanks!

Max