{"thread":{"id":"31319","subject":"[PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","startedAt":"2012-08-22T16:00:50Z","lastAt":"2012-08-22T18:07:09Z","messageCount":5,"participants":["Joachim Schmitz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"197588","messageId":"002301cd807f$4e19ad80$ea4d0880$@schmitz-digital.de","threadId":"31319","inReplyTo":null,"subject":"[PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-22T16:00:50Z","receivedAt":"2012-08-22T16:00:50Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"\nSigned-off-by: Joachim Schmitz \n---\n sha1_file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex af5cfbd..76714ad 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)\n return error(\"packfile %s index unavailable\", p->pack_name);\n\n if (!pack_max_fds) {\n+#ifdef _SC_OPEN_MAX\n+ unsigned int max_fds = sysconf(_SC_OPEN_MAX);\n+#else\n struct rlimit lim;\n unsigned int max_fds;\n\n@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)\n die_errno(\"cannot get RLIMIT_NOFILE\");\n\n max_fds = lim.rlim_cur;\n+#endif\n\n /* Save 3 for stdin/stdout/stderr, 22 for work */\n if (25 < max_fds)\n--\n1.7.12\n"},{"id":"197602","messageId":"7v4nnualjf.fsf@alter.siamese.dyndns.org","threadId":"31319","inReplyTo":"002301cd807f$4e19ad80$ea4d0880$@schmitz-digital.de","subject":"Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T17:23:00Z","receivedAt":"2012-08-22T17:23:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" writes:\n\n> Signed-off-by: Joachim Schmitz \n> ---\n> sha1_file.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index af5cfbd..76714ad 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)\n> return error(\"packfile %s index unavailable\", p->pack_name);\n>\n> if (!pack_max_fds) {\n> +#ifdef _SC_OPEN_MAX\n> + unsigned int max_fds = sysconf(_SC_OPEN_MAX);\n> +#else\n> struct rlimit lim;\n> unsigned int max_fds;\n>\n> @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)\n> die_errno(\"cannot get RLIMIT_NOFILE\");\n>\n> max_fds = lim.rlim_cur;\n> +#endif\n>\n> /* Save 3 for stdin/stdout/stderr, 22 for work */\n> if (25 < max_fds)\n> --\n> 1.7.12\n\nLooks sane but it would be more readable to make this a small helper\nfunction, so that we do not need to have #ifdef/#endif in the\nprimary flow of the code.\n\nBy the way, I noticed that you seem to be sending patches out of\ngit, instead of \"diff -ru\", which is a good sign ;-). But all of\nyour patches are whitespace damaged and cannot be applied X-<.\n"},{"id":"197614","messageId":"003b01cd808f$0a5bf610$1f13e230$@schmitz-digital.de","threadId":"31319","inReplyTo":"7v4nnualjf.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-22T17:53:29Z","receivedAt":"2012-08-22T17:53:29Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Wednesday, August 22, 2012 7:23 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over\n> getrlimit(RLIMIT_NOFILE,...)\n> \n> \"Joachim Schmitz\" writes:\n> \n> > Signed-off-by: Joachim Schmitz \n> > ---\n> > sha1_file.c | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)\n> > return error(\"packfile %s index unavailable\",\n> > p->pack_name);\n> >\n> > if (!pack_max_fds) {\n> > +#ifdef _SC_OPEN_MAX\n> > + unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else\n> > struct rlimit lim;\n> > unsigned int max_fds;\n> >\n> > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)\n> > die_errno(\"cannot get RLIMIT_NOFILE\");\n> >\n> > max_fds = lim.rlim_cur;\n> > +#endif\n> >\n> > /* Save 3 for stdin/stdout/stderr, 22 for work */\n> > if (25 < max_fds)\n> > --\n> > 1.7.12\n> \n> Looks sane but it would be more readable to make this a small helper\nfunction,\n> so that we do not need to have #ifdef/#endif in the primary flow of the\ncode.\n\nHmm, in compat/? Worth the effort fort hat single occrence?\n\n> By the way, I noticed that you seem to be sending patches out of git,\ninstead of\n> \"diff -ru\", which is a good sign ;-). \n\nNot quite, I'm generating them with \"git format-patch origin\", on the\nNonStop machine, but can't send email from there (a) behind a firewall and\nb) no email client available), so I copy/paste the resulting file into\nOutlook.\n\n>But all of your patches are whitespace\n> damaged and cannot be applied X-<.\n\nMay well be Outlooks fault? How to solve?\n"},{"id":"197619","messageId":"003f01cd8090$d7e50990$87af1cb0$@schmitz-digital.de","threadId":"31319","inReplyTo":"7v4nnualjf.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-22T18:06:23Z","receivedAt":"2012-08-22T18:06:23Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]\n> Sent: Wednesday, August 22, 2012 7:53 PM\n> To: 'Junio C Hamano'\n> Cc: 'git@vger.kernel.org'\n> Subject: RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over\n> getrlimit(RLIMIT_NOFILE,...)\n> \n> > From: Junio C Hamano [mailto:gitster@pobox.com]\n> > Sent: Wednesday, August 22, 2012 7:23 PM\n> > To: Joachim Schmitz\n> > Cc: git@vger.kernel.org\n> > Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over\n> > getrlimit(RLIMIT_NOFILE,...)\n> >\n> > \"Joachim Schmitz\" writes:\n> >\n> > > Signed-off-by: Joachim Schmitz \n> > > ---\n> > > sha1_file.c | 4 ++++\n> > > 1 file changed, 4 insertions(+)\n> > >\n> > > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644\n> > > --- a/sha1_file.c\n> > > +++ b/sha1_file.c\n> > > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)\n> > > return error(\"packfile %s index unavailable\",\n> > > p->pack_name);\n> > >\n> > > if (!pack_max_fds) {\n> > > +#ifdef _SC_OPEN_MAX\n> > > + unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else\n> > > struct rlimit lim;\n> > > unsigned int max_fds;\n> > >\n> > > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)\n> > > die_errno(\"cannot get RLIMIT_NOFILE\");\n> > >\n> > > max_fds = lim.rlim_cur;\n> > > +#endif\n> > >\n> > > /* Save 3 for stdin/stdout/stderr, 22 for work */\n> > > if (25 < max_fds)\n> > > --\n> > > 1.7.12\n> >\n> > Looks sane but it would be more readable to make this a small helper\n> > function, so that we do not need to have #ifdef/#endif in the primary\nflow of\n> the code.\n> \n> Hmm, in compat/? Worth the effort fort hat single occrence?\n> \n> > By the way, I noticed that you seem to be sending patches out of git,\n> > instead of \"diff -ru\", which is a good sign ;-).\n> \n> Not quite, I'm generating them with \"git format-patch origin\", on the\nNonStop\n> machine, but can't send email from there (a) behind a firewall and b) no\nemail\n> client available), so I copy/paste the resulting file into Outlook.\n> \n> >But all of your patches are whitespace\n> > damaged and cannot be applied X-<.\n> \n> May well be Outlooks fault? How to solve?\n\nLet's try this then:\n---\n\nSigned-off-by: Joachim Schmitz \n---\n sha1_file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex af5cfbd..76714ad 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n \tif (!pack_max_fds) {\n+#ifdef _SC_OPEN_MAX\n+\t\tunsigned int max_fds = sysconf(_SC_OPEN_MAX);\n+#else\n \t\tstruct rlimit lim;\n \t\tunsigned int max_fds;\n \n@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n \n \t\tmax_fds = lim.rlim_cur;\n+#endif\n \n \t\t/* Save 3 for stdin/stdout/stderr, 22 for work */\n \t\tif (25 < max_fds)\n-- \n1.7.12\n\nOK this way?\n\nBye, Jojo\n"},{"id":"197620","messageId":"7vboi294xe.fsf@alter.siamese.dyndns.org","threadId":"31319","inReplyTo":"003b01cd808f$0a5bf610$1f13e230$@schmitz-digital.de","subject":"Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T18:07:09Z","receivedAt":"2012-08-22T18:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" writes:\n\n>> Looks sane but it would be more readable to make this a small helper\n> function,\n>> so that we do not need to have #ifdef/#endif in the primary flow of the\n> code.\n>\n> Hmm, in compat/? Worth the effort fort hat single occrence?\n\nCompat/ is to have some systems that lack an interface we deem to be\ncommon on platforms emulate that interface, but in this case, we\njust want something like:\n\n\tstatic int get_max_fd_limit(void)\n {\n #ifdef _SC_OPEN_MAX\n \t...\n\t#else\n\t\t...\n\t#endif\n\t\treturn max_fds;\n\t}\n\nand that is not emulating anybody's standard interface.\n\nSuch a helper before the definition of the function that your patch\nis touching, in the same file as a file-scope static, was what I\nmeant. If other places in the code want to open tons of pipes or\nfile descriptors for some reason, we may want to make it part of the\nsupporting infrastructure, and drop \"static \" at the beginning and\nadd \"extern int get_max_fd_limit(void);\" to cache.h for others to\nuse, but as you said, there is currently only one place that uses\nit, so it can stay static to the file.\n"}]}