Re: [PATCH] block/curl: use strlen instead of strchr

[Michael Tokarev]

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

--
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