Lars Schneider <larsxschneider@xxxxxxxxx> wrote:
> > On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@xxxxxx> wrote:
> > On Mon, 5 Sep 2016, Eric Wong wrote:
> >> larsxschneider@xxxxxxxxx wrote:
> >>> -int git_open_noatime(const char *name)
> >>> +int git_open_noatime_cloexec(const char *name)
> >>> {
> >>> - static int sha1_file_open_flag = O_NOATIME;
> >>> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
> >>>
> >>> for (;;) {
> >>> int fd;
> >
> >> I question the need for the "_cloexec" suffixing in the
> >> function name since the old function is going away entirely.
> >
> > Me, too. While it is correct, it makes things harder to read, so it may
> > even cause more harm than it does good.
>
> What name would you suggest? Leaving the name as-is seems misleading to me.
> Maybe just "git_open()" ?
Maybe "_noatime" is useful in some cases, but maybe not *shrug*
My original point for removing the "_cloexec" suffix was that
(at least for Perl and Ruby), cloexec-by-default was so prevalent
in FD-creating syscalls that having the suffix wasn't needed.
> >> I prefer all FD-creating functions set cloexec by default
> >> for FD > 2 to avoid inadvertantly leaking FDs. So we
> >> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
> >> and fallback to the racy+slower F_SETFD when not available.
> I applied the same mechanism here. Would that be OK?
>
> Thanks,
> Lars
>
> - static int sha1_file_open_flag = O_NOATIME;
> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
>
> for (;;) {
> int fd;
> @@ -1471,12 +1471,17 @@ 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)) {
80 columns overflow
> + sha1_file_open_flag &= ~O_CLOEXEC;
> 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;
> + }
But otherwise much better since it doesn't blindly zero
sha1_file_open_flag :>