<div dir="ltr"><div>Hi,<br><br></div><div>Personally, I'd rather see these functions private (somebody has already mentioned that),<br>but I understand there are reasons for them to be public and maybe in the end it will have more pros.. <br></div><div>Anyway, I have few nitpicks and a questions - see below.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On 16 October 2014 18:11, Imran Zaman <span dir="ltr"><<a href="mailto:imran.zaman@gmail.com" target="_blank">imran.zaman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">strtol/strtoul utility functions are used extensively in<br>
weston/wayland, and are not bug-free in their current form.<br>
To avoid definition in weston and wayland, its wrapped<br>
in functions with appropriate input and output checks.<br>
Test cases are also updated.<br>
<br>
Signed-off-by: Imran Zaman <<a href="mailto:imran.zaman@gmail.com">imran.zaman@gmail.com</a>><br>
---<br>
 src/scanner.c        |   5 +--<br>
 src/wayland-client.c |   5 +--<br>
 src/wayland-util.c   |  38 ++++++++++++++++<br>
 src/wayland-util.h   |   4 ++<br>
 tests/fixed-test.c   | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++<br>
 5 files changed, 167 insertions(+), 7 deletions(-)<br>
<br>
diff --git a/src/scanner.c b/src/scanner.c<br>
index 809130b..165b12b 100644<br>
--- a/src/scanner.c<br>
+++ b/src/scanner.c<br>
@@ -315,7 +315,6 @@ start_element(void *data, const char *element_name, const char **atts)<br>
        struct description *description;<br>
        const char *name, *type, *interface_name, *value, *summary, *since;<br>
        const char *allow_null;<br>
-       char *end;<br>
        int i, version;<br>
<br>
        ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser);<br>
@@ -404,9 +403,7 @@ start_element(void *data, const char *element_name, const char **atts)<br>
                        message->destructor = 0;<br>
<br>
                if (since != NULL) {<br>
-                       errno = 0;<br>
-                       version = strtol(since, &end, 0);<br>
-                       if (errno == EINVAL || end == since || *end != '\0')<br>
+                       if (!wl_strtol(since, NULL, 0, (long *)&version))<br>
                                fail(&ctx->loc,<br>
                                     "invalid integer (%s)\n", since);<br>
                } else {<br>
diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
index b0f77b9..fd98705 100644<br>
--- a/src/wayland-client.c<br>
+++ b/src/wayland-client.c<br>
@@ -824,13 +824,12 @@ wl_display_connect_to_fd(int fd)<br>
 WL_EXPORT struct wl_display *<br>
 wl_display_connect(const char *name)<br>
 {<br>
-       char *connection, *end;<br>
+       char *connection;<br>
        int flags, fd;<br>
<br>
        connection = getenv("WAYLAND_SOCKET");<br>
        if (connection) {<br>
-               fd = strtol(connection, &end, 0);<br>
-               if (*end != '\0')<br>
+               if (!wl_strtol(connection, NULL, 0, (long *)&fd))<br>
                        return NULL;<br>
<br>
                flags = fcntl(fd, F_GETFD);<br>
diff --git a/src/wayland-util.c b/src/wayland-util.c<br>
index b099882..dfd2eb1 100644<br>
--- a/src/wayland-util.c<br>
+++ b/src/wayland-util.c<br>
@@ -26,6 +26,8 @@<br>
 #include <stdio.h><br>
 #include <string.h><br>
 #include <stdarg.h><br>
+#include <errno.h><br>
+#include <limits.h><br>
<br>
 #include "wayland-util.h"<br>
 #include "wayland-private.h"<br>
@@ -361,6 +363,42 @@ wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data)<br>
        for_each_helper(&map->server_entries, func, data);<br>
 }<br>
<br>
+WL_EXPORT bool<br>
+wl_strtol(const char *str, char **endptr, int base, long *val)<br>
+{<br>
+       char *end = NULL;<br>
+       long v;<br>
+<br>
+       if (!str || !val) return false;<br>
+       if (!endptr) endptr = &end;<br></blockquote><div><br></div><div>The 'return false' and 'endptr =&end' should be on new line<br><a href="http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33">http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+       errno = 0;<br>
+       v = strtol(str, endptr, base);<br>
+       if (errno != 0 || *endptr == str || **endptr != '\0')<br>
+               return false;<br>
+<br>
+       *val = v;<br>
+       return true;<br>
+}<br>
+<br>
+WL_EXPORT bool<br>
+wl_strtoul(const char *str, char **endptr, int base, unsigned long *val)<br>
+{<br>
+       char *end = NULL;<br>
+       unsigned long v;<br>
+<br>
+       if (!str || !val) return false;<br>
+       if (!endptr) endptr = &end;<br></blockquote><div> </div><div>The same<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+       errno = 0;<br>
+       v = strtoul(str, endptr, base);<br>
+       if (errno != 0 || *endptr == str || **endptr != '\0')<br>
+               return false;<br>
+<br>
+       *val = v;<br>
+       return true;<br>
+}<br>
+<br>
 static void<br>
 wl_log_stderr_handler(const char *fmt, va_list arg)<br>
 {<br>
diff --git a/src/wayland-util.h b/src/wayland-util.h<br>
index fd32826..c66cc41 100644<br>
--- a/src/wayland-util.h<br>
+++ b/src/wayland-util.h<br>
@@ -36,6 +36,7 @@ extern "C" {<br>
 #include <stddef.h><br>
 #include <inttypes.h><br>
 #include <stdarg.h><br>
+#include <stdbool.h><br>
<br>
 /* GCC visibility */<br>
 #if defined(__GNUC__) && __GNUC__ >= 4<br>
@@ -243,6 +244,9 @@ static inline wl_fixed_t wl_fixed_from_int(int i)<br>
        return i * 256;<br>
 }<br>
<br>
+bool wl_strtol(const char *str, char **endptr, int base, long *val);<br>
+bool wl_strtoul(const char *str, char **endptr, int base, unsigned long *val);<br>
+<br>
 /**<br>
  * \brief A union representing all of the basic data types that can be passed<br>
  * along the wayland wire format.<br>
diff --git a/tests/fixed-test.c b/tests/fixed-test.c<br>
index 739a3b1..aa0340c 100644<br>
--- a/tests/fixed-test.c<br>
+++ b/tests/fixed-test.c<br>
@@ -88,3 +88,125 @@ TEST(fixed_int_conversions)<br>
        i = wl_fixed_to_int(f);<br>
        assert(i == -0x50);<br>
 }<br>
+<br>
+TEST(strtol_conversions)<br>
+{<br>
+       bool ret;<br>
+       long val = -1;<br>
+       char *end = NULL;<br>
+<br>
+       ret = wl_strtol(NULL, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == -1);<br>
+<br>
+       ret = wl_strtol(NULL, NULL, 10, NULL);<br>
+       assert(ret == false);<br>
+<br>
+       char *str = "12";<br></blockquote><div> </div><div>This variable should be declared on the beginning of the function and only assigned here<br>(again from the Contributing document).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+       ret = wl_strtol(str, NULL, 10, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 12);<br>
+<br>
+       ret = wl_strtol(str, &end, 10, &val);<br>
+       assert(end != NULL);<br>
+       assert(*end == '\0');<br>
+<br>
+       str = "-12"; val = -1;<br>
+       ret = wl_strtol(str, &end, 10, &val);<br>
+       assert(ret == true);<br>
+       assert(val == -12);<br>
+<br>
+       str = "0x12"; val = -1;<br>
+       ret = wl_strtol(str, &end, 16, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 0x12);<br>
+<br>
+       str = "A"; val = -1;<br>
+       ret = wl_strtol(str, &end, 16, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 10);<br>
+<br>
+       str = "-0x20"; val = -1;<br>
+       ret = wl_strtol(str, &end, 16, &val);<br>
+       assert(ret == true);<br>
+       assert(val == -0x20);<br>
+<br>
+       str = "0012"; val = -1;<br>
+       ret = wl_strtol(str, &end, 8, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 10);<br>
+<br>
+       str = "0101"; val = -1;<br>
+       ret = wl_strtol(str, &end, 2, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 5);<br>
+<br>
+       str = "s12"; val = -1;<br>
+       ret = wl_strtol(str, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == -1);<br>
+<br>
+       ret = wl_strtol(str, &end, 10, &val);<br>
+       assert(end == str);<br>
+<br>
+       str = ""; val = -1;<br>
+       ret = wl_strtol(str, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == -1);<br>
+}<br>
+<br>
+TEST(strtoul_conversions)<br>
+{<br>
+       bool ret;<br>
+       unsigned long val = 0;<br>
+       char *end = NULL;<br>
+<br>
+       ret = wl_strtoul(NULL, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == 0);<br>
+<br>
+       ret = wl_strtoul(NULL, NULL, 10, NULL);<br>
+       assert(ret == false);<br>
+<br>
+       char *str = "15";<br></blockquote><div><br></div><div>the same<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+       ret = wl_strtoul(str, NULL, 10, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 15);<br>
+<br>
+       ret = wl_strtoul(str, &end, 10, &val);<br>
+       assert(end != NULL);<br>
+       assert(*end == '\0');<br>
+<br>
+       str = "0x12"; val = 0;<br>
+       ret = wl_strtoul(str, &end, 16, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 18);<br>
+<br>
+       str = "A"; val = 0;<br>
+       ret = wl_strtoul(str, &end, 16, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 10);<br>
+<br>
+       str = "0012"; val = 0;<br>
+       ret = wl_strtoul(str, &end, 8, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 10);<br>
+<br>
+       str = "0101"; val = 0;<br>
+       ret = wl_strtoul(str, &end, 2, &val);<br>
+       assert(ret == true);<br>
+       assert(val == 5);<br>
+<br>
+       str = "s15"; val = 0;<br>
+       ret = wl_strtoul(str, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == 0);<br>
+<br>
+       ret = wl_strtoul(str, &end, 10, &val);<br>
+       assert(end == str);<br>
+<br>
+       str = ""; val = 0;<br>
+       ret = wl_strtoul(str, NULL, 10, &val);<br>
+       assert(ret == false);<br>
+       assert(val == 0);<br>
+}<br>
<span class=""><font color="#888888">--<br>
1.9.1<br></font></span></blockquote><div><br></div><div>What I'm wondering is what should be the behavior of wl_strtoul for negative numbers?<br>strtoul silently converts them, but I don't think this is what we always want... or is it?<br>And also some tests for overflow would be handy.<br><br></div><div>Regards,<br>Marek<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class=""><font color="#888888">
<br>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</font></span></blockquote></div><br></div></div>