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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2016, 16:48 UTC
Message-ID: <xmqqshrg1ksv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 18 quoted lines

> On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>
>> You guys. I mean: You guys! You sure make my life hard. A brief look at
>> mingw.h could have answered your implicit question:
>
> So here's what you guys should do:
>
>  - leave O_NOATIME damn well alone. It works. It has worked for 10+
> years. Stop arguing against it, people who do.
>
>  - get rid of all O_CLOEXEC games. They don't work. If you want to
> close file descriptors at execve(), you - gasp - close the file
> descriptor before doing an execve.
>
> So O_CLOEXEC or FD_CLOEXEC is broken.
>
> DO NOT BREAK O_NOATIME JUST TO ADD COMPLETELY NEW BREAKAGE.

Excuse me, but care to elaborate a bit more?

Did I botch the way I ask fcntl(2) to set O_NOATIME in *1*? Its follow-up in *2* is optional and as peff said in *3*, I do not think we mind dropping that step and keeping O_NOATIME. It is not that much code to keep.

Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way it is broken to open(2) with O_CLOEXEC?

If we want to close file descriptors we opened at execve() time ourselves, we'd need to somehow keep track of which file descriptors are meant to be closed upon execve() time, and one way to mark that may be to use O_CLOEXEC, but once we mark a file descriptor with the bit, execve() would take care of "closing" it for us---wouldn't that be the whole point of that bit?

*1* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com *2* http://public-inbox.org/git/xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com *3* http://public-inbox.org/git/20161028075104.la24zydnr3ogb6qv@sigill.intra.peff.net