[webkit-reviews] review granted: [Bug 117266] [WK2] Looping for EINTR on close() is incorrect for Linux, at least : [Attachment 205002] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Correia
<sergio.correia at openbossa.org>'s request for review:
Bug 117266: [WK2] Looping for EINTR on close() is incorrect for Linux, at least
https://bugs.webkit.org/show_bug.cgi?id=117266

Attachment 205002: Patch
https://bugs.webkit.org/attachment.cgi?id=205002&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205002&action=review


> Source/WebKit2/ChangeLog:44
> +	   POSIX says the following for close():
> +
> +	   If close() is interrupted by a signal that is to be caught, it shall

> +	   return -1 with errno set to [EINTR] and the state of fildes is
> +	   unspecified.
> +
> +	   Different implementations do different things when close() is
interrupted
> +	   by a signal: HP-UX leaves the file descriptor open while returning
EINTR,
> +	   while Linux and AIX, for instance, close the descriptor
unconditionally.
> +
> +	   As someone pointed out in the discussion in [1], the actual problem
with
> +	   this behavior is not knowing how to safely proceed when getting
EINTR
> +	   during a close() call. In single-threaded applications, there's not
much of
> +	   a problem, since one could just call close() again and get EBADF if
the
> +	   descriptor had been closed the first time; however, in multithreaded

> +	   applications it becomes dangerous to call close() again, since it's
> +	   possible s subsequent call could close a descriptor just obtained by

> +	   another thread.
> +
> +	   As noted in [5], the Austin Group resolved the issue [6] by
requiring
> +	   the HPUX behavior if close returns with EINTR, but allowing
implementations
> +	   which want to deallocate the file descriptor even if interrupted by
a
> +	   signal to return with the EINPROGRESS error instead of EINTR.
> +
> +	   The ``while (close(fd) == -1 && errno == EINTR) { }'' idiom seems to
be
> +	   very popular, especially in WK2 code, and since it's the wrong thing
to do
> +	   at least on Linux, this patch is changing these sites to use a new
function
> +	   closeWithRetry() added in this commit, which currently works around
the
> +	   described Linux behavior.
> +
> +	   Some links for reference:
> +	   [1] https://news.ycombinator.com/item?id=3363819
> +	   [2] http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
> +	   [3]
https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrw
heninvokingclosethinkagain
> +	   [4] http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
> +	   [5] http://ewontfix.com/4/
> +	   [6] http://austingroupbugs.net/view.php?id=529

Comment is too big. The comment in WTF/ChangeLog is better. Just use something
like that. “Call closeWithRetry to work around a difference in how the retry
needs to be done on Linux.”

> Source/WTF/wtf/UniStdExtras.h:2
> +#ifndef UniStdExtras_h
> +#define UniStdExtras_h

These header guards go after the comment in the WebKit coding style.