[PATCH wayland] wl_strtol and wl_strtoul utility functions are added (inlined patch)

[Bryce Harrington]

On Wed, Oct 15, 2014 at 04:18:59PM +0300, Imran Zaman wrote:
> Hi
> 
> The patch is used to replace strtol and strtoul with wl_strtol and
> wl_strtoul with inputs and result checks.
> 
> The utility functions are used extensively in wayland and weston so added

These utility...

> appropriate
> input and output checks; test cases are also updated; will push the patch
> for weston as well.
> ----

Looks like this'll be a nice cleanup.

> 
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index 809130b..3e30fe7 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -315,7 +315,6 @@ start_element(void *data, const char *element_name,
> const char **atts)
>   struct description *description;
>   const char *name, *type, *interface_name, *value, *summary, *since;
>   const char *allow_null;
> - char *end;
>   int i, version;
> 
>   ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser);
> @@ -404,9 +403,7 @@ start_element(void *data, const char *element_name,
> const char **atts)
>   message->destructor = 0;
> 
>   if (since != NULL) {
> - errno = 0;
> - version = strtol(since, &end, 0);
> - if (errno == EINVAL || end == since || *end != '\0')
> + if (!wl_strtol(since, NULL, 0, &version))
>   fail(&ctx->loc,
>       "invalid integer (%s)\n", since);
>   } else {
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index b0f77b9..1229b5f 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -824,13 +824,12 @@ wl_display_connect_to_fd(int fd)
>  WL_EXPORT struct wl_display *
>  wl_display_connect(const char *name)
>  {
> - char *connection, *end;
> + char *connection;
>   int flags, fd;
> 
>   connection = getenv("WAYLAND_SOCKET");
>   if (connection) {
> - fd = strtol(connection, &end, 0);
> - if (*end != '\0')
> + if (!wl_strtol(connection, NULL, 0, &fd))
>   return NULL;
> 
>   flags = fcntl(fd, F_GETFD);
> diff --git a/src/wayland-util.c b/src/wayland-util.c
> index b099882..f8267f3 100644
> --- a/src/wayland-util.c
> +++ b/src/wayland-util.c
> @@ -26,6 +26,9 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <stdarg.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <stdlib.h>

Looks like stdlib.h is already included in wayland-util.c

>  #include "wayland-util.h"
>  #include "wayland-private.h"
> @@ -361,6 +364,42 @@ wl_map_for_each(struct wl_map *map, wl_iterator_func_t
> func, void *data)
>   for_each_helper(&map->server_entries, func, data);
>  }
> 
> +WL_EXPORT int
> +wl_strtol(const char *str, char **endptr, int base, int32_t *val)

For consistency with strtol shouldn't the final arg be type long?

> +{
> + char *end = NULL;
> + long v;
> +
> + if (!str || !val) return 0;
> + if (!endptr) endptr = &end;
> +
> + errno = 0;
> + v = strtol(str, endptr, base);
> + if (errno != 0 || *endptr == str || **endptr != '\0')
> + return 0;
> +
> + *val = v;

Here's a type conversion between long and int32_t.
(Which are both the same thing, but not guaranteed...)

> + return 1;
> +}

Following the recent return type discussions, maybe consider returning
bool (true|false) rather than int.  Looks like this function needn't
return anything other than 0/1 so a bool would make it unambiguous.

> +
> +WL_EXPORT int
> +wl_strtoul(const char *str, char **endptr, int base, uint32_t *val)
> +{
> + char *end = NULL;
> + unsigned long v;
> +
> + if (!str || !val) return 0;
> + if (!endptr) endptr = &end;
> +
> + errno = 0;
> + v = strtoul(str, endptr, base);
> + if (errno != 0 || *endptr == str || **endptr != '\0')
> + return 0;
> +
> + *val = v;
> + return 1;
> +}
> +

Ditto previous comments.

>  static void
>  wl_log_stderr_handler(const char *fmt, va_list arg)
>  {
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index fd32826..b77d4e3 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -243,6 +243,9 @@ static inline wl_fixed_t wl_fixed_from_int(int i)
>   return i * 256;
>  }
> 
> +int wl_strtol(const char *str, char **endptr, int base, int32_t *val);
> +int wl_strtoul(const char *str, char **endptr, int base, uint32_t *val);
> +
>  /**
>   * \brief A union representing all of the basic data types that can be
> passed
>   * along the wayland wire format.
> diff --git a/tests/fixed-test.c b/tests/fixed-test.c
> index 739a3b1..349cc48 100644
> --- a/tests/fixed-test.c
> +++ b/tests/fixed-test.c
> @@ -88,3 +88,61 @@ TEST(fixed_int_conversions)
>   i = wl_fixed_to_int(f);
>   assert(i == -0x50);
>  }

Thanks for including tests.  So often forgotten...!

> +TEST(strtol_conversions)
> +{
> + int ret;
> + int32_t val = -1;
> + char *end = NULL;
> +
> + char *str = "12";
> + ret = wl_strtol(str, NULL, 10, &val);
> + assert(ret == 1);
> + assert(val == 12);
> +
> + ret = wl_strtol(str, &end, 10, &val);
> + assert(end != NULL);
> + assert(*end == '\0');
> +
> + str = "s12"; val = -1;
> + ret = wl_strtol(str, NULL, 10, &val);
> + assert(ret == 0);
> + assert(val == -1);
> +
> + ret = wl_strtol(str, &end, 10, &val);
> + assert(end == str);
> +
> + str = ""; val = -1;
> + ret = wl_strtol(str, NULL, 10, &val);
> + assert(ret == 0);
> + assert(val == -1);
> +}
> +
> +TEST(strtoul_conversions)
> +{
> + int ret;
> + uint32_t val = 0;
> + char *end = NULL;
> +
> + char *str = "15";
> + ret = wl_strtoul(str, NULL, 10, &val);
> + assert(ret == 1);
> + assert(val == 15);
> +
> + ret = wl_strtoul(str, &end, 10, &val);
> + assert(end != NULL);
> + assert(*end == '\0');
> +
> + str = "s15"; val = 0;
> + ret = wl_strtoul(str, NULL, 10, &val);
> + assert(ret == 0);
> + assert(val == 0);
> +
> + ret = wl_strtoul(str, &end, 10, &val);
> + assert(end == str);
> +
> + str = ""; val = 0;
> + ret = wl_strtoul(str, NULL, 10, &val);
> + assert(ret == 0);
> + assert(val == 0);
> +}

Is base 10 all we care about?  If so, then maybe consider limiting the
scope of the function to just that?  If not, then perhaps include a few
tests for base 2, base 16.

Test also the error modes of passing str == NULL and NULL instead of
&val.  A zero or negative base param might be fun too.  May as well test
parsing of negative numbers too, since the two functions differ in this
respect.

I'm not sure how widely we're using strtol but if it's used in the
parsing of config file data, then it might be beneficial to include
tests for locale-specific number formats, such as "1,000,000" and
"1.000.000".

Bryce