Hi, sorry for the delay, I got stuck in some stuff from my day job.
On Sat, Jun 29, 2013 at 9:46 PM, Allan McRae <[email protected]> wrote: > 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. > > Good question. I am not sure about it, gotta check to make sure. In fact I wasn't even aware we supported such systems :) > > This patch changes the CLOSE macro in libalpm not to loop on EINTR any > > longer. Now it is simply a close() call. > [snip] > > @@ -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! > > Makes a lot of sense, especially after finding out we support other systems. Thanks for the feedback, Sergio > +/* 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) { > > > > >
