threads / patch / 31319

patchPrefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)

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

## tl;dr

5 messages between Aug 22, 2012 and Aug 22, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Joachim Schmitz· Aug 22, 2012, 16:00 UTC · lore
Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
---
 sha1_file.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to sha1_file.c +4 −1
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
Junio C Hamano· Aug 22, 2012, 17:23 UTC · re: Joachim Schmitz · lore

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

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:
Show 29 quoted lines
> 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-<.

Joachim Schmitz· Aug 22, 2012, 17:53 UTC · re: Junio C Hamano · lore

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

Show 39 quoted lines
> 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?
Junio C Hamano· Aug 22, 2012, 18:07 UTC · re: Joachim Schmitz · lore

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

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:
Show 6 quoted lines
>> 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.

Joachim Schmitz· Aug 22, 2012, 18:06 UTC · re: Junio C Hamano · lore

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

Show 47 quoted lines
> 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
Show 8 quoted lines
> 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
Show 6 quoted lines
> 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(+)
Show changes to sha1_file.c +4 −0
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

← back to recent threads