[v3] wayland-util: added wl_strtol/wl_strtoul utility functions

Submitted by Imran Zaman on Oct. 22, 2014, 11:32 a.m.

Details

Message ID 1413977548-8755-1-git-send-email-imran.zaman@gmail.com
State Changes Requested
Headers show

Not browsing as part of any series.

Commit Message

Imran Zaman Oct. 22, 2014, 11:32 a.m.
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@gmail.com>
---
 src/scanner.c        |   5 +-
 src/wayland-client.c |   5 +-
 src/wayland-util.c   |  55 ++++++++++++++++++++
 src/wayland-util.h   |   4 ++
 tests/fixed-test.c   | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 204 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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..a930364 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 <ctype.h>
 
 #include "wayland-util.h"
 #include "wayland-private.h"
@@ -361,6 +363,59 @@  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;
+
+	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;
+	int i = 0;
+
+	if (!str || !val)
+		return false;
+
+	/* check for negative numbers */
+	while (str[i]) {
+		if (!isspace(str[i])) {
+			if (str[i] == '-')
+				return false;
+			else
+				break;
+		}
+		i++;
+	}
+
+	if (!endptr)
+		endptr = &end;
+
+	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..ad40467 100644
--- a/tests/fixed-test.c
+++ b/tests/fixed-test.c
@@ -88,3 +88,145 @@  TEST(fixed_int_conversions)
 	i = wl_fixed_to_int(f);
 	assert(i == -0x50);
 }
+
+TEST(strtol_conversions)
+{
+	bool ret;
+	long val = -1;
+	char *end = NULL, *str = NULL;
+
+	ret = wl_strtol(NULL, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == -1);
+
+	ret = wl_strtol(NULL, NULL, 10, NULL);
+	assert(ret == false);
+
+	str = "12";
+	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 = "214748364789L"; val = -1;
+	ret = wl_strtol(str, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == -1);
+
+	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, *str = NULL;
+
+	ret = wl_strtoul(NULL, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == 0);
+
+	ret = wl_strtoul(NULL, NULL, 10, NULL);
+	assert(ret == false);
+
+	str = "15";
+	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 = "429496729533UL"; val = 0;
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == 0);
+
+	str = "-1"; val = 0;
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == 0);
+
+	str = "    -1234"; val = 0;
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == 0);
+
+	str = ""; val = 0;
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == false);
+	assert(val == 0);
+}

Comments

On 22 October 2014 13:32, Imran Zaman <imran.zaman@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@gmail.com>
> ---
>  src/scanner.c        |   5 +-
>  src/wayland-client.c |   5 +-
>  src/wayland-util.c   |  55 ++++++++++++++++++++
>  src/wayland-util.h   |   4 ++
>  tests/fixed-test.c   | 142
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 204 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))
>

This is baad. You cannot use the int version here, because in wl_strol you
write sizeof(long) on the address
of version and if sizeof(version) is smaller than sizeof(long) then you
overwrite memory. I'm getting ugly SIGSEGV
because of that.



>                                 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..a930364 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 <ctype.h>
>
>  #include "wayland-util.h"
>  #include "wayland-private.h"
> @@ -361,6 +363,59 @@ 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;
> +
> +       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;
> +       int i = 0;
> +
> +       if (!str || !val)
> +               return false;
> +
> +       /* check for negative numbers */
> +       while (str[i]) {
> +               if (!isspace(str[i])) {
> +                       if (str[i] == '-')
> +                               return false;
> +                       else
> +                               break;
> +               }
> +               i++;
> +       }
> +
> +       if (!endptr)
> +               endptr = &end;
> +
> +       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..ad40467 100644
> --- a/tests/fixed-test.c
> +++ b/tests/fixed-test.c
> @@ -88,3 +88,145 @@ TEST(fixed_int_conversions)
>         i = wl_fixed_to_int(f);
>         assert(i == -0x50);
>  }
> +
> +TEST(strtol_conversions)
> +{
> +       bool ret;
> +       long val = -1;
> +       char *end = NULL, *str = NULL;
> +
> +       ret = wl_strtol(NULL, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == -1);
> +
> +       ret = wl_strtol(NULL, NULL, 10, NULL);
> +       assert(ret == false);
> +
> +       str = "12";
> +       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 = "214748364789L"; val = -1;
> +       ret = wl_strtol(str, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == -1);
> +
> +       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, *str = NULL;
> +
> +       ret = wl_strtoul(NULL, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == 0);
> +
> +       ret = wl_strtoul(NULL, NULL, 10, NULL);
> +       assert(ret == false);
> +
> +       str = "15";
> +       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 = "429496729533UL"; val = 0;
> +       ret = wl_strtoul(str, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == 0);
> +
> +       str = "-1"; val = 0;
> +       ret = wl_strtoul(str, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == 0);
> +
> +       str = "    -1234"; val = 0;
> +       ret = wl_strtoul(str, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == 0);
> +
> +       str = ""; val = 0;
> +       ret = wl_strtoul(str, NULL, 10, &val);
> +       assert(ret == false);
> +       assert(val == 0);
> +}
>

Maybe the tests are big enough to deserve it's own binary? fixed-tests has
nothing in common with wl_strtol.
My opinion only..


> --
> 1.9.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>

Cheers,
Marek
Can I also suggest that we don't make this public API? These are internal
helpers for libwayland, not designed for any consumers. We've been burned
by making too much internal helper API public before.

On Mon, Oct 27, 2014 at 2:42 AM, Marek Chalupa <mchqwerty@gmail.com> wrote:

>
>
> On 22 October 2014 13:32, Imran Zaman <imran.zaman@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@gmail.com>
>> ---
>>  src/scanner.c        |   5 +-
>>  src/wayland-client.c |   5 +-
>>  src/wayland-util.c   |  55 ++++++++++++++++++++
>>  src/wayland-util.h   |   4 ++
>>  tests/fixed-test.c   | 142
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 204 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))
>>
>
> This is baad. You cannot use the int version here, because in wl_strol you
> write sizeof(long) on the address
> of version and if sizeof(version) is smaller than sizeof(long) then you
> overwrite memory. I'm getting ugly SIGSEGV
> because of that.
>
>
>
>>                                 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..a930364 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 <ctype.h>
>>
>>  #include "wayland-util.h"
>>  #include "wayland-private.h"
>> @@ -361,6 +363,59 @@ 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;
>> +
>> +       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;
>> +       int i = 0;
>> +
>> +       if (!str || !val)
>> +               return false;
>> +
>> +       /* check for negative numbers */
>> +       while (str[i]) {
>> +               if (!isspace(str[i])) {
>> +                       if (str[i] == '-')
>> +                               return false;
>> +                       else
>> +                               break;
>> +               }
>> +               i++;
>> +       }
>> +
>> +       if (!endptr)
>> +               endptr = &end;
>> +
>> +       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..ad40467 100644
>> --- a/tests/fixed-test.c
>> +++ b/tests/fixed-test.c
>> @@ -88,3 +88,145 @@ TEST(fixed_int_conversions)
>>         i = wl_fixed_to_int(f);
>>         assert(i == -0x50);
>>  }
>> +
>> +TEST(strtol_conversions)
>> +{
>> +       bool ret;
>> +       long val = -1;
>> +       char *end = NULL, *str = NULL;
>> +
>> +       ret = wl_strtol(NULL, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == -1);
>> +
>> +       ret = wl_strtol(NULL, NULL, 10, NULL);
>> +       assert(ret == false);
>> +
>> +       str = "12";
>> +       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 = "214748364789L"; val = -1;
>> +       ret = wl_strtol(str, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == -1);
>> +
>> +       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, *str = NULL;
>> +
>> +       ret = wl_strtoul(NULL, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == 0);
>> +
>> +       ret = wl_strtoul(NULL, NULL, 10, NULL);
>> +       assert(ret == false);
>> +
>> +       str = "15";
>> +       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 = "429496729533UL"; val = 0;
>> +       ret = wl_strtoul(str, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == 0);
>> +
>> +       str = "-1"; val = 0;
>> +       ret = wl_strtoul(str, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == 0);
>> +
>> +       str = "    -1234"; val = 0;
>> +       ret = wl_strtoul(str, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == 0);
>> +
>> +       str = ""; val = 0;
>> +       ret = wl_strtoul(str, NULL, 10, &val);
>> +       assert(ret == false);
>> +       assert(val == 0);
>> +}
>>
>
> Maybe the tests are big enough to deserve it's own binary? fixed-tests has
> nothing in common with wl_strtol.
> My opinion only..
>
>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
> Cheers,
> Marek
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
On 10/27/2014 09:51 AM, Jasper St. Pierre wrote:

>         +                       if (!wl_strtol(since, NULL, 0, (long
>         *)&version))
>
>
>     This is baad. You cannot use the int version here, because in
>     wl_strol you write sizeof(long) on the address
>     of version and if sizeof(version) is smaller than sizeof(long) then
>     you overwrite memory. I'm getting ugly SIGSEGV
>     because of that.

That is annoying since it means an integer version must be provided as 
well (plus the annoyance that on some platforms it will not complain and 
work if you call the wrong function).

I think it would be better now to make this function return the value, 
and set errno on any error. Or make the error indicator an output 
parameter. I'd also dump the "base" parameter as it is not being used by 
anybody.
2014-10-27 18:51 GMT+02:00 Jasper St. Pierre <jstpierre@mecheye.net>:
> Can I also suggest that we don't make this public API? These are internal
> helpers for libwayland, not designed for any consumers. We've been burned by
> making too much internal helper API public before.

+1
I don't think this belongs in the wayland API at all. That means
duplicating them in weston, but they will hardly need modifications
anyway.


--
Giulio

>
> On Mon, Oct 27, 2014 at 2:42 AM, Marek Chalupa <mchqwerty@gmail.com> wrote:
>>
>>
>>
>> On 22 October 2014 13:32, Imran Zaman <imran.zaman@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@gmail.com>
>>> ---
>>>  src/scanner.c        |   5 +-
>>>  src/wayland-client.c |   5 +-
>>>  src/wayland-util.c   |  55 ++++++++++++++++++++
>>>  src/wayland-util.h   |   4 ++
>>>  tests/fixed-test.c   | 142
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 204 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))
>>
>>
>> This is baad. You cannot use the int version here, because in wl_strol you
>> write sizeof(long) on the address
>> of version and if sizeof(version) is smaller than sizeof(long) then you
>> overwrite memory. I'm getting ugly SIGSEGV
>> because of that.
>>
>>
>>>
>>>                                 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..a930364 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 <ctype.h>
>>>
>>>  #include "wayland-util.h"
>>>  #include "wayland-private.h"
>>> @@ -361,6 +363,59 @@ 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;
>>> +
>>> +       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;
>>> +       int i = 0;
>>> +
>>> +       if (!str || !val)
>>> +               return false;
>>> +
>>> +       /* check for negative numbers */
>>> +       while (str[i]) {
>>> +               if (!isspace(str[i])) {
>>> +                       if (str[i] == '-')
>>> +                               return false;
>>> +                       else
>>> +                               break;
>>> +               }
>>> +               i++;
>>> +       }
>>> +
>>> +       if (!endptr)
>>> +               endptr = &end;
>>> +
>>> +       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..ad40467 100644
>>> --- a/tests/fixed-test.c
>>> +++ b/tests/fixed-test.c
>>> @@ -88,3 +88,145 @@ TEST(fixed_int_conversions)
>>>         i = wl_fixed_to_int(f);
>>>         assert(i == -0x50);
>>>  }
>>> +
>>> +TEST(strtol_conversions)
>>> +{
>>> +       bool ret;
>>> +       long val = -1;
>>> +       char *end = NULL, *str = NULL;
>>> +
>>> +       ret = wl_strtol(NULL, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == -1);
>>> +
>>> +       ret = wl_strtol(NULL, NULL, 10, NULL);
>>> +       assert(ret == false);
>>> +
>>> +       str = "12";
>>> +       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 = "214748364789L"; val = -1;
>>> +       ret = wl_strtol(str, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == -1);
>>> +
>>> +       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, *str = NULL;
>>> +
>>> +       ret = wl_strtoul(NULL, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == 0);
>>> +
>>> +       ret = wl_strtoul(NULL, NULL, 10, NULL);
>>> +       assert(ret == false);
>>> +
>>> +       str = "15";
>>> +       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 = "429496729533UL"; val = 0;
>>> +       ret = wl_strtoul(str, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == 0);
>>> +
>>> +       str = "-1"; val = 0;
>>> +       ret = wl_strtoul(str, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == 0);
>>> +
>>> +       str = "    -1234"; val = 0;
>>> +       ret = wl_strtoul(str, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == 0);
>>> +
>>> +       str = ""; val = 0;
>>> +       ret = wl_strtoul(str, NULL, 10, &val);
>>> +       assert(ret == false);
>>> +       assert(val == 0);
>>> +}
>>
>>
>> Maybe the tests are big enough to deserve it's own binary? fixed-tests has
>> nothing in common with wl_strtol.
>> My opinion only..
>>
>>>
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> wayland-devel mailing list
>>> wayland-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>>
>> Cheers,
>> Marek
>>
>>
>> _______________________________________________
>> 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
>
Hi,

On 28 October 2014 11:21, Giulio Camuffo <giuliocamuffo@gmail.com> wrote:

> 2014-10-27 18:51 GMT+02:00 Jasper St. Pierre <jstpierre@mecheye.net>:
> > Can I also suggest that we don't make this public API? These are internal
> > helpers for libwayland, not designed for any consumers. We've been
> burned by
> > making too much internal helper API public before.
>
> +1
> I don't think this belongs in the wayland API at all. That means
> duplicating them in weston, but they will hardly need modifications
> anyway.
>

AOL. We're a window system, not a replacement libc.

Cheers,
Dan
You guys should check the reason why the patch is there rather than
throwing out random thoughts or blunt comments.

I did this patch mainly because weston/wayland has been using
strtol/strtoul functions in number of places with buggy error checks, and
duplicate code everywhere. Weston and wayland go together; so in bigger
picture, its a very useful patch IMO.. I hardly find any patches with
proper tests, but I did it so to make it more effective. But I guess in
wayland/weston community, only maintainers are allowed to push patches
others are strongly discouraged to do so. I guess its better to encourage
people/community for giving helping hand.

Anyways we will now only push patches (including multi-seat support) in
Tizen weston/wayland rather than wasting time in upstreamn weston/wayland
as it seems to be long bureaucratic process to overcome with virtually no
success.

BR
imran


On Tue, Oct 28, 2014 at 4:50 PM, Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
>
> On 28 October 2014 11:21, Giulio Camuffo <giuliocamuffo@gmail.com> wrote:
>
>> 2014-10-27 18:51 GMT+02:00 Jasper St. Pierre <jstpierre@mecheye.net>:
>> > Can I also suggest that we don't make this public API? These are
>> internal
>> > helpers for libwayland, not designed for any consumers. We've been
>> burned by
>> > making too much internal helper API public before.
>>
>> +1
>> I don't think this belongs in the wayland API at all. That means
>> duplicating them in weston, but they will hardly need modifications
>> anyway.
>>
>
> AOL. We're a window system, not a replacement libc.
>
> Cheers,
> Dan
>
Hi,

On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:

> You guys should check the reason why the patch is there rather than
> throwing out random thoughts or blunt comments.
>
> I did this patch mainly because weston/wayland has been using
> strtol/strtoul functions in number of places with buggy error checks, and
> duplicate code everywhere. Weston and wayland go together; so in bigger
> picture, its a very useful patch IMO.. I hardly find any patches with
> proper tests, but I did it so to make it more effective. But I guess in
> wayland/weston community, only maintainers are allowed to push patches
> others are strongly discouraged to do so. I guess its better to encourage
> people/community for giving helping hand.
>
> Anyways we will now only push patches (including multi-seat support) in
> Tizen weston/wayland rather than wasting time in upstreamn weston/wayland
> as it seems to be long bureaucratic process to overcome with virtually no
> success.
>

That's not what we've said. No-one said 'don't take the patch'; all we said
is 'please don't expose it as part of libwayland-*'s _public_ API'.

I like the idea of the patch, I like how it's caught a number of buggy
spots where we've open-coded the same thing, and I like that it's
well-tested. For internal usage, it's great! I just don't want to expose
string manipulation functions in the public API of a window system.

You're right that the test infrastructure is in a pretty bad state.
Anything which helps that is more than welcome, and you may have seen a
bunch of patches from Derek Foreman (not a maintainer) on this list, which
are progressing well and go a long way towards improving our test suite
into something that will be really useful day to day. Any further
contributions along those lines - from anyone - are totally welcome.

As for your multiseat patches, the last discussions I remember involved
some pretty fundamental disagreements about the whole architecture,
particularly input support. I haven't seen any more patches or discussion
since the last IRC chat, though.

Cheers,
Daniel
Daniel!

As per your logic, I see wl_list APIs exposed etc, which shouldn't be part
of libwayland as well.
similarly, wl_fixed_to_double and wl_array shouldn't be part of the window
system. Isnt it?
I can make inline functions if that helps.

Btw here is an API patch, which has not be reviewed till now.
http://lists.freedesktop.org/archives/wayland-devel/2014-October/017833.html

BR
imran

On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
>
> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>
>> You guys should check the reason why the patch is there rather than
>> throwing out random thoughts or blunt comments.
>>
>> I did this patch mainly because weston/wayland has been using
>> strtol/strtoul functions in number of places with buggy error checks, and
>> duplicate code everywhere. Weston and wayland go together; so in bigger
>> picture, its a very useful patch IMO.. I hardly find any patches with
>> proper tests, but I did it so to make it more effective. But I guess in
>> wayland/weston community, only maintainers are allowed to push patches
>> others are strongly discouraged to do so. I guess its better to encourage
>> people/community for giving helping hand.
>>
>> Anyways we will now only push patches (including multi-seat support) in
>> Tizen weston/wayland rather than wasting time in upstreamn weston/wayland
>> as it seems to be long bureaucratic process to overcome with virtually no
>> success.
>>
>
> That's not what we've said. No-one said 'don't take the patch'; all we
> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
>
> I like the idea of the patch, I like how it's caught a number of buggy
> spots where we've open-coded the same thing, and I like that it's
> well-tested. For internal usage, it's great! I just don't want to expose
> string manipulation functions in the public API of a window system.
>
> You're right that the test infrastructure is in a pretty bad state.
> Anything which helps that is more than welcome, and you may have seen a
> bunch of patches from Derek Foreman (not a maintainer) on this list, which
> are progressing well and go a long way towards improving our test suite
> into something that will be really useful day to day. Any further
> contributions along those lines - from anyone - are totally welcome.
>
> As for your multiseat patches, the last discussions I remember involved
> some pretty fundamental disagreements about the whole architecture,
> particularly input support. I haven't seen any more patches or discussion
> since the last IRC chat, though.
>
> Cheers,
> Daniel
>
>
On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
>
> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>
>> You guys should check the reason why the patch is there rather than
>> throwing out random thoughts or blunt comments.
>>
>> I did this patch mainly because weston/wayland has been using
>> strtol/strtoul functions in number of places with buggy error checks, and
>> duplicate code everywhere. Weston and wayland go together; so in bigger
>> picture, its a very useful patch IMO.. I hardly find any patches with
>> proper tests, but I did it so to make it more effective. But I guess in
>> wayland/weston community, only maintainers are allowed to push patches
>> others are strongly discouraged to do so. I guess its better to encourage
>> people/community for giving helping hand.
>>
>> Anyways we will now only push patches (including multi-seat support) in
>> Tizen weston/wayland rather than wasting time in upstreamn weston/wayland
>> as it seems to be long bureaucratic process to overcome with virtually no
>> success.
>>
>
> That's not what we've said. No-one said 'don't take the patch'; all we
> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
>
> I like the idea of the patch, I like how it's caught a number of buggy
> spots where we've open-coded the same thing, and I like that it's
> well-tested. For internal usage, it's great! I just don't want to expose
> string manipulation functions in the public API of a window system.
>
> You're right that the test infrastructure is in a pretty bad state.
> Anything which helps that is more than welcome, and you may have seen a
> bunch of patches from Derek Foreman (not a maintainer) on this list, which
> are progressing well and go a long way towards improving our test suite
> into something that will be really useful day to day. Any further
> contributions along those lines - from anyone - are totally welcome.
>
> As for your multiseat patches, the last discussions I remember involved
> some pretty fundamental disagreements about the whole architecture,
> particularly input support. I haven't seen any more patches or discussion
> since the last IRC chat, though.
>
> Cheers,
> Daniel
>
2014-10-29 8:45 GMT+02:00 Imran Zaman <imran.zaman@gmail.com>:
> Daniel!
>
> As per your logic, I see wl_list APIs exposed etc, which shouldn't be part
> of libwayland as well.
> similarly, wl_fixed_to_double and wl_array shouldn't be part of the window
> system. Isnt it?
> I can make inline functions if that helps.

wl_list is used in the server side API, so it's a bit different.
However, I'd agree that it'd be better if it wasn't exposed but we
can't remove it now. wl_fixed is a wayland specific type so all the
wl_fixed_* functions need to be part of the API.
On the other hand wl_strtol would just be a function, there are is no
other API that depends on it.

>
> Btw here is an API patch, which has not be reviewed till now.
> http://lists.freedesktop.org/archives/wayland-devel/2014-October/017833.html

Yes, like there are many other patches waiting for reviews. You need
to have patience, it's not like we are ignoring it. But, if I may add,
the way you are reacting to a comment to this patch doesn't encourage
to review your other ones.


--
Giulio

>
> BR
> imran
>
> On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>
>> Hi,
>>
>> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>>>
>>> You guys should check the reason why the patch is there rather than
>>> throwing out random thoughts or blunt comments.
>>>
>>> I did this patch mainly because weston/wayland has been using
>>> strtol/strtoul functions in number of places with buggy error checks, and
>>> duplicate code everywhere. Weston and wayland go together; so in bigger
>>> picture, its a very useful patch IMO.. I hardly find any patches with proper
>>> tests, but I did it so to make it more effective. But I guess in
>>> wayland/weston community, only maintainers are allowed to push patches
>>> others are strongly discouraged to do so. I guess its better to encourage
>>> people/community for giving helping hand.
>>>
>>> Anyways we will now only push patches (including multi-seat support) in
>>> Tizen weston/wayland rather than wasting time in upstreamn weston/wayland as
>>> it seems to be long bureaucratic process to overcome with virtually no
>>> success.
>>
>>
>> That's not what we've said. No-one said 'don't take the patch'; all we
>> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
>>
>> I like the idea of the patch, I like how it's caught a number of buggy
>> spots where we've open-coded the same thing, and I like that it's
>> well-tested. For internal usage, it's great! I just don't want to expose
>> string manipulation functions in the public API of a window system.
>>
>> You're right that the test infrastructure is in a pretty bad state.
>> Anything which helps that is more than welcome, and you may have seen a
>> bunch of patches from Derek Foreman (not a maintainer) on this list, which
>> are progressing well and go a long way towards improving our test suite into
>> something that will be really useful day to day. Any further contributions
>> along those lines - from anyone - are totally welcome.
>>
>> As for your multiseat patches, the last discussions I remember involved
>> some pretty fundamental disagreements about the whole architecture,
>> particularly input support. I haven't seen any more patches or discussion
>> since the last IRC chat, though.
>>
>> Cheers,
>> Daniel
>>
>
> On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>>
>> Hi,
>>
>> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>>>
>>> You guys should check the reason why the patch is there rather than
>>> throwing out random thoughts or blunt comments.
>>>
>>> I did this patch mainly because weston/wayland has been using
>>> strtol/strtoul functions in number of places with buggy error checks, and
>>> duplicate code everywhere. Weston and wayland go together; so in bigger
>>> picture, its a very useful patch IMO.. I hardly find any patches with proper
>>> tests, but I did it so to make it more effective. But I guess in
>>> wayland/weston community, only maintainers are allowed to push patches
>>> others are strongly discouraged to do so. I guess its better to encourage
>>> people/community for giving helping hand.
>>>
>>> Anyways we will now only push patches (including multi-seat support) in
>>> Tizen weston/wayland rather than wasting time in upstreamn weston/wayland as
>>> it seems to be long bureaucratic process to overcome with virtually no
>>> success.
>>
>>
>> That's not what we've said. No-one said 'don't take the patch'; all we
>> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
>>
>> I like the idea of the patch, I like how it's caught a number of buggy
>> spots where we've open-coded the same thing, and I like that it's
>> well-tested. For internal usage, it's great! I just don't want to expose
>> string manipulation functions in the public API of a window system.
>>
>> You're right that the test infrastructure is in a pretty bad state.
>> Anything which helps that is more than welcome, and you may have seen a
>> bunch of patches from Derek Foreman (not a maintainer) on this list, which
>> are progressing well and go a long way towards improving our test suite into
>> something that will be really useful day to day. Any further contributions
>> along those lines - from anyone - are totally welcome.
>>
>> As for your multiseat patches, the last discussions I remember involved
>> some pretty fundamental disagreements about the whole architecture,
>> particularly input support. I haven't seen any more patches or discussion
>> since the last IRC chat, though.
>>
>> Cheers,
>> Daniel
>
>
On Wed, Oct 29, 2014 at 10:09 AM, Giulio Camuffo <giuliocamuffo@gmail.com>
wrote:

> 2014-10-29 8:45 GMT+02:00 Imran Zaman <imran.zaman@gmail.com>:
> > Daniel!
> >
> > As per your logic, I see wl_list APIs exposed etc, which shouldn't be
> part
> > of libwayland as well.
> > similarly, wl_fixed_to_double and wl_array shouldn't be part of the
> window
> > system. Isnt it?
> > I can make inline functions if that helps.
>
> wl_list is used in the server side API, so it's a bit different.
> However, I'd agree that it'd be better if it wasn't exposed but we
> can't remove it now. wl_fixed is a wayland specific type so all the
> wl_fixed_* functions need to be part of the API.
> On the other hand wl_strtol would just be a function, there are is no
> other API that depends on it.
>
> >
> > Btw here is an API patch, which has not be reviewed till now.
> >
> http://lists.freedesktop.org/archives/wayland-devel/2014-October/017833.html
>
> Yes, like there are many other patches waiting for reviews. You need
> to have patience, it's not like we are ignoring it. But, if I may add,
> the way you are reacting to a comment to this patch doesn't encourage
> to review your other ones.
>
>
>
Neither the random/comments to the patch are encouraging :-) e.g. "AOL.
We're a window system, not a replacement libc."
Its your choice to review it or not; I did not put the API patch link here
just because it has not been reviewed. I have lots of patience but Tizen
may need something urgent or else we may need to fork wayland/weston in
Tizen. I put it in the thread because Daniel said that we did not have any
further progress/discussion on that end.

Anyways I take the patch off, as it does not "sound" like it should be here
to save everyone's time. If the time allows, I will remove it from public
APIs in addition to one critical bug fix and resubmit the patch.



> --
> Giulio
>
> >
> > BR
> > imran
> >
> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org>
> wrote:
> >
> >> Hi,
> >>
> >> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
> >>>
> >>> You guys should check the reason why the patch is there rather than
> >>> throwing out random thoughts or blunt comments.
> >>>
> >>> I did this patch mainly because weston/wayland has been using
> >>> strtol/strtoul functions in number of places with buggy error checks,
> and
> >>> duplicate code everywhere. Weston and wayland go together; so in bigger
> >>> picture, its a very useful patch IMO.. I hardly find any patches with
> proper
> >>> tests, but I did it so to make it more effective. But I guess in
> >>> wayland/weston community, only maintainers are allowed to push patches
> >>> others are strongly discouraged to do so. I guess its better to
> encourage
> >>> people/community for giving helping hand.
> >>>
> >>> Anyways we will now only push patches (including multi-seat support) in
> >>> Tizen weston/wayland rather than wasting time in upstreamn
> weston/wayland as
> >>> it seems to be long bureaucratic process to overcome with virtually no
> >>> success.
> >>
> >>
> >> That's not what we've said. No-one said 'don't take the patch'; all we
> >> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
> >>
> >> I like the idea of the patch, I like how it's caught a number of buggy
> >> spots where we've open-coded the same thing, and I like that it's
> >> well-tested. For internal usage, it's great! I just don't want to expose
> >> string manipulation functions in the public API of a window system.
> >>
> >> You're right that the test infrastructure is in a pretty bad state.
> >> Anything which helps that is more than welcome, and you may have seen a
> >> bunch of patches from Derek Foreman (not a maintainer) on this list,
> which
> >> are progressing well and go a long way towards improving our test suite
> into
> >> something that will be really useful day to day. Any further
> contributions
> >> along those lines - from anyone - are totally welcome.
> >>
> >> As for your multiseat patches, the last discussions I remember involved
> >> some pretty fundamental disagreements about the whole architecture,
> >> particularly input support. I haven't seen any more patches or
> discussion
> >> since the last IRC chat, though.
> >>
> >> Cheers,
> >> Daniel
> >>
> >
> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org>
> wrote:
> >>
> >> Hi,
> >>
> >> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
> >>>
> >>> You guys should check the reason why the patch is there rather than
> >>> throwing out random thoughts or blunt comments.
> >>>
> >>> I did this patch mainly because weston/wayland has been using
> >>> strtol/strtoul functions in number of places with buggy error checks,
> and
> >>> duplicate code everywhere. Weston and wayland go together; so in bigger
> >>> picture, its a very useful patch IMO.. I hardly find any patches with
> proper
> >>> tests, but I did it so to make it more effective. But I guess in
> >>> wayland/weston community, only maintainers are allowed to push patches
> >>> others are strongly discouraged to do so. I guess its better to
> encourage
> >>> people/community for giving helping hand.
> >>>
> >>> Anyways we will now only push patches (including multi-seat support) in
> >>> Tizen weston/wayland rather than wasting time in upstreamn
> weston/wayland as
> >>> it seems to be long bureaucratic process to overcome with virtually no
> >>> success.
> >>
> >>
> >> That's not what we've said. No-one said 'don't take the patch'; all we
> >> said is 'please don't expose it as part of libwayland-*'s _public_ API'.
> >>
> >> I like the idea of the patch, I like how it's caught a number of buggy
> >> spots where we've open-coded the same thing, and I like that it's
> >> well-tested. For internal usage, it's great! I just don't want to expose
> >> string manipulation functions in the public API of a window system.
> >>
> >> You're right that the test infrastructure is in a pretty bad state.
> >> Anything which helps that is more than welcome, and you may have seen a
> >> bunch of patches from Derek Foreman (not a maintainer) on this list,
> which
> >> are progressing well and go a long way towards improving our test suite
> into
> >> something that will be really useful day to day. Any further
> contributions
> >> along those lines - from anyone - are totally welcome.
> >>
> >> As for your multiseat patches, the last discussions I remember involved
> >> some pretty fundamental disagreements about the whole architecture,
> >> particularly input support. I haven't seen any more patches or
> discussion
> >> since the last IRC chat, though.
> >>
> >> Cheers,
> >> Daniel
> >
> >
>
I will push new patch with minor fix to the strtol function in wayland and
move this old patch (after segfault fix) to weston so that it does not end
up in libwayland APIs.
Consequently I changed its property in patchwork

BR
imran

On Wed, Oct 29, 2014 at 10:27 AM, Imran Zaman <imran.zaman@gmail.com> wrote:

>
>
> On Wed, Oct 29, 2014 at 10:09 AM, Giulio Camuffo <giuliocamuffo@gmail.com>
> wrote:
>
>> 2014-10-29 8:45 GMT+02:00 Imran Zaman <imran.zaman@gmail.com>:
>> > Daniel!
>> >
>> > As per your logic, I see wl_list APIs exposed etc, which shouldn't be
>> part
>> > of libwayland as well.
>> > similarly, wl_fixed_to_double and wl_array shouldn't be part of the
>> window
>> > system. Isnt it?
>> > I can make inline functions if that helps.
>>
>> wl_list is used in the server side API, so it's a bit different.
>> However, I'd agree that it'd be better if it wasn't exposed but we
>> can't remove it now. wl_fixed is a wayland specific type so all the
>> wl_fixed_* functions need to be part of the API.
>> On the other hand wl_strtol would just be a function, there are is no
>> other API that depends on it.
>>
>> >
>> > Btw here is an API patch, which has not be reviewed till now.
>> >
>> http://lists.freedesktop.org/archives/wayland-devel/2014-October/017833.html
>>
>> Yes, like there are many other patches waiting for reviews. You need
>> to have patience, it's not like we are ignoring it. But, if I may add,
>> the way you are reacting to a comment to this patch doesn't encourage
>> to review your other ones.
>>
>>
>>
> Neither the random/comments to the patch are encouraging :-) e.g. "AOL.
> We're a window system, not a replacement libc."
> Its your choice to review it or not; I did not put the API patch link here
> just because it has not been reviewed. I have lots of patience but Tizen
> may need something urgent or else we may need to fork wayland/weston in
> Tizen. I put it in the thread because Daniel said that we did not have any
> further progress/discussion on that end.
>
> Anyways I take the patch off, as it does not "sound" like it should be
> here to save everyone's time. If the time allows, I will remove it from
> public APIs in addition to one critical bug fix and resubmit the patch.
>
>
>
>> --
>> Giulio
>>
>> >
>> > BR
>> > imran
>> >
>> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org>
>> wrote:
>> >
>> >> Hi,
>> >>
>> >> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>> >>>
>> >>> You guys should check the reason why the patch is there rather than
>> >>> throwing out random thoughts or blunt comments.
>> >>>
>> >>> I did this patch mainly because weston/wayland has been using
>> >>> strtol/strtoul functions in number of places with buggy error checks,
>> and
>> >>> duplicate code everywhere. Weston and wayland go together; so in
>> bigger
>> >>> picture, its a very useful patch IMO.. I hardly find any patches with
>> proper
>> >>> tests, but I did it so to make it more effective. But I guess in
>> >>> wayland/weston community, only maintainers are allowed to push patches
>> >>> others are strongly discouraged to do so. I guess its better to
>> encourage
>> >>> people/community for giving helping hand.
>> >>>
>> >>> Anyways we will now only push patches (including multi-seat support)
>> in
>> >>> Tizen weston/wayland rather than wasting time in upstreamn
>> weston/wayland as
>> >>> it seems to be long bureaucratic process to overcome with virtually no
>> >>> success.
>> >>
>> >>
>> >> That's not what we've said. No-one said 'don't take the patch'; all we
>> >> said is 'please don't expose it as part of libwayland-*'s _public_
>> API'.
>> >>
>> >> I like the idea of the patch, I like how it's caught a number of buggy
>> >> spots where we've open-coded the same thing, and I like that it's
>> >> well-tested. For internal usage, it's great! I just don't want to
>> expose
>> >> string manipulation functions in the public API of a window system.
>> >>
>> >> You're right that the test infrastructure is in a pretty bad state.
>> >> Anything which helps that is more than welcome, and you may have seen a
>> >> bunch of patches from Derek Foreman (not a maintainer) on this list,
>> which
>> >> are progressing well and go a long way towards improving our test
>> suite into
>> >> something that will be really useful day to day. Any further
>> contributions
>> >> along those lines - from anyone - are totally welcome.
>> >>
>> >> As for your multiseat patches, the last discussions I remember involved
>> >> some pretty fundamental disagreements about the whole architecture,
>> >> particularly input support. I haven't seen any more patches or
>> discussion
>> >> since the last IRC chat, though.
>> >>
>> >> Cheers,
>> >> Daniel
>> >>
>> >
>> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone <daniel@fooishbar.org>
>> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On 28 October 2014 15:40, Imran Zaman <imran.zaman@gmail.com> wrote:
>> >>>
>> >>> You guys should check the reason why the patch is there rather than
>> >>> throwing out random thoughts or blunt comments.
>> >>>
>> >>> I did this patch mainly because weston/wayland has been using
>> >>> strtol/strtoul functions in number of places with buggy error checks,
>> and
>> >>> duplicate code everywhere. Weston and wayland go together; so in
>> bigger
>> >>> picture, its a very useful patch IMO.. I hardly find any patches with
>> proper
>> >>> tests, but I did it so to make it more effective. But I guess in
>> >>> wayland/weston community, only maintainers are allowed to push patches
>> >>> others are strongly discouraged to do so. I guess its better to
>> encourage
>> >>> people/community for giving helping hand.
>> >>>
>> >>> Anyways we will now only push patches (including multi-seat support)
>> in
>> >>> Tizen weston/wayland rather than wasting time in upstreamn
>> weston/wayland as
>> >>> it seems to be long bureaucratic process to overcome with virtually no
>> >>> success.
>> >>
>> >>
>> >> That's not what we've said. No-one said 'don't take the patch'; all we
>> >> said is 'please don't expose it as part of libwayland-*'s _public_
>> API'.
>> >>
>> >> I like the idea of the patch, I like how it's caught a number of buggy
>> >> spots where we've open-coded the same thing, and I like that it's
>> >> well-tested. For internal usage, it's great! I just don't want to
>> expose
>> >> string manipulation functions in the public API of a window system.
>> >>
>> >> You're right that the test infrastructure is in a pretty bad state.
>> >> Anything which helps that is more than welcome, and you may have seen a
>> >> bunch of patches from Derek Foreman (not a maintainer) on this list,
>> which
>> >> are progressing well and go a long way towards improving our test
>> suite into
>> >> something that will be really useful day to day. Any further
>> contributions
>> >> along those lines - from anyone - are totally welcome.
>> >>
>> >> As for your multiseat patches, the last discussions I remember involved
>> >> some pretty fundamental disagreements about the whole architecture,
>> >> particularly input support. I haven't seen any more patches or
>> discussion
>> >> since the last IRC chat, though.
>> >>
>> >> Cheers,
>> >> Daniel
>> >
>> >
>>
>
>