wl_strtol and wl_strtoul utility functions are added (inlined patch)

Submitted by Imran Zaman on Oct. 15, 2014, 1:18 p.m.

Details

Message ID CAPfuZRjkZuc4BP1jz5M_CoWvEEq7oa=ARXmOTiT64O-ZcX1BKg@mail.gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Imran Zaman Oct. 15, 2014, 1:18 p.m.
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
appropriate
input and output checks; test cases are also updated; will push the patch
for weston as well.
----

Patch hide | download patch | download mbox

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>

 #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)
+{
+ 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;
+ return 1;
+}
+
+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;
+}
+
 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);
 }
+
+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);
+}

Comments

Why? What's the rationale for this?

On Wed, Oct 15, 2014 at 6:18 AM, Imran Zaman <imran.zaman@gmail.com> 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
> appropriate
> input and output checks; test cases are also updated; will push the patch
> for weston as well.
> ----
>
>
> 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>
>
>  #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)
> +{
> + 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;
> + return 1;
> +}
> +
> +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;
> +}
> +
>  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);
>  }
> +
> +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);
> +}
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
The reason is that strtol is used at many places in weston/wayland..
and its not covering all the error cases everywhere.. so its better to
encapsulate it in a function which i did..

On Wed, Oct 15, 2014 at 9:31 PM, Jasper St. Pierre
<jstpierre@mecheye.net> wrote:
> Why? What's the rationale for this?
>
> On Wed, Oct 15, 2014 at 6:18 AM, Imran Zaman <imran.zaman@gmail.com> 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
>> appropriate
>> input and output checks; test cases are also updated; will push the patch
>> for weston as well.
>> ----
>>
>>
>> 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>
>>
>>  #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)
>> +{
>> + 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;
>> + return 1;
>> +}
>> +
>> +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;
>> +}
>> +
>>  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);
>>  }
>> +
>> +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);
>> +}
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
>
>
> --
>   Jasper
You might be able to remove the base argument, is anybody using that? 
That would remove confusion over which is the base and return argument

Also it seems like calling code would be easier to read with a version 
that returns the value but puts the error into errno. Something like this:

    value = wl_strtol (string, NULL);
    if (errno) { string_was_bad (); }

Of course that means it is easier to ignore errors.

In general the idea seems good. Is there any precedent for such 
functions, maybe one already exists?

On 10/15/2014 12:01 PM, Imran Zaman wrote:
> The reason is that strtol is used at many places in weston/wayland..
> and its not covering all the error cases everywhere.. so its better to
> encapsulate it in a function which i did..
2014-10-15 22:01 GMT+03:00 Imran Zaman <imran.zaman@gmail.com>:
> The reason is that strtol is used at many places in weston/wayland..
> and its not covering all the error cases everywhere.. so its better to
> encapsulate it in a function which i did..

You may have a point here, but imho it must not be an exported
function. Put the declaration in wayland-private.h, and just copy it
to weston if you want to have it there too.


--
Giulio

>
> On Wed, Oct 15, 2014 at 9:31 PM, Jasper St. Pierre
> <jstpierre@mecheye.net> wrote:
>> Why? What's the rationale for this?
>>
>> On Wed, Oct 15, 2014 at 6:18 AM, Imran Zaman <imran.zaman@gmail.com> 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
>>> appropriate
>>> input and output checks; test cases are also updated; will push the patch
>>> for weston as well.
>>> ----
>>>
>>>
>>> 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>
>>>
>>>  #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)
>>> +{
>>> + 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;
>>> + return 1;
>>> +}
>>> +
>>> +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;
>>> +}
>>> +
>>>  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);
>>>  }
>>> +
>>> +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);
>>> +}
>>>
>>> _______________________________________________
>>> wayland-devel mailing list
>>> wayland-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>>
>>
>>
>>
>> --
>>   Jasper
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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
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@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