block/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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)) {
--
2.34.1On 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
--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
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.
--
Best regards,
Vladimir
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.
--
Best regards,
Vladimir
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
--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
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
© 2016 - 2024 Red Hat, Inc.