Re: [PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open

[Johannes Schindelin]

Hi Max,

On 2015-10-01 05:29, Max Kirillov wrote:
> Windows does not support setting O_CLOEXEC by fcntl,
> but there is an open flag O_NOINHERIT which results in same
> behaviour. Use it in git_open_noatime() and also bring
> setting O_CLOEXEC there also to make it consistent. Rename
> the function to git_open_noatime_cloexec(), to avoid confusion.

Which problem does this solve? I am actually suspecting that this will rather 
cause problems because now `exec()`ing children might cause bogus file 
descriptors to be still held on.

So I would recommend to drop this patch. It is not needed to fix the reported 
bug.

Ciao,
Dscho

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open

[Max Kirillov]

Windows does not support setting O_CLOEXEC by fcntl,
but there is an open flag O_NOINHERIT which results in same
behaviour. Use it in git_open_noatime() and also bring
setting O_CLOEXEC there also to make it consistent. Rename
the function to git_open_noatime_cloexec(), to avoid confusion.

Signed-off-by: Max Kirillov 
---
 builtin/pack-objects.c |  2 +-
 cache.h|  2 +-
 pack-bitmap.c  |  2 +-
 sha1_file.c| 48 
 4 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1c63f8f..a8052f4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -714,7 +714,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
 
-   fd = git_open_noatime(reuse_packfile->pack_name);
+   fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
  reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 153bc46..77ef5ca 100644
--- a/cache.h
+++ b/cache.h
@@ -972,7 +972,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 git_open_noatime(const char *name);
+extern int git_open_noatime_cloexec(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 parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 637770a..4a3b23c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -274,7 +274,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
 
idx_name = pack_bitmap_filename(packfile);
-   fd = git_open_noatime(idx_name);
+   fd = git_open_noatime_cloexec(idx_name);
free(idx_name);
 
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 62f1dad..2da5de2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -385,7 +385,7 @@ void read_info_alternates(const char * relative_base, int 
depth)
int fd;
 
sprintf(path, "%s/%s", relative_base, alt_file_name);
-   fd = git_open_noatime(path);
+   fd = git_open_noatime_cloexec(path);
if (fd < 0)
return;
if (fstat(fd, ) || (st.st_size == 0)) {
@@ -575,7 +575,7 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
-   int fd = git_open_noatime(path);
+   int fd = git_open_noatime_cloexec(path);
struct stat st;
 
if (fd < 0)
@@ -995,7 +995,6 @@ static int open_packed_git_1(struct packed_git *p)
struct pack_header hdr;
unsigned char sha1[20];
unsigned char *idx_sha1;
-   long fd_flag;
 
if (!p->index_data && open_pack_index(p))
return error("packfile %s index unavailable", p->pack_name);
@@ -1013,7 +1012,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
 
-   p->pack_fd = git_open_noatime(p->pack_name);
+   p->pack_fd = git_open_noatime_cloexec(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, ))
return -1;
pack_open_fds++;
@@ -1026,16 +1025,6 @@ static int open_packed_git_1(struct packed_git *p)
} else if (p->pack_size != st.st_size)
return error("packfile %s size changed", p->pack_name);
 
-   /* We leave these file descriptors open with sliding mmap;
-* there is no point keeping them open across exec(), though.
-*/
-   fd_flag = fcntl(p->pack_fd, F_GETFD, 0);
-   if (fd_flag < 0)
-   return error("cannot determine file descriptor flags");
-   fd_flag |= FD_CLOEXEC;
-   if (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)
-   return error("cannot set FD_CLOEXEC");
-
/* Verify we recognize this pack file format. */
if (read_in_full(p->pack_fd, , sizeof(hdr)) != sizeof(hdr))
return error("file %s is far too short to be a packfile", 
p->pack_name);
@@ -1515,17 +1504,21 @@ int check_sha1_signature(const unsigned char *sha1, 
void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
-int git_open_noatime(const char *name)
+int