[PATCH v2] wayland-util: added wl_strtol/wl_strtoul utility functions
Imran Zaman
imran.zaman at gmail.com
Wed Oct 22 04:34:49 PDT 2014
I pushed an updated patch v3. Added test cases for overflow and also
check for negative numbers for wl_strtoul..
please review
BR
imran
On Wed, Oct 22, 2014 at 12:13 PM, Marek Chalupa <mchqwerty at gmail.com> wrote:
> 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 <imran.zaman at gmail.com> 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 <imran.zaman at gmail.com>
>> ---
>> 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
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
More information about the wayland-devel
mailing list