@matheusbsbs / matheus-bagliano / commits / 1b8ac5ead5

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>

Junio C Hamano committed Oct 28, 2016 at 06:23 UTC 1b8ac5ead520146802debd52cb38e5c27b3483a2

3 files changed +21 -28

cache.h

@@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,

1122 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);

1123 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);

1124 extern int force_object_loose(const unsigned char *sha1, time_t mtime);

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

1126 extern int git_open(const char *name);

1127 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);

1128 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);

read-cache.c

+1 -8

@@ -156,14 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)

156 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)

157 {

158 int match = -1;

159 - static int cloexec = O_CLOEXEC;

160 - int fd = open(ce->name, O_RDONLY | cloexec);

161 -

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

163 - /* Try again w/o O_CLOEXEC: the kernel might not support it */

164 - cloexec &= ~O_CLOEXEC;

165 - fd = open(ce->name, O_RDONLY | cloexec);

166 - }

159 + int fd = git_open_cloexec(ce->name, O_RDONLY);

160

161 if (fd >= 0) {

162 unsigned char sha1[20];

sha1_file.c

+19 -20

@@ -1559,31 +1559,30 @@ int check_sha1_signature(const unsigned char *sha1, void *map,

1559 return hashcmp(sha1, real_sha1) ? -1 : 0;

1560 }

1561

1562 -int git_open(const char *name)

1562 +int git_open_cloexec(const char *name, int flags)

1563 {

1564 - static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;

1565 -

1566 - for (;;) {

1567 - int fd;

1568 -

1569 - errno = 0;

1570 - fd = open(name, O_RDONLY | sha1_file_open_flag);

1571 - if (fd >= 0)

1572 - return fd;

1564 + static int cloexec = O_CLOEXEC;

1565 + int fd = open(name, flags | cloexec);

1566

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

1568 /* Try again w/o O_CLOEXEC: the kernel might not support it */

1575 - if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {

1576 - sha1_file_open_flag &= ~O_CLOEXEC;

1577 - continue;

1578 - }

1569 + cloexec &= ~O_CLOEXEC;

1570 + fd = open(name, flags | cloexec);

1571 + }

1572 + return fd;

1573 +}

1574

1580 - /* Might the failure be due to O_NOATIME? */

1581 - if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {

1582 - sha1_file_open_flag &= ~O_NOATIME;

1583 - continue;

1584 - }

1585 - return -1;

1575 +int git_open(const char *name)

1576 +{

1577 + static int noatime = O_NOATIME;

1578 + int fd = git_open_cloexec(name, O_RDONLY);

1579 +

1580 + if (0 <= fd && (noatime & O_NOATIME)) {

1581 + int flags = fcntl(fd, F_GETFL);

1582 + if (fcntl(fd, F_SETFL, flags | noatime))

1583 + noatime = 0;

1584 }

1585 + return fd;

1586 }

1587

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