[riot-notifications] [RIOT-OS/RIOT] uri_parser: provide function to split query (#16695)

benpicco notifications at github.com
Tue Aug 3 16:36:52 CEST 2021


@benpicco commented on this pull request.



> +    char *name;                     /**< name of the query parameter */
+    char *value;                    /**< value of the query parameter */

```suggestion
   const char *name;                /**< name of the query parameter */
   const char *value;               /**< value of the query parameter */
```

> +            case '&':
+                if (params[idx].value == NULL) {
+                    /* we should have picked up a parameter value by now */
+                    return -1;
+                }
+                params[idx].value_len = c - params[idx].value;
+                if (*c == '#') {
+                    /* we've reached the end of the query string, next comes an
+                     * anchor, enforce end of loop
+                     * XXX: can be removed when uri_parser has anchor support */
+                    c = query_end;
+                }
+                else if ((idx + 1) < params_len) {
+                    /* c is an ampersand (&), so mark the next char as the next
+                     * parameter's name name */
+                    params[++idx].name = (char *)c + 1U;

```suggestion
                    params[++idx].name = c + 1U;
```

Why needlessly cast away the `const` parameter? I see this is going on a lot in this file - but it can be entirely avoided. 

<details><summary>see this patch</summary>

```patch
diff --git a/sys/include/uri_parser.h b/sys/include/uri_parser.h
index 4c688450e8..793f4b0c11 100644
--- a/sys/include/uri_parser.h
+++ b/sys/include/uri_parser.h
@@ -39,8 +39,8 @@ extern "C" {
  * @brief container that holds all results
  */
 typedef struct {
-    char *scheme;                   /**< scheme */
-    char *userinfo;                 /**< userinfo */
+    const char *scheme;             /**< scheme */
+    const char *userinfo;           /**< userinfo */
 
     /**
      * @brief host part
@@ -49,7 +49,7 @@ typedef struct {
      * '[' and ']' as well as the zoneid (with leading '%'), if
      * present.
      */
-    char *host;
+    const char *host;
 
     /**
      * @brief Pointer to the start of the address, if @ref host is an
@@ -58,18 +58,18 @@ typedef struct {
      * @note @ref ipv6addr does not include the brackets '[' and ']'
      * and the zoneid part.
      */
-    char *ipv6addr;
+    const char *ipv6addr;
 
     /**
      * @brief zoneid if @ref host is IPv6 address, NULL otherwise
      *
      * @see https://tools.ietf.org/html/rfc6874
      */
-    char *zoneid;
+    const char *zoneid;
 
-    char *port;                     /**< port */
-    char *path;                     /**< path */
-    char *query;                    /**< query */
+    const char *port;               /**< port */
+    const char *path;               /**< path */
+    const char *query;              /**< query */
     uint16_t scheme_len;            /**< length of @ref scheme */
     uint16_t userinfo_len;          /**< length of @ref userinfo */
     uint16_t host_len;              /**< length of @ref host */
@@ -84,8 +84,8 @@ typedef struct {
  * @brief   Container to represent a query parameter
  */
 typedef struct {
-    char *name;                     /**< name of the query parameter */
-    char *value;                    /**< value of the query parameter */
+    const char *name;               /**< name of the query parameter */
+    const char *value;              /**< value of the query parameter */
     uint16_t name_len;              /**< length of @ref name */
     uint16_t value_len;             /**< length of @ref value */
 } uri_parser_query_param_t;
diff --git a/sys/uri_parser/uri_parser.c b/sys/uri_parser/uri_parser.c
index 2229191005..5185889f15 100644
--- a/sys/uri_parser/uri_parser.c
+++ b/sys/uri_parser/uri_parser.c
@@ -26,7 +26,7 @@
 #include "debug.h"
 
 /* strchr for non-Null-terminated strings (buffers) */
-static char *_strchrb(char *start, char *stop, char c)
+static const char *_strchrb(const char *start, const char *stop, char c)
 {
     for (; start < stop; start++) {
         if (*start == c) {
@@ -36,8 +36,8 @@ static char *_strchrb(char *start, char *stop, char c)
     return NULL;
 }
 
-static char *_consume_scheme(uri_parser_result_t *result, char *uri,
-                             char *uri_end, bool *has_authority)
+static const char *_consume_scheme(uri_parser_result_t *result, const char *uri,
+                                   const char *uri_end, bool *has_authority)
 {
     assert(uri);
 
@@ -49,7 +49,7 @@ static char *_consume_scheme(uri_parser_result_t *result, char *uri,
         return NULL;
     }
 
-    char *p = _strchrb(uri, uri_end, ':');
+    const char *p = _strchrb(uri, uri_end, ':');
 
     result->scheme = uri;
     result->scheme_len = p - uri;
@@ -65,11 +65,11 @@ static char *_consume_scheme(uri_parser_result_t *result, char *uri,
     return p + 1;
 }
 
-void _consume_userinfo(uri_parser_result_t *result, char *uri,
-                       char *authority_end)
+void _consume_userinfo(uri_parser_result_t *result, const char *uri,
+                       const char *authority_end)
 {
     /* check for userinfo within authority */
-    char *userinfo_end = _strchrb(uri, authority_end, '@');
+    const char *userinfo_end = _strchrb(uri, authority_end, '@');
 
     /* check if match */
     if (userinfo_end) {
@@ -88,14 +88,14 @@ void _consume_userinfo(uri_parser_result_t *result, char *uri,
     }
 }
 
-bool _consume_port(uri_parser_result_t *result, char *ipv6_end,
-                   char *authority_end)
+bool _consume_port(uri_parser_result_t *result, const char *ipv6_end,
+                   const char *authority_end)
 {
     /* check for port after host part */
-    char *port_begin = NULL;
+    const char *port_begin = NULL;
     /* repeat until last ':' in authority section */
     /* if ipv6 address, check after ipv6 end marker */
-    char *p = (ipv6_end ? ipv6_end : result->host);
+    const char *p = (ipv6_end ? ipv6_end : result->host);
     while (p != NULL && (p < authority_end)) {
         port_begin = p;
         p = _strchrb(p + 1, authority_end, ':');
@@ -116,13 +116,13 @@ bool _consume_port(uri_parser_result_t *result, char *ipv6_end,
     return true;
 }
 
-static char *_consume_authority(uri_parser_result_t *result, char *uri,
-                                char *uri_end)
+static const char *_consume_authority(uri_parser_result_t *result, const char *uri,
+                                      const char *uri_end)
 {
     assert(uri);
 
     /* search until first '/' */
-    char *authority_end = _strchrb(uri, uri_end, '/');
+    const char *authority_end = _strchrb(uri, uri_end, '/');
     if (!authority_end) {
         authority_end = uri_end;
     }
@@ -137,7 +137,7 @@ static char *_consume_authority(uri_parser_result_t *result, char *uri,
         return authority_end;
     }
 
-    char *ipv6_end = NULL;
+    const char *ipv6_end = NULL;
     /* validate IPv6 form */
     if (result->host[0] == '[') {
         ipv6_end = _strchrb(result->host, uri_end, ']');
@@ -146,7 +146,7 @@ static char *_consume_authority(uri_parser_result_t *result, char *uri,
             return NULL;
         }
 
-        char *zoneid_start = _strchrb(result->host, ipv6_end, '%');
+        const char *zoneid_start = _strchrb(result->host, ipv6_end, '%');
         if (zoneid_start) {
             /* skip % */
             result->zoneid = zoneid_start + 1;
@@ -175,8 +175,8 @@ static char *_consume_authority(uri_parser_result_t *result, char *uri,
     return authority_end;
 }
 
-static char *_consume_path(uri_parser_result_t *result, char *uri,
-                           char *uri_end)
+static const char *_consume_path(uri_parser_result_t *result, const char *uri,
+                                 const char *uri_end)
 {
     assert(uri);
 
@@ -184,7 +184,7 @@ static char *_consume_path(uri_parser_result_t *result, char *uri,
     result->path_len = (uri_end - uri);
 
     /* check for query start '?' */
-    char *path_end = _strchrb(uri, uri_end, '?');
+    const char *path_end = _strchrb(uri, uri_end, '?');
 
     /* no query string found, return! */
     if (!path_end) {
@@ -201,8 +201,8 @@ static char *_consume_path(uri_parser_result_t *result, char *uri,
     return (result->query + result->query_len);
 }
 
-static int _parse_relative(uri_parser_result_t *result, char *uri,
-                           char *uri_end)
+static int _parse_relative(uri_parser_result_t *result, const char *uri,
+                           const char *uri_end)
 {
     uri = _consume_path(result, uri, uri_end);
     /* uri should point to uri_end, otherwise there's something left
@@ -214,8 +214,8 @@ static int _parse_relative(uri_parser_result_t *result, char *uri,
     return 0;
 }
 
-static int _parse_absolute(uri_parser_result_t *result, char *uri,
-                           char *uri_end)
+static int _parse_absolute(uri_parser_result_t *result, const char *uri,
+                           const char *uri_end)
 {
     bool has_authority;
 
@@ -246,7 +246,7 @@ static int _parse_absolute(uri_parser_result_t *result, char *uri,
 
 bool uri_parser_is_absolute(const char *uri, size_t uri_len)
 {
-    char *colon = _strchrb((char *)uri, (char *)(uri + uri_len), ':');
+    const char *colon = _strchrb(uri, (uri + uri_len), ':');
 
     /* potentially absolute, if ':' exists */
     if (colon) {
@@ -292,10 +292,10 @@ int uri_parser_process(uri_parser_result_t *result, const char *uri,
     memset(result, 0, sizeof(*result));
 
     if (uri_parser_is_absolute(uri, uri_len)) {
-        return _parse_absolute(result, (char *)uri, (char *)(uri + uri_len));
+        return _parse_absolute(result, uri, (uri + uri_len));
     }
     else {
-        return _parse_relative(result, (char *)uri, (char *)(uri + uri_len));
+        return _parse_relative(result, uri, (uri + uri_len));
     }
 
     return 0;
@@ -343,7 +343,7 @@ int uri_parser_split_query(const uri_parser_result_t *uri,
                 else if ((idx + 1) < params_len) {
                     /* c is an ampersand (&), so mark the next char as the next
                      * parameter's name name */
-                    params[++idx].name = (char *)c + 1U;
+                    params[++idx].name = c + 1U;
                     assert(params[idx].name_len == 0);
                 }
                 else {
@@ -361,7 +361,7 @@ int uri_parser_split_query(const uri_parser_result_t *uri,
                 }
                 params[idx].name_len = c - params[idx].name;
                 /* pick next char as start of value */
-                params[idx].value = (char *)c + 1U;
+                params[idx].value = c + 1U;
                 /* make sure the precondition on params is met */
                 assert(params[idx].value_len == 0);
                 break;
```
</details>

> +    TEST_ASSERT_EQUAL_INT(-1, res);
+
+    INIT_URI_RESULTS("=&&");
+    res = uri_parser_split_query(&_uri_results, _params, ARRAY_SIZE(_params));
+    TEST_ASSERT_EQUAL_INT(-1, res);
+
+    INIT_URI_RESULTS("==");
+    res = uri_parser_split_query(&_uri_results, _params, ARRAY_SIZE(_params));
+    TEST_ASSERT_EQUAL_INT(-1, res);
+
+    INIT_URI_RESULTS("key=value&name=value=another");
+    res = uri_parser_split_query(&_uri_results, _params, ARRAY_SIZE(_params));
+    TEST_ASSERT_EQUAL_INT(-1, res);
+}
+
+

```suggestion
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16695#pullrequestreview-721267702
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210803/aab9964c/attachment-0001.htm>


More information about the notifications mailing list