| Message ID | 20240628054942.657397-1-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | block/curl: use strlen instead of strchr | expand |
On 6/28/24 08:49, Vladimir Sementsov-Ogievskiy wrote: > We already know where colon is, so no reason to search for it. Also, > avoid a code, which looks like we forget to check return value of > strchr() to NULL. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > > This replaces my patch > [PATCH] block/curl: explicitly assert that strchr returns non-NULL value > Supersedes: <20240627153059.589070-1-vsementsov@yandex-team.ru> > > block/curl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/curl.c b/block/curl.c > index 419f7c89ef..d03bfe4817 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -219,7 +219,7 @@ 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 = header + strlen(accept_ranges); > > /* Skip whitespace between the header name and value. */ > while (p < end && *p && g_ascii_isspace(*p)) { Heck. All these strlen()s look ugly, especially in the loop iterations.. To my taste anyway. How about this instead: diff --git a/block/curl.c b/block/curl.c index 419f7c89ef..5e91807115 100644 --- a/block/curl.c +++ b/block/curl.c @@ -210,35 +210,34 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque) { BDRVCURLState *s = opaque; size_t realsize = size * nmemb; - const char *header = (char *)ptr; - const char *end = header + realsize; - const char *accept_ranges = "accept-ranges:"; - const char *bytes = "bytes"; + const char *p = (char *)ptr; + const char *end = p + realsize; + const char *t; - if (realsize >= strlen(accept_ranges) - && g_ascii_strncasecmp(header, accept_ranges, - strlen(accept_ranges)) == 0) { + for (t = "accept-ranges:"; p < end && *t; ++p, ++t) { + if (g_ascii_tolower(*p) != *t) { + return realsize; + } + } - char *p = strchr(header, ':') + 1; + /* Skip whitespace between the header name and value. */ + while (p < end && *p && g_ascii_isspace(*p)) { + p++; + } - /* Skip whitespace between the header name and value. */ - while (p < end && *p && g_ascii_isspace(*p)) { - p++; + for (t = "bytes"; p < end && *t; ++p, ++t) { + if (g_ascii_tolower(*p) != *t) { + return realsize; } + } - if (end - p >= strlen(bytes) - && strncmp(p, bytes, strlen(bytes)) == 0) { - - /* Check that there is nothing but whitespace after the value. */ - p += strlen(bytes); - while (p < end && *p && g_ascii_isspace(*p)) { - p++; - } + /* Check that there is nothing but whitespace after the value. */ + while (p < end && *p && g_ascii_isspace(*p)) { + p++; + } - if (p == end || !*p) { - s->accept_range = true; - } - } + if (p == end || !*p) { + s->accept_range = true; } return realsize; Whole function for easy read: /* Called from curl_multi_do_locked, with s->mutex held. */ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque) { BDRVCURLState *s = opaque; size_t realsize = size * nmemb; const char *p = (char *)ptr; const char *end = p + realsize; const char *t; for (t = "accept-ranges:"; p < end && *t; ++p, ++t) { if (g_ascii_tolower(*p) != *t) { return realsize; } } /* Skip whitespace between the header name and value. */ while (p < end && *p && g_ascii_isspace(*p)) { p++; } for (t = "bytes"; p < end && *t; ++p, ++t) { if (g_ascii_tolower(*p) != *t) { return realsize; } } /* Check that there is nothing but whitespace after the value. */ while (p < end && *p && g_ascii_isspace(*p)) { p++; } if (p == end || !*p) { s->accept_range = true; } return realsize; } Dunno why it also checks for *p being !=0 in the last part, but ok. Sorry for the noise. But I dislike usage of strlen() etc like this :) /mjt
On 6/29/24 09:20, Michael Tokarev wrote: > On 6/28/24 08:49, Vladimir Sementsov-Ogievskiy wrote: >> We already know where colon is, so no reason to search for it. Also, >> avoid a code, which looks like we forget to check return value of >> strchr() to NULL. >> >> Suggested-by: Kevin Wolf <kwolf@redhat.com> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> >> This replaces my patch >> [PATCH] block/curl: explicitly assert that strchr returns non-NULL value >> Supersedes: <20240627153059.589070-1-vsementsov@yandex-team.ru> >> >> block/curl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/curl.c b/block/curl.c >> index 419f7c89ef..d03bfe4817 100644 >> --- a/block/curl.c >> +++ b/block/curl.c >> @@ -219,7 +219,7 @@ 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 = header + strlen(accept_ranges); >> /* Skip whitespace between the header name and value. */ >> while (p < end && *p && g_ascii_isspace(*p)) { > > Heck. All these strlen()s look ugly, especially in the > loop iterations.. To my taste anyway. > > How about this instead: Or even this: diff --git a/block/curl.c b/block/curl.c index 419f7c89ef..982597ea7f 100644 --- a/block/curl.c +++ b/block/curl.c @@ -210,37 +210,28 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque) { BDRVCURLState *s = opaque; size_t realsize = size * nmemb; - const char *header = (char *)ptr; - const char *end = header + realsize; - const char *accept_ranges = "accept-ranges:"; - const char *bytes = "bytes"; - - if (realsize >= strlen(accept_ranges) - && g_ascii_strncasecmp(header, accept_ranges, - strlen(accept_ranges)) == 0) { - - char *p = strchr(header, ':') + 1; - - /* Skip whitespace between the header name and value. */ - while (p < end && *p && g_ascii_isspace(*p)) { - p++; - } - - if (end - p >= strlen(bytes) - && strncmp(p, bytes, strlen(bytes)) == 0) { - - /* Check that there is nothing but whitespace after the value. */ - p += strlen(bytes); - while (p < end && *p && g_ascii_isspace(*p)) { - p++; - } - - if (p == end || !*p) { - s->accept_range = true; + const char *p = (char *)ptr; + const char *end = p + realsize; + const char *t = "accept-ranges : bytes "; + + while (p < end && *t) { + if (*t == ' ') { + if (g_ascii_isspace(*p)) { + ++p; + } else { + ++t; } + } else if (*t == g_ascii_tolower(*p)) { + ++p, ++t; + } else { + break; } } + if (p == end || !*p) { + s->accept_range = true; + } + return realsize; } Whole thing: static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque) { BDRVCURLState *s = opaque; size_t realsize = size * nmemb; const char *p = (char *)ptr; const char *end = p + realsize; const char *t = "accept-ranges : bytes "; while (p < end && *t) { if (*t == ' ') { if (g_ascii_isspace(*p)) { ++p; } else { ++t; } } else if (*t == g_ascii_tolower(*p)) { ++p, ++t; } else { break; } } if (p == end || !*p) { s->accept_range = true; } return realsize; } /mjt
On 6/29/24 09:36, Michael Tokarev wrote: .. > + while (p < end && *t) { > + if (*t == ' ') { > + if (g_ascii_isspace(*p)) { > + ++p; > + } else { > + ++t; > } > + } else if (*t == g_ascii_tolower(*p)) { > + ++p, ++t; > + } else { > + break; > } > } > > + if (p == end || !*p) { if (!*t && p == end) ofc. /mjt
On 29.06.24 09:20, Michael Tokarev wrote: > On 6/28/24 08:49, Vladimir Sementsov-Ogievskiy wrote: >> We already know where colon is, so no reason to search for it. Also, >> avoid a code, which looks like we forget to check return value of >> strchr() to NULL. >> >> Suggested-by: Kevin Wolf <kwolf@redhat.com> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> >> This replaces my patch >> [PATCH] block/curl: explicitly assert that strchr returns non-NULL value >> Supersedes: <20240627153059.589070-1-vsementsov@yandex-team.ru> >> >> block/curl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/curl.c b/block/curl.c >> index 419f7c89ef..d03bfe4817 100644 >> --- a/block/curl.c >> +++ b/block/curl.c >> @@ -219,7 +219,7 @@ 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 = header + strlen(accept_ranges); >> /* Skip whitespace between the header name and value. */ >> while (p < end && *p && g_ascii_isspace(*p)) { > > Heck. All these strlen()s look ugly, especially in the > loop iterations.. I expect that strlen of string constant is calculated in compilation time. My aim was to fix Coverity complain (I don't see this complain in public qemu coverity project, that's why I don't specify CID in commit message), not to rewrite the whole function. So I'd prefer Kevein's suggesting which is both minimal and makes the code obviously correct.. The only simpler thing is to mark the problem false-positive in Coverity.
On 01.07.24 09:34, Vladimir Sementsov-Ogievskiy wrote: > On 29.06.24 09:20, Michael Tokarev wrote: >> On 6/28/24 08:49, Vladimir Sementsov-Ogievskiy wrote: >>> We already know where colon is, so no reason to search for it. Also, >>> avoid a code, which looks like we forget to check return value of >>> strchr() to NULL. >>> >>> Suggested-by: Kevin Wolf <kwolf@redhat.com> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> --- >>> >>> This replaces my patch >>> [PATCH] block/curl: explicitly assert that strchr returns non-NULL value >>> Supersedes: <20240627153059.589070-1-vsementsov@yandex-team.ru> >>> >>> block/curl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/block/curl.c b/block/curl.c >>> index 419f7c89ef..d03bfe4817 100644 >>> --- a/block/curl.c >>> +++ b/block/curl.c >>> @@ -219,7 +219,7 @@ 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 = header + strlen(accept_ranges); >>> /* Skip whitespace between the header name and value. */ >>> while (p < end && *p && g_ascii_isspace(*p)) { >> >> Heck. All these strlen()s look ugly, especially in the >> loop iterations.. > > I expect that strlen of string constant is calculated in compilation time. > > My aim was to fix Coverity complain (I don't see this complain in public qemu coverity project, that's why I don't specify CID in commit message), not to rewrite the whole function. So I'd prefer Kevein's suggesting which is both minimal and makes the code obviously correct.. The only simpler thing is to mark the problem false-positive in Coverity. > Upd: I missed that you sent a patch, this changes things. Of course, you code looks nicer than old one.
diff --git a/block/curl.c b/block/curl.c index 419f7c89ef..d03bfe4817 100644 --- a/block/curl.c +++ b/block/curl.c @@ -219,7 +219,7 @@ 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 = header + strlen(accept_ranges); /* Skip whitespace between the header name and value. */ while (p < end && *p && g_ascii_isspace(*p)) {
We already know where colon is, so no reason to search for it. Also, avoid a code, which looks like we forget to check return value of strchr() to NULL. Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- This replaces my patch [PATCH] block/curl: explicitly assert that strchr returns non-NULL value Supersedes: <20240627153059.589070-1-vsementsov@yandex-team.ru> block/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)