On 30/06/13 11:13, Sergio Correia wrote: > From: Sergio Correia <[email protected]> > > Linux closes the descriptor unconditionally even if the close() call is > interrupted.
Is this also the case for other ELF using systems we support (BSD, HURD)? I believe it is but I am being too lazy to check. > This patch changes the CLOSE macro in libalpm not to loop on EINTR any > longer. Now it is simply a close() call. > > There was another site performing such a loop on EINTR after close, in > pacman/conf.c, and it was also changed to a simple close() call. > > Links for reference: > - http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html > - http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR > - > https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain > - http://ewontfix.com/4/ > - http://austingroupbugs.net/view.php?id=529 > > Signed-off-by: Sergio Correia <[email protected]> > --- > lib/libalpm/util.h | 10 +++++++++- > src/pacman/conf.c | 7 +++---- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h > index 24b7c22..09834af 100644 > --- a/lib/libalpm/util.h > +++ b/lib/libalpm/util.h > @@ -88,7 +88,15 @@ void _alpm_alloc_fail(size_t size); > #endif > > #define OPEN(fd, path, flags) do { fd = open(path, flags | O_BINARY); } > while(fd == -1 && errno == EINTR) > -#define CLOSE(fd) do { int _ret; do { _ret = close(fd); } while(_ret == -1 > && errno == EINTR); } while(0) > + Comments should not be Linux specific. Also, that comment is beyond excessive! > +/* Linux closes the file descriptor unconditionally, even if it returns > EINTR, and hence looping for such > + * condition after calling close() is wrong and can be potentially > dangerous, especially in multithreaded code. > + * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html > + * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR > + * > https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain > + * http://ewontfix.com/4/ > + * http://austingroupbugs.net/view.php?id=529 */ > +#define CLOSE(fd) do { close(fd); } while(0) > > /** > * Used as a buffer/state holder for _alpm_archive_fgets(). > diff --git a/src/pacman/conf.c b/src/pacman/conf.c > index 5adc96c..8a42306 100644 > --- a/src/pacman/conf.c > +++ b/src/pacman/conf.c > @@ -257,14 +257,13 @@ static int download_with_xfercommand(const char *url, > const char *localpath, > cleanup: > /* restore the old cwd if we have it */ > if(cwdfd >= 0) { > - int close_ret; > if(fchdir(cwdfd) != 0) { > pm_printf(ALPM_LOG_ERROR, _("could not restore working > directory (%s)\n"), > strerror(errno)); > } > - do { > - close_ret = close(cwdfd); > - } while(close_ret == -1 && errno == EINTR); > + /* looping for EINTR on close() is wrong on Linux, as it closes > the > + * descriptor unconditionally even if the close() call is > interrupted.*/ > + close(cwdfd); > } > > if(ret == -1) { >
