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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 25, 2016, 10:27 UTC
Message-ID: <alpine.DEB.2.20.1610251220280.3264@virtualbox>
In-Reply-To: <20161024180300.52359-2-larsxschneider@gmail.com>

Hi Lars,

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

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

>  			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