Re: [PATCH 2/5] util/uri: Simplify uri_string_unescape()

[Stefan Weil]

Am 22.01.24 um 20:17 schrieb Thomas Huth:

> uri_string_unescape() basically does the same as the glib function
> g_uri_unescape_string(), with just an additional length parameter.
> So we can simplify this function a lot by limiting the length with
> g_strndup() first and then by calling g_uri_unescape_string() instead
> of walking through the string manually.
>
> Suggested-by: Stefan Weil <stefan.weil@weilnetz.de>
>

Can my e-mail address be replaced by another one (sw@weilnetz.de)?

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  util/uri.c | 49 +++----------------------------------------------
>  1 file changed, 3 insertions(+), 46 deletions(-)
>
> diff --git a/util/uri.c b/util/uri.c
> index 33b6c7214e..2a75f535ba 100644
> --- a/util/uri.c
> +++ b/util/uri.c
> @@ -1561,15 +1561,6 @@ done_cd:
>      return 0;
>  }
>  
> -static int is_hex(char c)
> -{
> -    if (((c >= '0') && (c <= '9')) || ((c >= 'a') && (c <= 'f')) ||
> -        ((c >= 'A') && (c <= 'F'))) {
> -        return 1;
> -    }
> -    return 0;
> -}
> -
>  /**
>   * uri_string_unescape:
>   * @str:  the string to unescape
> @@ -1585,8 +1576,7 @@ static int is_hex(char c)
>   */
>  char *uri_string_unescape(const char *str, int len)
>  {
> -    char *ret, *out;
> -    const char *in;
> +    g_autofree char *lstr = NULL;
>

Is it necessary to assign NULL? It does not look so.

>  
>      if (str == NULL) {
>          return NULL;
> @@ -1594,42 +1584,9 @@ char *uri_string_unescape(const char *str, int len)
>      if (len <= 0) {
>          len = strlen(str);
>      }
> -    if (len < 0) {
> -        return NULL;
> -    }
> -
> -    ret = g_malloc(len + 1);
> +    lstr = g_strndup(str, len);
>  
> -    in = str;
> -    out = ret;
> -    while (len > 0) {
> -        if ((len > 2) && (*in == '%') && (is_hex(in[1])) && (is_hex(in[2]))) {
> -            in++;
> -            if ((*in >= '0') && (*in <= '9')) {
> -                *out = (*in - '0');
> -            } else if ((*in >= 'a') && (*in <= 'f')) {
> -                *out = (*in - 'a') + 10;
> -            } else if ((*in >= 'A') && (*in <= 'F')) {
> -                *out = (*in - 'A') + 10;
> -            }
> -            in++;
> -            if ((*in >= '0') && (*in <= '9')) {
> -                *out = *out * 16 + (*in - '0');
> -            } else if ((*in >= 'a') && (*in <= 'f')) {
> -                *out = *out * 16 + (*in - 'a') + 10;
> -            } else if ((*in >= 'A') && (*in <= 'F')) {
> -                *out = *out * 16 + (*in - 'A') + 10;
> -            }
> -            in++;
> -            len -= 3;
> -            out++;
> -        } else {
> -            *out++ = *in++;
> -            len--;
> -        }
> -    }
> -    *out = 0;
> -    return ret;
> +    return g_uri_unescape_string(lstr, NULL);
>  }
>  
>  /**
>

Thank you.

Reviewed-by: Stefan Weil <sw@weilnetz.de>