Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

[Junio C Hamano <gitster@pobox.com>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Would the best endgame shape for this function be to open with
>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
>> but ignoring an error from it, I guess?  That would be the closest
>> to what we historically had, I would think.
>
> I think that's the best model.

OK, so perhaps like this.

-- >8 --
Subject: git_open(): untangle possible NOATIME and CLOEXEC interactions

The way we structured the fallback-retry for opening with O_NOATIME
and O_CLOEXEC meant that if we failed due to lack of support to open
the file with O_NOATIME option (i.e. EINVAL), we would still try to
drop O_CLOEXEC first and retry, and then drop O_NOATIME.  A platform
on which O_NOATIME is defined in the header without support from the
kernel wouldn't have a chance to open with O_CLOEXEC option due to
this code structure.

Arguably, O_CLOEXEC is more important than O_NOATIME, as the latter
is mostly about performance, while the former can affect correctness.
Let's revert the recent changes to the way git_open() attempts to
open a file with O_NOATIME and retries without to the original
sequence, and then use a separate fcntl(fd, F_SETFD, FD_CLOEXEC) on
the resulting file descriptor.  The helper to do the latter can be
usable in the codepath in ce_compare_data() that was recently added
to open a file descriptor with O_CLOEXEC, so let's refactor that
codepath with the helper while we are at it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |  5 +++--
 read-cache.c      | 12 ++++--------
 sha1_file.c       | 49 ++++++++++++++++++++++++++++++-------------------
 3 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 43718dabae..a751630db5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -679,9 +679,10 @@ char *gitstrdup(const char *s);
 #define getpagesize() sysconf(_SC_PAGESIZE)
 #endif
 
-#ifndef O_CLOEXEC
-#define O_CLOEXEC 0
+#ifndef FD_CLOEXEC
+#define FD_CLOEXEC 0
 #endif
+extern int git_set_cloexec(int);
 
 #ifdef FREAD_READS_DIRECTORIES
 #ifdef fopen
diff --git a/read-cache.c b/read-cache.c
index db5d910642..fb91514885 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,17 +156,13 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
 	int match = -1;
-	static int cloexec = O_CLOEXEC;
-	int fd = open(ce->name, O_RDONLY | cloexec);
-
-	if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
-		/* Try again w/o O_CLOEXEC: the kernel might not support it */
-		cloexec &= ~O_CLOEXEC;
-		fd = open(ce->name, O_RDONLY | cloexec);
-	}
+	int fd = open(ce->name, O_RDONLY);
 
 	if (fd >= 0) {
 		unsigned char sha1[20];
+
+		/* do not let child processes to hold onto the open fd */
+		git_set_cloexec(fd);
 		if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
 			match = hashcmp(sha1, ce->oid.hash);
 		/* index_fd() closed the file descriptor already */
diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..41383a6c20 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1559,31 +1559,42 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
 	return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
-int git_open(const char *name)
+int git_set_cloexec(int fd)
 {
-	static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
+	static int cloexec = FD_CLOEXEC;
 
-	for (;;) {
-		int fd;
+	if (cloexec) {
+		if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
+			cloexec = 0;
+		/*
+		 * We might want to diagnose and complain upon seeing
+		 * an error from this call, but let's keep the same
+		 * behaviour as before for now.
+		 */
+	}
+	return 0;
+}
 
-		errno = 0;
-		fd = open(name, O_RDONLY | sha1_file_open_flag);
-		if (fd >= 0)
-			return fd;
+int git_open(const char *name)
+{
+	static int noatime = O_NOATIME;
+	int fd;
 
-		/* Try again w/o O_CLOEXEC: the kernel might not support it */
-		if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
-			sha1_file_open_flag &= ~O_CLOEXEC;
-			continue;
-		}
+	errno = 0;
+	fd = open(name, O_RDONLY | noatime);
 
-		/* Might the failure be due to O_NOATIME? */
-		if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
-			sha1_file_open_flag &= ~O_NOATIME;
-			continue;
-		}
-		return -1;
+	/* Might the failure be due to O_NOATIME? */
+	if ((noatime & O_NOATIME) && errno != ENOENT) {
+		noatime = 0;
+		fd = open(name, O_RDONLY);
 	}
+
+	if (fd < 0)
+		return fd;
+
+	/* do not let child processes to hold onto the open fd */
+	git_set_cloexec(fd);
+	return fd;
 }
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)