| Message ID | pull.1389.git.git.1670000578395.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 786e67611d46cd761e1aaa73f85b62f3dda1960a |
| Headers | show |
| Series | maintenance: compare output of pthread functions for inequality with 0 | expand |
On 12/2/22 12:02 PM, Rose via GitGitGadget wrote: > From: Seija <doremylover123@gmail.com> > > 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 <doremylover123@gmail.com> > --- > 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 Good catch! LGTM Thanks, Jeff
On Fri, Dec 02 2022, Rose via GitGitGadget wrote: > From: Seija <doremylover123@gmail.com> > > 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 <doremylover123@gmail.com> > --- > 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: * 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...
On 12/2/22 1:10 PM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Dec 02 2022, Rose via GitGitGadget wrote: > >> From: Seija <doremylover123@gmail.com> >> >> 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 <doremylover123@gmail.com> >> --- >> 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... >
On 2022-12-02 at 18:10:57, Ævar Arnfjörð Bjarmason wrote: > > 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: > > * 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. Actually, I don't think that's correct. > 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()). Yeah, I think we need to do this. That's because unlike most other functions, the pthread functions _don't_ set errno, and instead return the error value. That's why on a typical Unix system, we would have never failed before this patch: because errno values are always positive.
On Fri, Dec 02 2022, brian m. carlson wrote: > [[PGP Signed Part:Undecided]] > On 2022-12-02 at 18:10:57, Ævar Arnfjörð Bjarmason wrote: >> >> 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: >> >> * 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. > > Actually, I don't think that's correct. > >> 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()). > > Yeah, I think we need to do this. That's because unlike most other > functions, the pthread functions _don't_ set errno, and instead return > the error value. That's why on a typical Unix system, we would have > never failed before this patch: because errno values are always > positive. I was skimming the POSIX docs earlier, which seem to indicate that you're not promised anyhting about "errno" being set, just the return value. But at the same time I was reading glibc's pthread implementation, where a lot of the time (but not all the time!) you'll also get errno, just as an artifact of the library carrying forward an error from an internal API which failed while setting errno (e.g. malloc()). In any case, the best thing to do for our codebase is probably: if ((errno = pthread_create(...))) die_errno(...); Since that gives our usag.[ch] library the chance to do something more clever than doing the same strerror() formatting hardcoded at every callsite.
On 2022-12-02 at 22:46:25, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Dec 02 2022, brian m. carlson wrote: > > > Yeah, I think we need to do this. That's because unlike most other > > functions, the pthread functions _don't_ set errno, and instead return > > the error value. That's why on a typical Unix system, we would have > > never failed before this patch: because errno values are always > > positive. > > I was skimming the POSIX docs earlier, which seem to indicate that > you're not promised anyhting about "errno" being set, just the return > value. Technically true. But POSIX says this: The value of errno shall be defined only after a call to a function for which it is explicitly stated to be set and until it is changed by the next function call or if the application assigns it a value. The value of errno should only be examined when it is indicated to be valid by a function's return value. Applications shall obtain the definition of errno by the inclusion of <errno.h>. No function in this volume of POSIX.1-2017 shall set errno to 0. The setting of errno after a successful call to a function is unspecified unless the description of that function specifies that errno shall not be modified. So literally any function can set it and it is unspecified after a pthread function call (which doesn't explicitly say it's set). For example, sync(2), which has no errors defined, could well set errno, although its value would be unspecified (but it would not be zero unless it already was before the call). However, we don't care there, because POSIX doesn't allow returning multiple errors (that's not very C), and it won't contain anything useful. I should have said instead that they return errors instead of setting errno to indicate them. > But at the same time I was reading glibc's pthread implementation, where > a lot of the time (but not all the time!) you'll also get errno, just as > an artifact of the library carrying forward an error from an internal > API which failed while setting errno (e.g. malloc()). And this is probably part of why POSIX has this policy. I'm sure this same thing is true for pretty much every libc. > In any case, the best thing to do for our codebase is probably: > > if ((errno = pthread_create(...))) > die_errno(...); I agree that's probably fine to do here. If folks feel setting errno this way is too icky, we can also just call die with strerror. I don't have a strong feeling one way or the other.
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; }