From: Jeff Hostetler Date: Fri, 02 Dec 2022 18:44:30 GMT Subject: Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0 Message-ID: In-Reply-To: <221202.86k039fz7x.gmgdl@evledraar.gmail.com> On 12/2/22 1:10 PM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Dec 02 2022, Rose via GitGitGadget wrote: > >> From: Seija >> >> The documentation for pthread_create and pthread_sigmask state that: >> >> "On success, pthread_create() returns 0; >> on error, it returns an error number" >> >> As such, we ought to check for an error >> by seeing if the output is not 0. >> >> Checking for "less than" is a mistake >> as the error code numbers can be greater than 0. >> >> Signed-off-by: Seija >> --- >> maintenance: compare output of pthread functions for inequality with 0 >> >> The documentation for pthread_create and pthread_sigmask state that "On >> success, pthread_create() returns 0; on error, it returns an error >> number, and the contents of *thread are undefined." >> >> As such, we ought to check for an error by seeing if the output is not >> 0, rather than being less than 0, since nothing stops these functions >> from returning a positive number. >> >> Signed-off by: Seija doremylover123@gmail.com >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1389%2FAtariDreams%2Faddress-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1389/AtariDreams/address-v1 >> Pull-Request: https://github.com/git/git/pull/1389 >> >> builtin/fsmonitor--daemon.c | 4 ++-- >> run-command.c | 2 +- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c >> index 6f30a4f93a7..52a08bb3b57 100644 >> --- a/builtin/fsmonitor--daemon.c >> +++ b/builtin/fsmonitor--daemon.c >> @@ -1209,7 +1209,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) >> * events. >> */ >> if (pthread_create(&state->listener_thread, NULL, >> - fsm_listen__thread_proc, state) < 0) { >> + fsm_listen__thread_proc, state)) { >> ipc_server_stop_async(state->ipc_server_data); >> err = error(_("could not start fsmonitor listener thread")); >> goto cleanup; >> @@ -1220,7 +1220,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) >> * Start the health thread to watch over our process. >> */ >> if (pthread_create(&state->health_thread, NULL, >> - fsm_health__thread_proc, state) < 0) { >> + fsm_health__thread_proc, state)) { >> ipc_server_stop_async(state->ipc_server_data); >> err = error(_("could not start fsmonitor health thread")); >> goto cleanup; >> diff --git a/run-command.c b/run-command.c >> index 48b9ba6d6f0..756f1839aab 100644 >> --- a/run-command.c >> +++ b/run-command.c >> @@ -1019,7 +1019,7 @@ static void *run_thread(void *data) >> sigset_t mask; >> sigemptyset(&mask); >> sigaddset(&mask, SIGPIPE); >> - if (pthread_sigmask(SIG_BLOCK, &mask, NULL) < 0) { >> + if (pthread_sigmask(SIG_BLOCK, &mask, NULL)) { >> ret = error("unable to block SIGPIPE in async thread"); >> return (void *)ret; >> } >> >> base-commit: 805265fcf7a737664a8321aaf4a0587b78435184 > > This looks good to me, and skimming through the rest of the > pthread_create() it seems the rest of the code in-tree is correct. > > But (and especially if you're interested) we really should follow-up > here and fix the "error()" etc. part of this. After this we have cases > in-tree where we on failure: But to be clear, the pthread_ changes are good by themselves and can be considered a single task that could be advanced without any extra stuff. All of the following, if of interest to you or anyone else, should be done in one or more separate/later and independent series. > > * Call die_errno() (good) > * Call die(), error() etc., but with a manual strerror() argument, > these should just use the *_errno() helper. > * Don't report on the errno at all, e.g. in this case shown here. > > It seems to me that all of these should be using die_errno(), > error_errno() etc. > > Or maybe it's the other way around, and we should not rely on the global > "errno", but always capture the return value, and give that to > strerror() (or set "errno = ret", and call {die,error,warning}_errno()). > > In any case, some low-hanging #leftoverbits there... >