[PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[Joachim Schmitz]

Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
---
 sha1_file.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..76714ad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
                return error("packfile %s index unavailable", p->pack_name);

        if (!pack_max_fds) {
+#ifdef _SC_OPEN_MAX
+               unsigned int max_fds = sysconf(_SC_OPEN_MAX);
+#else
                struct rlimit lim;
                unsigned int max_fds;

@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
                        die_errno("cannot get RLIMIT_NOFILE");

                max_fds = lim.rlim_cur;
+#endif

                /* Save 3 for stdin/stdout/stderr, 22 for work */
                if (25 < max_fds)
--
1.7.12

Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

> Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
> ---
>  sha1_file.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index af5cfbd..76714ad 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
>                 return error("packfile %s index unavailable", p->pack_name);
>
>         if (!pack_max_fds) {
> +#ifdef _SC_OPEN_MAX
> +               unsigned int max_fds = sysconf(_SC_OPEN_MAX);
> +#else
>                 struct rlimit lim;
>                 unsigned int max_fds;
>
> @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
>                         die_errno("cannot get RLIMIT_NOFILE");
>
>                 max_fds = lim.rlim_cur;
> +#endif
>
>                 /* Save 3 for stdin/stdout/stderr, 22 for work */
>                 if (25 < max_fds)
> --
> 1.7.12

Looks sane but it would be more readable to make this a small helper
function, so that we do not need to have #ifdef/#endif in the
primary flow of the code.

By the way, I noticed that you seem to be sending patches out of
git, instead of "diff -ru", which is a good sign ;-).  But all of
your patches are whitespace damaged and cannot be applied X-<.

RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, August 22, 2012 7:23 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
> 
> "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> 
> > Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
> > ---
> >  sha1_file.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
> >                 return error("packfile %s index unavailable",
> > p->pack_name);
> >
> >         if (!pack_max_fds) {
> > +#ifdef _SC_OPEN_MAX
> > +               unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else
> >                 struct rlimit lim;
> >                 unsigned int max_fds;
> >
> > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
> >                         die_errno("cannot get RLIMIT_NOFILE");
> >
> >                 max_fds = lim.rlim_cur;
> > +#endif
> >
> >                 /* Save 3 for stdin/stdout/stderr, 22 for work */
> >                 if (25 < max_fds)
> > --
> > 1.7.12
> 
> Looks sane but it would be more readable to make this a small helper
function,
> so that we do not need to have #ifdef/#endif in the primary flow of the
code.

Hmm, in compat/? Worth the effort fort hat single occrence?

> By the way, I noticed that you seem to be sending patches out of git,
instead of
> "diff -ru", which is a good sign ;-).  

Not quite, I'm generating them with "git format-patch origin", on the
NonStop machine, but can't send email from there (a) behind a firewall and
b) no email client available), so I copy/paste the resulting file into
Outlook.

>But all of your patches are whitespace
> damaged and cannot be applied X-<.

May well be Outlooks fault? How to solve?

RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[Joachim Schmitz]

> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> Sent: Wednesday, August 22, 2012 7:53 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
> 
> > From: Junio C Hamano [mailto:gitster@pobox.com]
> > Sent: Wednesday, August 22, 2012 7:23 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org
> > Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> > getrlimit(RLIMIT_NOFILE,...)
> >
> > "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> >
> > > Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
> > > ---
> > >  sha1_file.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
> > >                 return error("packfile %s index unavailable",
> > > p->pack_name);
> > >
> > >         if (!pack_max_fds) {
> > > +#ifdef _SC_OPEN_MAX
> > > +               unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else
> > >                 struct rlimit lim;
> > >                 unsigned int max_fds;
> > >
> > > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
> > >                         die_errno("cannot get RLIMIT_NOFILE");
> > >
> > >                 max_fds = lim.rlim_cur;
> > > +#endif
> > >
> > >                 /* Save 3 for stdin/stdout/stderr, 22 for work */
> > >                 if (25 < max_fds)
> > > --
> > > 1.7.12
> >
> > Looks sane but it would be more readable to make this a small helper
> > function, so that we do not need to have #ifdef/#endif in the primary
flow of
> the code.
> 
> Hmm, in compat/? Worth the effort fort hat single occrence?
> 
> > By the way, I noticed that you seem to be sending patches out of git,
> > instead of "diff -ru", which is a good sign ;-).
> 
> Not quite, I'm generating them with "git format-patch origin", on the
NonStop
> machine, but can't send email from there (a) behind a firewall and b) no
email
> client available), so I copy/paste the resulting file into Outlook.
> 
> >But all of your patches are whitespace
> > damaged and cannot be applied X-<.
> 
> May well be Outlooks fault? How to solve?

Let's try this then:
---

Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
---
 sha1_file.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..76714ad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
 		return error("packfile %s index unavailable", p->pack_name);
 
 	if (!pack_max_fds) {
+#ifdef _SC_OPEN_MAX
+		unsigned int max_fds = sysconf(_SC_OPEN_MAX);
+#else
 		struct rlimit lim;
 		unsigned int max_fds;
 
@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
 			die_errno("cannot get RLIMIT_NOFILE");
 
 		max_fds = lim.rlim_cur;
+#endif
 
 		/* Save 3 for stdin/stdout/stderr, 22 for work */
 		if (25 < max_fds)
-- 
1.7.12

OK this way?

Bye, Jojo

Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> Looks sane but it would be more readable to make this a small helper
> function,
>> so that we do not need to have #ifdef/#endif in the primary flow of the
> code.
>
> Hmm, in compat/? Worth the effort fort hat single occrence?

Compat/ is to have some systems that lack an interface we deem to be
common on platforms emulate that interface, but in this case, we
just want something like:

	static int get_max_fd_limit(void)
        {
        #ifdef _SC_OPEN_MAX
        	...
	#else
		...
	#endif
		return max_fds;
	}

and that is not emulating anybody's standard interface.

Such a helper before the definition of the function that your patch
is touching, in the same file as a file-scope static, was what I
meant.  If other places in the code want to open tons of pipes or
file descriptors for some reason, we may want to make it part of the
supporting infrastructure, and drop "static " at the beginning and
add "extern int get_max_fd_limit(void);" to cache.h for others to
use, but as you said, there is currently only one place that uses
it, so it can stay static to the file.