Re: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> [Subject: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR]

close never returns with errno == EINTR on Linux, but it can happen on
Solaris, for example.

I can't think of a reason git would want to call close() without this
loop.  Maybe it would make sense to wrap close() unconditionally (see
below).

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -141,6 +141,21 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
>  	}
>  }
>  
> +/*
> + * xclose() is the same a close(), but it automatically restarts close()
> + * operations with a recoverable error (EINTR).
> + */

If there is to be an xclose, I think I'd say something like

	/*
	 * xclose() is like close(), but it retries if interrupted by
	 * a signal on platforms like Solaris that allow that to avoid
	 * unnecessarily leaking a file descriptor.  It quietly returns
	 * -1 with errno set appropriately on failure.
	 */

to avoid confusion with the semantics of xmalloc.

> +int xclose(int fd)
> +{
> +	int ret;
> +	while (1) {
> +		ret = close(fd);
> +		if ((ret < 0) && errno == EINTR)
> +			continue;
> +		return ret;
> +	}
> +}

Micronit: close() can only return 0 or -1, so this can be written more
simply as

	while (close(fd)) {
		if (errno == EINTR)
			continue;
		return -1;
	}
	return 0;

Untested.

-- >8 --
Subject: compat: wrap close() to avoid having to worry about EINTR

On some non-Linux platforms, close() can return EINTR to indicate
interruption by a signal.  In a poll or select loop, that could be
good behavior to prevent hangs, but that doesn't apply anywhere within
git.  Let close() loop unconditionally in this case to avoid
preventable file descriptor leaks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-compat-util.h |   12 +++++++++++-
 wrapper.c         |   15 ---------------
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 6e06ad4..1326edb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -428,7 +428,6 @@ extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
-extern int xclose(int fd);
 extern int xdup(int fd);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
@@ -450,6 +449,17 @@ static inline int has_extension(const char *filename, const char *ext)
 	return len > extlen && !memcmp(filename + len - extlen, ext, extlen);
 }
 
+/* Sane close - resumes after interruption by signals */
+static inline int git_close(int fd)
+{
+	while (close(fd)) {
+		if (errno != EINTR)
+			return -1;
+	}
+	return 0;
+}
+#define close git_close
+
 /* Sane ctype - no locale, and works with signed chars */
 #undef isascii
 #undef isspace
diff --git a/wrapper.c b/wrapper.c
index 717e989..2829000 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -141,21 +141,6 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
-/*
- * xclose() is the same a close(), but it automatically restarts close()
- * operations with a recoverable error (EINTR).
- */
-int xclose(int fd)
-{
-	int ret;
-	while (1) {
-		ret = close(fd);
-		if ((ret < 0) && errno == EINTR)
-			continue;
-		return ret;
-	}
-}
-
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;
-- 
1.7.5.1