Interesting read.. have some arguments about the raised points.. but
lets not go in that direction..
Can you guys suggest to do some changes or shall i drop the patch
altogether? if you are happy with current implementation lets live
with it.. I noticed bug when weston was not working properly due to
improper error handling of strtol in one place, so I thought it would
be good idea to fix it wherever it is used (and hence this patch) but
seems that it is not always the case :-)
BR
imran
p.s: To me it was really simple to understand that in MOST (=NOT ALL)
of the cases str-to-number conversion was done using strtol/strtoul
but without proper error handling. I moved it to single place to make
the life easier IMHO...
On Mon, Dec 1, 2014 at 8:30 PM, Bill Spitzak <spit...@gmail.com> wrote:
> On 12/01/2014 04:10 AM, Pekka Paalanen wrote:
>
>> other = strtol(pid, &end, 0);
>> if (end != pid + 10) {
>> weston_log("can't parse lock file %s\n",
>> lockfile);
>> close(fd);
>> errno = EEXIST;
>> return -1;
>> }
>
>
>> 'pid' is a fixed size string read in from the lock file, which is
>> converted into a number of type pid_t. Because the number is assumed to
>> be printed by "%10d\n", the file should have at least 11 bytes
>> available, and we assume all the 10 characters form a valid number
>> (with leading spaces). It's all just a way to avoid dealing with
>> unexpected EOF when reading the file, and to avoid not knowing in
>> advance how many characters long the number is.
>>
>> This code still wants to parse the whole string as a single number, but
>> it also knows the number will end in a newline instead of nul. It
>> wouldn't be difficult to replace that newline with nul before parsing,
>> if you really want to convert this code to use a helper. But while
>> doing so, you have to ask yourself: does this actually make the code
>> any easier to understand or more correct?
>
>
> I believe you are correct, and this is a good indication that blindly
> inserting the replacement function is not a good idea.
>
> The original code failed if there was a digit at offset 11 (as well as other
> reasons for failing). The proposed replacement failed if offset 11 was
> anything other than NUL. This is different. When I said the endptr was not
> needed, my proposal actually has the exact same mistake.
>
> This does bring up a question as to whether the helper function should eat
> trailing whitespace.
>
> But also that you can't drop the wrapper in everywhere.
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
