git_open(): untangle possible NOATIME and CLOEXEC interactions

The way we structured the fallback/retry mechanism 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.

Instead use O_CLOEXEC to open the file, and then use fcntl(2) to set
O_NOATIME on the resulting file descriptor.  open(2) itself does not
cause atime to be updated according to Linus [*1*].

The helper to do the former can be usable in the codepath in
ce_compare_data() that was recently added to open a file descriptor
with O_CLOEXEC; use it while we are at it.

*1* <CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com>

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index a902ca1f8e4d63ebc2a4605b03ac4369eaaaa566..6f1c21a35240ada37fed213992262bbd9c3e3fa1 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);

+extern int git_open_cloexec(const char *name, int flags);

 extern int git_open(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int git_open(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);

diff --git a/read-cache.c b/read-cache.c
index db5d910642663e73e4e4fc8d91e0caba4445bdb1..c27d3e240b06fc75fd2deac190a764fce28ed059 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,14 +156,7 @@ 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 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 = git_open_cloexec(ce->name, O_RDONLY);

 
        if (fd >= 0) {
                unsigned char sha1[20];
 
        if (fd >= 0) {
                unsigned char sha1[20];

diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dcd8cf3dcda47ef6114ee68eb4c5f3be..dfbf398183d5b9593b56c27b8cf5011d0ddd5e84 100644 (file)
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1559,31 +1559,30 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
        return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
        return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 

-int git_open(const char *name)
+int git_open_cloexec(const char *name, int flags)

 {
 {

-       static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
-
-       for (;;) {
-               int fd;
-
-               errno = 0;
-               fd = open(name, O_RDONLY | sha1_file_open_flag);
-               if (fd >= 0)
-                       return fd;
+       static int cloexec = O_CLOEXEC;
+       int fd = open(name, flags | cloexec);

 
 

+       if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {

                /* Try again w/o O_CLOEXEC: the kernel might not support it */
                /* 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;
-               }
+               cloexec &= ~O_CLOEXEC;
+               fd = open(name, flags | cloexec);
+       }
+       return fd;
+}

 
 

-               /* 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;
+int git_open(const char *name)
+{
+       static int noatime = O_NOATIME;
+       int fd = git_open_cloexec(name, O_RDONLY);
+
+       if (0 <= fd && (noatime & O_NOATIME)) {
+               int flags = fcntl(fd, F_GETFL);
+               if (fcntl(fd, F_SETFL, flags | noatime))
+                       noatime = 0;

        }
        }

+       return fd;

 }
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
 }
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)