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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2016, 17:02 UTC
Message-ID: <xmqqshrks6ot.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1610251230150.3264@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 11 quoted lines

> 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);
> 	}

I deliberately separated the part that can and designed to be toggled (O_CLOEXEC) and the part that is meant to be constant (O_RDONLY), and I do not think the first part of suggestion is particularly a good idea.

I didn't write the same open twice, but I agree that an extra "we fallback to opening with a different flags" inside the if () { } block that is there exactly for implementing that fallback is an excellent idea. I like that part of the suggestion.

Thanks.