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

[Jeff King <peff@peff.net>]

On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote:

> diff --git a/sha1_file.c b/sha1_file.c
> index 5d2bcd3ed1..09045df1dc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1571,12 +1571,17 @@ int git_open(const char *name)
>  		if (fd >= 0)
>  			return fd;
>  
> -		/* Might the failure be due to O_NOATIME? */
> -		if (errno != ENOENT && sha1_file_open_flag) {
> -			sha1_file_open_flag = 0;
> +		/* 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;
>  		}

So if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try
again with just O_NOATIME. And then if _that_ fails...

> +		/* Might the failure be due to O_NOATIME? */
> +		if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> +			sha1_file_open_flag &= ~O_NOATIME;
> +			continue;
> +		}

We drop O_NOATIME, and end up with an empty flag field.

But we will never have tried just O_CLOEXEC, which might have worked.

Because of the order here, this would not be a regression (i.e., any
system that used to work will still eventually find a working comb), but
it does mean that systems without O_NOATIME do not get the benefit of
the new O_CLOEXEC protection.

Unfortunately, I think covering all of the cases would be 2^nr_flags.
That's only 4 right now, but it does not bode well as a pattern.

I'm not sure it's worth worrying about or not; I don't know which
systems are actually lacking either of the flags, or if they tend to
have both.

-Peff