Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Lars,

On Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> All processes that the Git main process spawns inherit the open file
> descriptors of the main process. These leaked file descriptors can
> cause problems.
> 
> Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
> descriptors. Since `git_open_noatime` does not describe the function
> properly anymore rename it to `git_open`.

The patch series may be a little bit more pleasant to read if you renamed
git_open_noatime() to git_open() first, in a separate commit.

> @@ -1598,12 +1598,18 @@ int git_open_noatime(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 (O_CLOEXEC && errno == EINVAL &&
> +			(sha1_file_open_flag & O_CLOEXEC)) {
> +			sha1_file_open_flag &= ~O_CLOEXEC;

How about

		if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
			sha1_file_open_flag &= ~O_CLOEXEC;

instead? It is shorter and should be just as easily optimized out by a
C compiler if O_CLOEXEC was defined as 0.

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

I *think* the --patience diff option would have made that patch a little
more obvious.

Otherwise, the patch looks fine to me,
Dscho