{"thread":{"id":"31339","subject":"[PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","startedAt":"2012-08-24T09:52:22Z","lastAt":"2012-08-24T18:36:21Z","messageCount":3,"participants":["Joachim Schmitz","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"197760","messageId":"002f01cd81de$28f43bf0$7adcb3d0$@schmitz-digital.de","threadId":"31339","inReplyTo":null,"subject":"[PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-24T09:52:22Z","receivedAt":"2012-08-24T09:52:22Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"\nSigned-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n---\nAs discussed now as a small helper function rather than #ifdef/#endif in the primary flow of the code.\nAnd hopefully without having screwed up whitespace and line breaks\n\n sha1_file.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex af5cfbd..427f9e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -731,6 +731,20 @@ void free_pack_by_name(const char *pack_name)\n \t}\n }\n \n+static unsigned int get_max_fd_limit(void)\n+{\n+#ifdef _SC_OPEN_MAX\n+\treturn sysconf(_SC_OPEN_MAX);\n+#else\n+\tstruct rlimit lim;\n+\n+\tif (getrlimit(RLIMIT_NOFILE, &lim))\n+\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\n+\treturn lim.rlim_cur;\n+#endif\n+}\n+\n /*\n  * Do not call this directly as this leaks p->pack_fd on error return;\n  * call open_packed_git() instead.\n@@ -747,13 +761,7 @@ 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-\t\tstruct rlimit lim;\n-\t\tunsigned int max_fds;\n-\n-\t\tif (getrlimit(RLIMIT_NOFILE, &lim))\n-\t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n-\n-\t\tmax_fds = lim.rlim_cur;\n+\t\tunsigned int max_fds = get_max_fd_limit();\n \n \t\t/* Save 3 for stdin/stdout/stderr, 22 for work */\n \t\tif (25 < max_fds)\n-- \n1.7.12\n"},{"id":"197782","messageId":"7v393c1br5.fsf@alter.siamese.dyndns.org","threadId":"31339","inReplyTo":"002f01cd81de$28f43bf0$7adcb3d0$@schmitz-digital.de","subject":"Re: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-24T16:43:42Z","receivedAt":"2012-08-24T16:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n> Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n> ---\n> As discussed now as a small helper function rather than #ifdef/#endif in the primary flow of the code.\n> And hopefully without having screwed up whitespace and line breaks\n\nThe formatting looks fine.\n\nPerhaps I am being overly paranoid, but I would prefer not to change\nthings for people who have been using getrlimit().  For them, if\nthey also have sysconf(_SC_OPEN_MAX), your code _ought to_ work, but\nif it does not work for whatever reason (perhaps some platforms\nclaim to have both, but getrlimit() works and sysconf(_SC_OPEN_MAX)\nis broken), it will given them an unnecessary regression.\n\nSo how about doing it this way instead?\n\n-- >8 --\nSubject: sha1_file.c: introduce get_max_fd_limit() helper\n\nNot all platforms have getrlimit(), but there are other ways to see\nthe maximum number of files that a process can have open.  If\ngetrlimit() is unavailable, fall back to sysconf(_SC_OPEN_MAX) if\navailable, and use OPEN_MAX from <limits.h>.\n\nSigned-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sha1_file.c | 26 +++++++++++++++++++-------\n 1 file changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git c/sha1_file.c w/sha1_file.c\nindex af5cfbd..9152974 100644\n--- c/sha1_file.c\n+++ w/sha1_file.c\n@@ -731,6 +731,24 @@ void free_pack_by_name(const char *pack_name)\n \t}\n }\n \n+static unsigned int get_max_fd_limit(void)\n+{\n+#ifdef RLIMIT_NOFILE\n+\tstruct rlimit lim;\n+\n+\tif (getrlimit(RLIMIT_NOFILE, &lim))\n+\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\n+\treturn lim.rlim_cur;\n+#elif defined(_SC_OPEN_MAX)\n+\treturn sysconf(_SC_OPEN_MAX);\n+#elif defined(OPEN_MAX)\n+\treturn OPEN_MAX;\n+#else\n+\treturn 1; /* see the caller ;-) */\n+#endif\n+}\n+\n /*\n  * Do not call this directly as this leaks p->pack_fd on error return;\n  * call open_packed_git() instead.\n@@ -747,13 +765,7 @@ 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-\t\tstruct rlimit lim;\n-\t\tunsigned int max_fds;\n-\n-\t\tif (getrlimit(RLIMIT_NOFILE, &lim))\n-\t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n-\n-\t\tmax_fds = lim.rlim_cur;\n+\t\tunsigned int max_fds = get_max_fd_limit();\n \n \t\t/* Save 3 for stdin/stdout/stderr, 22 for work */\n \t\tif (25 < max_fds)\n"},{"id":"197788","messageId":"004801cd8227$5bce8910$136b9b30$@schmitz-digital.de","threadId":"31339","inReplyTo":"7v393c1br5.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-24T18:36:21Z","receivedAt":"2012-08-24T18:36:21Z","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: Friday, August 24, 2012 6:44 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> > Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n> > ---\n> > As discussed now as a small helper function rather than #ifdef/#endif in the primary flow of the code.\n> > And hopefully without having screwed up whitespace and line breaks\n> \n> The formatting looks fine.\n> \n> Perhaps I am being overly paranoid, but I would prefer not to change\n> things for people who have been using getrlimit().  For them, if\n> they also have sysconf(_SC_OPEN_MAX), your code _ought to_ work, but\n> if it does not work for whatever reason (perhaps some platforms\n> claim to have both, but getrlimit() works and sysconf(_SC_OPEN_MAX)\n> is broken), it will given them an unnecessary regression.\n\nSounds reasonable, so reasonable that I wonder why I didn't have that idea ;-)\n\n> So how about doing it this way instead?\n> \n> -- >8 --\n> Subject: sha1_file.c: introduce get_max_fd_limit() helper\n> \n> Not all platforms have getrlimit(), but there are other ways to see\n> the maximum number of files that a process can have open.  If\n> getrlimit() is unavailable, fall back to sysconf(_SC_OPEN_MAX) if\n> available, and use OPEN_MAX from <limits.h>.\n> \n> Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  sha1_file.c | 26 +++++++++++++++++++-------\n>  1 file changed, 19 insertions(+), 7 deletions(-)\n> \n> diff --git c/sha1_file.c w/sha1_file.c\n> index af5cfbd..9152974 100644\n> --- c/sha1_file.c\n> +++ w/sha1_file.c\n> @@ -731,6 +731,24 @@ void free_pack_by_name(const char *pack_name)\n>  \t}\n>  }\n> \n> +static unsigned int get_max_fd_limit(void)\n> +{\n> +#ifdef RLIMIT_NOFILE\n> +\tstruct rlimit lim;\n> +\n> +\tif (getrlimit(RLIMIT_NOFILE, &lim))\n> +\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n> +\n> +\treturn lim.rlim_cur;\n> +#elif defined(_SC_OPEN_MAX)\n> +\treturn sysconf(_SC_OPEN_MAX);\n> +#elif defined(OPEN_MAX)\n> +\treturn OPEN_MAX;\n> +#else\n> +\treturn 1; /* see the caller ;-) */\n> +#endif\n> +}\n> +\n>  /*\n>   * Do not call this directly as this leaks p->pack_fd on error return;\n>   * call open_packed_git() instead.\n> @@ -747,13 +765,7 @@ 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> -\t\tstruct rlimit lim;\n> -\t\tunsigned int max_fds;\n> -\n> -\t\tif (getrlimit(RLIMIT_NOFILE, &lim))\n> -\t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n> -\n> -\t\tmax_fds = lim.rlim_cur;\n> +\t\tunsigned int max_fds = get_max_fd_limit();\n> \n>  \t\t/* Save 3 for stdin/stdout/stderr, 22 for work */\n>  \t\tif (25 < max_fds)\n\nLooks good to me. \nStupid newbie question: how would I revert my commit to my clone, to then add (and test) this one?\n\nBye, Jojo\n"}]}