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 <[email protected]> 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
