Re: [PATCH] block/curl: explicitly assert that strchr returns non-NULL value

[Vladimir Sementsov-Ogievskiy]

On 27.06.24 21:05, Kevin Wolf wrote:
> Am 27.06.2024 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > strchr may return NULL if colon is not found. It seems clearer to
> > assert explicitly that we don't expect it here, than dereference 1 in
> > the next line.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >   block/curl.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/curl.c b/block/curl.c
> > index 419f7c89ef..ccfffd6c12 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t 
> > nmemb, void *opaque)
> >           && g_ascii_strncasecmp(header, accept_ranges,
> >                                  strlen(accept_ranges)) == 0) {
> >   
> > -        char *p = strchr(header, ':') + 1;
> > +        char *p = strchr(header, ':');
> > +        assert(p != NULL);
> > +        p += 1;
>
> I'm not sure if this is actually much clearer because it doesn't say why
> we don't expect NULL here. If you don't look at the context, it almost
> looks like an assert() where proper error handling is needed. If you do,
> then the original line is clear enough.
>
> My first thought was that maybe what we want is a comment, but we
> actually already know where the colon is. So how about this instead:
>
>      char *p = header + strlen(accept_ranges);

Oh, right. That's better.

> >           /* Skip whitespace between the header name and value. */
> >           while (p < end && *p && g_ascii_isspace(*p)) {
> > --
> > 2.34.1

--
Best regards,
Vladimir