{"thread":{"id":"58890","subject":"[PATCH] maintenance: compare output of pthread functions for inequality with 0","startedAt":"2022-12-02T17:03:13Z","lastAt":"2022-12-03T00:26:40Z","messageCount":7,"participants":["Rose via GitGitGadget","Jeff Hostetler","Ævar Arnfjörð Bjarmason","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468451","messageId":"pull.1389.git.git.1670000578395.gitgitgadget@gmail.com","threadId":"58890","inReplyTo":null,"subject":"[PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-02T17:02:58Z","receivedAt":"2022-12-02T17:03:13Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija <doremylover123@gmail.com>\n\nThe documentation for pthread_create and pthread_sigmask state that:\n\n\"On success, pthread_create() returns 0;\non error, it returns an error number\"\n\nAs such, we ought to check for an error\nby seeing if the output is not 0.\n\nChecking for \"less than\" is a mistake\nas the error code numbers can be greater than 0.\n\nSigned-off-by: Seija <doremylover123@gmail.com>\n---\n    maintenance: compare output of pthread functions for inequality with 0\n    \n    The documentation for pthread_create and pthread_sigmask state that \"On\n    success, pthread_create() returns 0; on error, it returns an error\n    number, and the contents of *thread are undefined.\"\n    \n    As such, we ought to check for an error by seeing if the output is not\n    0, rather than being less than 0, since nothing stops these functions\n    from returning a positive number.\n    \n    Signed-off by: Seija doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1389%2FAtariDreams%2Faddress-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1389/AtariDreams/address-v1\nPull-Request: https://github.com/git/git/pull/1389\n\n builtin/fsmonitor--daemon.c | 4 ++--\n run-command.c               | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 6f30a4f93a7..52a08bb3b57 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1209,7 +1209,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n \t * events.\n \t */\n \tif (pthread_create(&state->listener_thread, NULL,\n-\t\t\t   fsm_listen__thread_proc, state) < 0) {\n+\t\t\t   fsm_listen__thread_proc, state)) {\n \t\tipc_server_stop_async(state->ipc_server_data);\n \t\terr = error(_(\"could not start fsmonitor listener thread\"));\n \t\tgoto cleanup;\n@@ -1220,7 +1220,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n \t * Start the health thread to watch over our process.\n \t */\n \tif (pthread_create(&state->health_thread, NULL,\n-\t\t\t   fsm_health__thread_proc, state) < 0) {\n+\t\t\t   fsm_health__thread_proc, state)) {\n \t\tipc_server_stop_async(state->ipc_server_data);\n \t\terr = error(_(\"could not start fsmonitor health thread\"));\n \t\tgoto cleanup;\ndiff --git a/run-command.c b/run-command.c\nindex 48b9ba6d6f0..756f1839aab 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1019,7 +1019,7 @@ static void *run_thread(void *data)\n \t\tsigset_t mask;\n \t\tsigemptyset(&mask);\n \t\tsigaddset(&mask, SIGPIPE);\n-\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL) < 0) {\n+\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL)) {\n \t\t\tret = error(\"unable to block SIGPIPE in async thread\");\n \t\t\treturn (void *)ret;\n \t\t}\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\n-- \ngitgitgadget\n"},{"id":"468453","messageId":"34ed06a2-9c67-62d5-7985-4b7379a0cf6c@jeffhostetler.com","threadId":"58890","inReplyTo":"pull.1389.git.git.1670000578395.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-12-02T18:05:32Z","receivedAt":"2022-12-02T18:05:40Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 12/2/22 12:02 PM, Rose via GitGitGadget wrote:\n> From: Seija <doremylover123@gmail.com>\n> \n> The documentation for pthread_create and pthread_sigmask state that:\n> \n> \"On success, pthread_create() returns 0;\n> on error, it returns an error number\"\n> \n> As such, we ought to check for an error\n> by seeing if the output is not 0.\n> \n> Checking for \"less than\" is a mistake\n> as the error code numbers can be greater than 0.\n> \n> Signed-off-by: Seija <doremylover123@gmail.com>\n> ---\n>      maintenance: compare output of pthread functions for inequality with 0\n>      \n>      The documentation for pthread_create and pthread_sigmask state that \"On\n>      success, pthread_create() returns 0; on error, it returns an error\n>      number, and the contents of *thread are undefined.\"\n>      \n>      As such, we ought to check for an error by seeing if the output is not\n>      0, rather than being less than 0, since nothing stops these functions\n>      from returning a positive number.\n>      \n>      Signed-off by: Seija doremylover123@gmail.com\n\nGood catch!\n\nLGTM\n\nThanks,\nJeff\n\n"},{"id":"468455","messageId":"221202.86k039fz7x.gmgdl@evledraar.gmail.com","threadId":"58890","inReplyTo":"pull.1389.git.git.1670000578395.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-02T18:10:57Z","receivedAt":"2022-12-02T18:16:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 02 2022, Rose via GitGitGadget wrote:\n\n> From: Seija <doremylover123@gmail.com>\n>\n> The documentation for pthread_create and pthread_sigmask state that:\n>\n> \"On success, pthread_create() returns 0;\n> on error, it returns an error number\"\n>\n> As such, we ought to check for an error\n> by seeing if the output is not 0.\n>\n> Checking for \"less than\" is a mistake\n> as the error code numbers can be greater than 0.\n>\n> Signed-off-by: Seija <doremylover123@gmail.com>\n> ---\n>     maintenance: compare output of pthread functions for inequality with 0\n>     \n>     The documentation for pthread_create and pthread_sigmask state that \"On\n>     success, pthread_create() returns 0; on error, it returns an error\n>     number, and the contents of *thread are undefined.\"\n>     \n>     As such, we ought to check for an error by seeing if the output is not\n>     0, rather than being less than 0, since nothing stops these functions\n>     from returning a positive number.\n>     \n>     Signed-off by: Seija doremylover123@gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1389%2FAtariDreams%2Faddress-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1389/AtariDreams/address-v1\n> Pull-Request: https://github.com/git/git/pull/1389\n>\n>  builtin/fsmonitor--daemon.c | 4 ++--\n>  run-command.c               | 2 +-\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\n> index 6f30a4f93a7..52a08bb3b57 100644\n> --- a/builtin/fsmonitor--daemon.c\n> +++ b/builtin/fsmonitor--daemon.c\n> @@ -1209,7 +1209,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n>  \t * events.\n>  \t */\n>  \tif (pthread_create(&state->listener_thread, NULL,\n> -\t\t\t   fsm_listen__thread_proc, state) < 0) {\n> +\t\t\t   fsm_listen__thread_proc, state)) {\n>  \t\tipc_server_stop_async(state->ipc_server_data);\n>  \t\terr = error(_(\"could not start fsmonitor listener thread\"));\n>  \t\tgoto cleanup;\n> @@ -1220,7 +1220,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n>  \t * Start the health thread to watch over our process.\n>  \t */\n>  \tif (pthread_create(&state->health_thread, NULL,\n> -\t\t\t   fsm_health__thread_proc, state) < 0) {\n> +\t\t\t   fsm_health__thread_proc, state)) {\n>  \t\tipc_server_stop_async(state->ipc_server_data);\n>  \t\terr = error(_(\"could not start fsmonitor health thread\"));\n>  \t\tgoto cleanup;\n> diff --git a/run-command.c b/run-command.c\n> index 48b9ba6d6f0..756f1839aab 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1019,7 +1019,7 @@ static void *run_thread(void *data)\n>  \t\tsigset_t mask;\n>  \t\tsigemptyset(&mask);\n>  \t\tsigaddset(&mask, SIGPIPE);\n> -\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL) < 0) {\n> +\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL)) {\n>  \t\t\tret = error(\"unable to block SIGPIPE in async thread\");\n>  \t\t\treturn (void *)ret;\n>  \t\t}\n>\n> base-commit: 805265fcf7a737664a8321aaf4a0587b78435184\n\nThis looks good to me, and skimming through the rest of the\npthread_create() it seems the rest of the code in-tree is correct.\n\nBut (and especially if you're interested) we really should follow-up\nhere and fix the \"error()\" etc. part of this. After this we have cases\nin-tree where we on failure:\n\n * Call die_errno() (good)\n * Call die(), error() etc., but with a manual strerror() argument,\n   these should just use the *_errno() helper.\n * Don't report on the errno at all, e.g. in this case shown here.\n\nIt seems to me that all of these should be using die_errno(),\nerror_errno() etc.\n\nOr maybe it's the other way around, and we should not rely on the global\n\"errno\", but always capture the return value, and give that to\nstrerror() (or set \"errno = ret\", and call {die,error,warning}_errno()).\n\nIn any case, some low-hanging #leftoverbits there...\n\n"},{"id":"468459","messageId":"d4a5bd6e-77bd-eb63-5c7f-8f3d46cc0418@jeffhostetler.com","threadId":"58890","inReplyTo":"221202.86k039fz7x.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-12-02T18:44:30Z","receivedAt":"2022-12-02T18:45:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 12/2/22 1:10 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Dec 02 2022, Rose via GitGitGadget wrote:\n> \n>> From: Seija <doremylover123@gmail.com>\n>>\n>> The documentation for pthread_create and pthread_sigmask state that:\n>>\n>> \"On success, pthread_create() returns 0;\n>> on error, it returns an error number\"\n>>\n>> As such, we ought to check for an error\n>> by seeing if the output is not 0.\n>>\n>> Checking for \"less than\" is a mistake\n>> as the error code numbers can be greater than 0.\n>>\n>> Signed-off-by: Seija <doremylover123@gmail.com>\n>> ---\n>>      maintenance: compare output of pthread functions for inequality with 0\n>>      \n>>      The documentation for pthread_create and pthread_sigmask state that \"On\n>>      success, pthread_create() returns 0; on error, it returns an error\n>>      number, and the contents of *thread are undefined.\"\n>>      \n>>      As such, we ought to check for an error by seeing if the output is not\n>>      0, rather than being less than 0, since nothing stops these functions\n>>      from returning a positive number.\n>>      \n>>      Signed-off by: Seija doremylover123@gmail.com\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1389%2FAtariDreams%2Faddress-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1389/AtariDreams/address-v1\n>> Pull-Request: https://github.com/git/git/pull/1389\n>>\n>>   builtin/fsmonitor--daemon.c | 4 ++--\n>>   run-command.c               | 2 +-\n>>   2 files changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\n>> index 6f30a4f93a7..52a08bb3b57 100644\n>> --- a/builtin/fsmonitor--daemon.c\n>> +++ b/builtin/fsmonitor--daemon.c\n>> @@ -1209,7 +1209,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n>>   \t * events.\n>>   \t */\n>>   \tif (pthread_create(&state->listener_thread, NULL,\n>> -\t\t\t   fsm_listen__thread_proc, state) < 0) {\n>> +\t\t\t   fsm_listen__thread_proc, state)) {\n>>   \t\tipc_server_stop_async(state->ipc_server_data);\n>>   \t\terr = error(_(\"could not start fsmonitor listener thread\"));\n>>   \t\tgoto cleanup;\n>> @@ -1220,7 +1220,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n>>   \t * Start the health thread to watch over our process.\n>>   \t */\n>>   \tif (pthread_create(&state->health_thread, NULL,\n>> -\t\t\t   fsm_health__thread_proc, state) < 0) {\n>> +\t\t\t   fsm_health__thread_proc, state)) {\n>>   \t\tipc_server_stop_async(state->ipc_server_data);\n>>   \t\terr = error(_(\"could not start fsmonitor health thread\"));\n>>   \t\tgoto cleanup;\n>> diff --git a/run-command.c b/run-command.c\n>> index 48b9ba6d6f0..756f1839aab 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -1019,7 +1019,7 @@ static void *run_thread(void *data)\n>>   \t\tsigset_t mask;\n>>   \t\tsigemptyset(&mask);\n>>   \t\tsigaddset(&mask, SIGPIPE);\n>> -\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL) < 0) {\n>> +\t\tif (pthread_sigmask(SIG_BLOCK, &mask, NULL)) {\n>>   \t\t\tret = error(\"unable to block SIGPIPE in async thread\");\n>>   \t\t\treturn (void *)ret;\n>>   \t\t}\n>>\n>> base-commit: 805265fcf7a737664a8321aaf4a0587b78435184\n> \n> This looks good to me, and skimming through the rest of the\n> pthread_create() it seems the rest of the code in-tree is correct.\n> \n> But (and especially if you're interested) we really should follow-up\n> here and fix the \"error()\" etc. part of this. After this we have cases\n> in-tree where we on failure:\n\nBut to be clear, the pthread_ changes are good by themselves and can\nbe considered a single task that could be advanced without any\nextra stuff.\n\nAll of the following, if of interest to you or anyone else, should\nbe done in one or more separate/later and independent series.\n\n> \n>   * Call die_errno() (good)\n>   * Call die(), error() etc., but with a manual strerror() argument,\n>     these should just use the *_errno() helper.\n>   * Don't report on the errno at all, e.g. in this case shown here.\n> \n> It seems to me that all of these should be using die_errno(),\n> error_errno() etc.\n> \n> Or maybe it's the other way around, and we should not rely on the global\n> \"errno\", but always capture the return value, and give that to\n> strerror() (or set \"errno = ret\", and call {die,error,warning}_errno()).\n> \n> In any case, some low-hanging #leftoverbits there...\n> \n"},{"id":"468465","messageId":"Y4pl5BzQnw0Fm+5S@tapette.crustytoothpaste.net","threadId":"58890","inReplyTo":"221202.86k039fz7x.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-12-02T20:55:21Z","receivedAt":"2022-12-02T20:55:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-12-02 at 18:10:57, Ævar Arnfjörð Bjarmason wrote:\n> \n> But (and especially if you're interested) we really should follow-up\n> here and fix the \"error()\" etc. part of this. After this we have cases\n> in-tree where we on failure:\n> \n>  * Call die_errno() (good)\n>  * Call die(), error() etc., but with a manual strerror() argument,\n>    these should just use the *_errno() helper.\n>  * Don't report on the errno at all, e.g. in this case shown here.\n> \n> It seems to me that all of these should be using die_errno(),\n> error_errno() etc.\n\nActually, I don't think that's correct.\n\n> Or maybe it's the other way around, and we should not rely on the global\n> \"errno\", but always capture the return value, and give that to\n> strerror() (or set \"errno = ret\", and call {die,error,warning}_errno()).\n\nYeah, I think we need to do this.  That's because unlike most other\nfunctions, the pthread functions _don't_ set errno, and instead return\nthe error value.  That's why on a typical Unix system, we would have\nnever failed before this patch: because errno values are always\npositive.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"468470","messageId":"221202.86y1rpe7ma.gmgdl@evledraar.gmail.com","threadId":"58890","inReplyTo":"Y4pl5BzQnw0Fm+5S@tapette.crustytoothpaste.net","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-02T22:46:25Z","receivedAt":"2022-12-02T22:57:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 02 2022, brian m. carlson wrote:\n\n> [[PGP Signed Part:Undecided]]\n> On 2022-12-02 at 18:10:57, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> But (and especially if you're interested) we really should follow-up\n>> here and fix the \"error()\" etc. part of this. After this we have cases\n>> in-tree where we on failure:\n>> \n>>  * Call die_errno() (good)\n>>  * Call die(), error() etc., but with a manual strerror() argument,\n>>    these should just use the *_errno() helper.\n>>  * Don't report on the errno at all, e.g. in this case shown here.\n>> \n>> It seems to me that all of these should be using die_errno(),\n>> error_errno() etc.\n>\n> Actually, I don't think that's correct.\n>\n>> Or maybe it's the other way around, and we should not rely on the global\n>> \"errno\", but always capture the return value, and give that to\n>> strerror() (or set \"errno = ret\", and call {die,error,warning}_errno()).\n>\n> Yeah, I think we need to do this.  That's because unlike most other\n> functions, the pthread functions _don't_ set errno, and instead return\n> the error value.  That's why on a typical Unix system, we would have\n> never failed before this patch: because errno values are always\n> positive.\n\nI was skimming the POSIX docs earlier, which seem to indicate that\nyou're not promised anyhting about \"errno\" being set, just the return\nvalue.\n\nBut at the same time I was reading glibc's pthread implementation, where\na lot of the time (but not all the time!) you'll also get errno, just as\nan artifact of the library carrying forward an error from an internal\nAPI which failed while setting errno (e.g. malloc()).\n\nIn any case, the best thing to do for our codebase is probably:\n\n\tif ((errno = pthread_create(...)))\n        \tdie_errno(...);\n\nSince that gives our usag.[ch] library the chance to do something more\nclever than doing the same strerror() formatting hardcoded at every\ncallsite.\n"},{"id":"468475","messageId":"Y4qXu5+0jP7+r4yl@tapette.crustytoothpaste.net","threadId":"58890","inReplyTo":"221202.86y1rpe7ma.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-12-03T00:26:35Z","receivedAt":"2022-12-03T00:26:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-12-02 at 22:46:25, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Dec 02 2022, brian m. carlson wrote:\n> \n> > Yeah, I think we need to do this.  That's because unlike most other\n> > functions, the pthread functions _don't_ set errno, and instead return\n> > the error value.  That's why on a typical Unix system, we would have\n> > never failed before this patch: because errno values are always\n> > positive.\n> \n> I was skimming the POSIX docs earlier, which seem to indicate that\n> you're not promised anyhting about \"errno\" being set, just the return\n> value.\n\nTechnically true.  But POSIX says this:\n\n  The value of errno shall be defined only after a call to a function\n  for which it is explicitly stated to be set and until it is changed by\n  the next function call or if the application assigns it a value. The\n  value of errno should only be examined when it is indicated to be\n  valid by a function's return value. Applications shall obtain the\n  definition of errno by the inclusion of <errno.h>. No function in this\n  volume of POSIX.1-2017 shall set errno to 0. The setting of errno\n  after a successful call to a function is unspecified unless the\n  description of that function specifies that errno shall not be\n  modified.\n\nSo literally any function can set it and it is unspecified after a\npthread function call (which doesn't explicitly say it's set).  For\nexample, sync(2), which has no errors defined, could well set errno,\nalthough its value would be unspecified (but it would not be zero unless\nit already was before the call).\n\nHowever, we don't care there, because POSIX doesn't allow returning\nmultiple errors (that's not very C), and it won't contain anything\nuseful.  I should have said instead that they return errors instead of\nsetting errno to indicate them.\n\n> But at the same time I was reading glibc's pthread implementation, where\n> a lot of the time (but not all the time!) you'll also get errno, just as\n> an artifact of the library carrying forward an error from an internal\n> API which failed while setting errno (e.g. malloc()).\n\nAnd this is probably part of why POSIX has this policy.  I'm sure this\nsame thing is true for pretty much every libc.\n\n> In any case, the best thing to do for our codebase is probably:\n> \n> \tif ((errno = pthread_create(...)))\n>         \tdie_errno(...);\n\nI agree that's probably fine to do here.  If folks feel setting errno\nthis way is too icky, we can also just call die with strerror.  I don't\nhave a strong feeling one way or the other.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"}]}