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

From: Eric Wong <e@80x24.org>
Date: Sep 7, 2016, 18:17 UTC
Message-ID: <20160907181747.GB14931@starla>
In-Reply-To: <F8E7B7CE-1177-4CBD-999E-21C593A8ACD2@gmail.com>

Lars Schneider <larsxschneider@gmail.com> wrote:

Show 20 quoted lines

> > On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > On Mon, 5 Sep 2016, Eric Wong wrote:
> >> larsxschneider@gmail.com 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.

Show 19 quoted lines

> 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

Show 9 quoted lines

> +                       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 :>