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
On 30/06/13 11:13, Sergio Correia wrote:
From: Sergio Correia
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/checkforein... + * 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) {