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

[Imran Zaman]

Here are my comments:
Giulio Camuffo: if we copy-paste the same code to weston as well,
means we have to write tests etc for two functions in weston as well;
and it will come with maintenance overhead without any benefit of
hiding the APIs in wayland (I can take the responsibility of
maintaining these two APIs :)
Thiago: I can look into strtol_l functions..
Bryce: I will incorporate your changes and also update the test cases.
There are cases when it is used other than thr base 10 as well; let me
check if there is something that can be done for locale-specific
numbers..

I will push the updated patch after the changes..

On Thu, Oct 16, 2014 at 4:05 AM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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