Hi, Personally, I'd rather see these functions private (somebody has already mentioned that), but I understand there are reasons for them to be public and maybe in the end it will have more pros.. Anyway, I have few nitpicks and a questions - see below.
On 16 October 2014 18:11, Imran Zaman <[email protected]> wrote: > strtol/strtoul utility functions are used extensively in > weston/wayland, and are not bug-free in their current form. > To avoid definition in weston and wayland, its wrapped > in functions with appropriate input and output checks. > Test cases are also updated. > > Signed-off-by: Imran Zaman <[email protected]> > --- > src/scanner.c | 5 +-- > src/wayland-client.c | 5 +-- > src/wayland-util.c | 38 ++++++++++++++++ > src/wayland-util.h | 4 ++ > tests/fixed-test.c | 122 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 167 insertions(+), 7 deletions(-) > > diff --git a/src/scanner.c b/src/scanner.c > index 809130b..165b12b 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, (long *)&version)) > fail(&ctx->loc, > "invalid integer (%s)\n", since); > } else { > diff --git a/src/wayland-client.c b/src/wayland-client.c > index b0f77b9..fd98705 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, (long *)&fd)) > return NULL; > > flags = fcntl(fd, F_GETFD); > diff --git a/src/wayland-util.c b/src/wayland-util.c > index b099882..dfd2eb1 100644 > --- a/src/wayland-util.c > +++ b/src/wayland-util.c > @@ -26,6 +26,8 @@ > #include <stdio.h> > #include <string.h> > #include <stdarg.h> > +#include <errno.h> > +#include <limits.h> > > #include "wayland-util.h" > #include "wayland-private.h" > @@ -361,6 +363,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 bool > +wl_strtol(const char *str, char **endptr, int base, long *val) > +{ > + char *end = NULL; > + long v; > + > + if (!str || !val) return false; > + if (!endptr) endptr = &end; > The 'return false' and 'endptr =&end' should be on new line http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33 > + > + errno = 0; > + v = strtol(str, endptr, base); > + if (errno != 0 || *endptr == str || **endptr != '\0') > + return false; > + > + *val = v; > + return true; > +} > + > +WL_EXPORT bool > +wl_strtoul(const char *str, char **endptr, int base, unsigned long *val) > +{ > + char *end = NULL; > + unsigned long v; > + > + if (!str || !val) return false; > + if (!endptr) endptr = &end; > The same > + > + errno = 0; > + v = strtoul(str, endptr, base); > + if (errno != 0 || *endptr == str || **endptr != '\0') > + return false; > + > + *val = v; > + return true; > +} > + > 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..c66cc41 100644 > --- a/src/wayland-util.h > +++ b/src/wayland-util.h > @@ -36,6 +36,7 @@ extern "C" { > #include <stddef.h> > #include <inttypes.h> > #include <stdarg.h> > +#include <stdbool.h> > > /* GCC visibility */ > #if defined(__GNUC__) && __GNUC__ >= 4 > @@ -243,6 +244,9 @@ static inline wl_fixed_t wl_fixed_from_int(int i) > return i * 256; > } > > +bool wl_strtol(const char *str, char **endptr, int base, long *val); > +bool wl_strtoul(const char *str, char **endptr, int base, unsigned long > *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..aa0340c 100644 > --- a/tests/fixed-test.c > +++ b/tests/fixed-test.c > @@ -88,3 +88,125 @@ TEST(fixed_int_conversions) > i = wl_fixed_to_int(f); > assert(i == -0x50); > } > + > +TEST(strtol_conversions) > +{ > + bool ret; > + long val = -1; > + char *end = NULL; > + > + ret = wl_strtol(NULL, NULL, 10, &val); > + assert(ret == false); > + assert(val == -1); > + > + ret = wl_strtol(NULL, NULL, 10, NULL); > + assert(ret == false); > + > + char *str = "12"; > This variable should be declared on the beginning of the function and only assigned here (again from the Contributing document). > + ret = wl_strtol(str, NULL, 10, &val); > + assert(ret == true); > + assert(val == 12); > + > + ret = wl_strtol(str, &end, 10, &val); > + assert(end != NULL); > + assert(*end == '\0'); > + > + str = "-12"; val = -1; > + ret = wl_strtol(str, &end, 10, &val); > + assert(ret == true); > + assert(val == -12); > + > + str = "0x12"; val = -1; > + ret = wl_strtol(str, &end, 16, &val); > + assert(ret == true); > + assert(val == 0x12); > + > + str = "A"; val = -1; > + ret = wl_strtol(str, &end, 16, &val); > + assert(ret == true); > + assert(val == 10); > + > + str = "-0x20"; val = -1; > + ret = wl_strtol(str, &end, 16, &val); > + assert(ret == true); > + assert(val == -0x20); > + > + str = "0012"; val = -1; > + ret = wl_strtol(str, &end, 8, &val); > + assert(ret == true); > + assert(val == 10); > + > + str = "0101"; val = -1; > + ret = wl_strtol(str, &end, 2, &val); > + assert(ret == true); > + assert(val == 5); > + > + str = "s12"; val = -1; > + ret = wl_strtol(str, NULL, 10, &val); > + assert(ret == false); > + 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 == false); > + assert(val == -1); > +} > + > +TEST(strtoul_conversions) > +{ > + bool ret; > + unsigned long val = 0; > + char *end = NULL; > + > + ret = wl_strtoul(NULL, NULL, 10, &val); > + assert(ret == false); > + assert(val == 0); > + > + ret = wl_strtoul(NULL, NULL, 10, NULL); > + assert(ret == false); > + > + char *str = "15"; > the same > + ret = wl_strtoul(str, NULL, 10, &val); > + assert(ret == true); > + assert(val == 15); > + > + ret = wl_strtoul(str, &end, 10, &val); > + assert(end != NULL); > + assert(*end == '\0'); > + > + str = "0x12"; val = 0; > + ret = wl_strtoul(str, &end, 16, &val); > + assert(ret == true); > + assert(val == 18); > + > + str = "A"; val = 0; > + ret = wl_strtoul(str, &end, 16, &val); > + assert(ret == true); > + assert(val == 10); > + > + str = "0012"; val = 0; > + ret = wl_strtoul(str, &end, 8, &val); > + assert(ret == true); > + assert(val == 10); > + > + str = "0101"; val = 0; > + ret = wl_strtoul(str, &end, 2, &val); > + assert(ret == true); > + assert(val == 5); > + > + str = "s15"; val = 0; > + ret = wl_strtoul(str, NULL, 10, &val); > + assert(ret == false); > + 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 == false); > + assert(val == 0); > +} > -- > 1.9.1 > What I'm wondering is what should be the behavior of wl_strtoul for negative numbers? strtoul silently converts them, but I don't think this is what we always want... or is it? And also some tests for overflow would be handy. Regards, Marek > > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel >
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
