Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 25, 2016, 10:33 UTC
Message-ID: <alpine.DEB.2.20.1610251230150.3264@virtualbox>
In-Reply-To: <xmqqwpgx4j89.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 24 Oct 2016, Junio C Hamano wrote:

Show 47 quoted lines

> Eric Wong <e@80x24.org> writes:
> 
> > larsxschneider@gmail.com wrote:
> >> +++ b/read-cache.c
> >> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> >>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> >>  {
> >>  	int match = -1;
> >> -	int fd = open(ce->name, O_RDONLY);
> >> +	int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> >> +
> >> +	if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> >> +		/* Try again w/o O_CLOEXEC: the kernel might not support it */
> >> +		fd = open(ce->name, O_RDONLY);
> >
> > In the case of O_CLOEXEC != 0 and repeated EINVALs,
> > it'd be good to use something like sha1_file_open_flag as in 1/2
> > so we don't repeatedly hit EINVAL.  Thanks.
> 
> Sounds sane.  
> 
> It's just only once, so perhaps we do not mind a recursion like
> this?
> 
>  read-cache.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index b594865d89..a6978b9321 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>  {
>  	int match = -1;
> -	int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> +	static int cloexec = O_CLOEXEC;
> +	int fd = open(ce->name, O_RDONLY | cloexec);
>  
> -	if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> +	if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
>  		/* Try again w/o O_CLOEXEC: the kernel might not support it */
> -		fd = open(ce->name, O_RDONLY);
> +		cloexec &= ~O_CLOEXEC;
> +		return ce_compare_data(ce, st);
> +	}
>

That still looks overly complicated, repeatedly ORing cloexec and recursing without need. How about this instead?

	static int oflags = O_RDONLY | O_CLOEXEC;
	int fd = open(ce->name, oflags);

	if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
  		/* Try again w/o O_CLOEXEC: the kernel might not support it */
		oflags &= ~O_CLOEXEC;
		fd = open(ce->name, oflags);
	}

Ciao, Dscho