Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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 wrote: > > > On Wed, Oct 29, 2014 at 10:09 AM, Giulio Camuffo > wrote: > >> 2014-10-29 8:45 GMT+02:00 Imran Zaman : >> > 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 >> wrote: >> > >> >> Hi, >> >> >> >> On 28 October 2014 15:40, Imran Zaman 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 >> wrote: >> >> >> >> Hi, >> >> >> >> On 28 October 2014 15:40, Imran Zaman 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
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
On Wed, Oct 29, 2014 at 10:09 AM, Giulio Camuffo wrote: > 2014-10-29 8:45 GMT+02:00 Imran Zaman : > > 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 > wrote: > > > >> Hi, > >> > >> On 28 October 2014 15:40, Imran Zaman 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 > wrote: > >> > >> Hi, > >> > >> On 28 October 2014 15:40, Imran Zaman 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
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
2014-10-29 8:45 GMT+02:00 Imran Zaman : > 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 wrote: > >> Hi, >> >> On 28 October 2014 15:40, Imran Zaman 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 wrote: >> >> Hi, >> >> On 28 October 2014 15:40, Imran Zaman 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
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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 wrote: > Hi, > > On 28 October 2014 15:40, Imran Zaman 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 wrote: > Hi, > > On 28 October 2014 15:40, Imran Zaman 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 > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
Hi, On 28 October 2014 15:40, Imran Zaman 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 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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 wrote: > Hi, > > On 28 October 2014 11:21, Giulio Camuffo wrote: > >> 2014-10-27 18:51 GMT+02:00 Jasper St. Pierre : >> > 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 > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
Hi, On 28 October 2014 11:21, Giulio Camuffo wrote: > 2014-10-27 18:51 GMT+02:00 Jasper St. Pierre : > > 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 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
2014-10-27 18:51 GMT+02:00 Jasper St. Pierre :
> 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 wrote:
>>
>>
>>
>> On 22 October 2014 13:32, Imran Zaman 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
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> +#include
>>>
>>> #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;
>>> +
>>>
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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 wrote:
>
>
> On 22 October 2014 13:32, Imran Zaman 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>> +#include
>>
>> #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
>>
Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
On 22 October 2014 13:32, Imran Zaman 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
> ---
> 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
> #include
> #include
> +#include
> +#include
>
> #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
> #include
> #include
> +#include
>
> /* 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
[PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
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
---
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))
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
#include
#include
+#include
+#include
#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
#include
#include
+#include
/* 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
