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

From: Jeff King <peff@peff.net>
Date: Oct 26, 2016, 04:25 UTC
Message-ID: <20161026042555.neaxvnmggtcku5cc@sigill.intra.peff.net>
In-Reply-To: <20161025181621.4201-3-gitster@pobox.com>

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

Show 16 quoted lines

> 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...

Show 5 quoted lines

> +		/* 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