maintenance: compare output of pthread functions for inequality with 0
PATCH 1/1 maintenance: compare output of pthread functions for inequality with 0
From: Seija <[email protected]>
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.
---
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 [email protected]
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(-)
builtin/fsmonitor--daemon.c +2 -2
@@ -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;
run-command.c +1 -1
@@ -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
Signature · 1 line
gitgitgadget
Quoted 21 lines · Rose via GitGitGadget
From: Seija <[email protected]>
The documentation for
pthread_createandpthread_sigmaskstate 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 <[email protected]>---
maintenance: compare output of pthread functions for inequality with 0The documentation for
pthread_createandpthread_sigmaskstate that "On success,pthread_create()returns 0; on error, it returns an error number, and the contents of *thread are undefined."
Quoted 5 lines
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 [email protected]
Good catch!
LGTM
Thanks,
Jeff
Quoted 71 lines · Rose via GitGitGadget
From: Seija <[email protected]>
The documentation for
pthread_createandpthread_sigmaskstate 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 <[email protected]>---
maintenance: compare output of pthread functions for inequality with 0The documentation for
pthread_createandpthread_sigmaskstate 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 [email protected]
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/1389builtin/fsmonitor--daemon.c | 4 ++-- run-command.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)builtin/fsmonitor--daemon.c +2
-2@@ -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;run-command.c +1
-1@@ -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; }
Quoted 1 line
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...
Quoted 74 lines · Ævar Arnfjörð Bjarmason
Quoted 71 lines · Rose via GitGitGadget
From: Seija <[email protected]>
The documentation for
pthread_createandpthread_sigmaskstate 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 <[email protected]>---
maintenance: compare output of pthread functions for inequality with 0The documentation for
pthread_createandpthread_sigmaskstate 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 [email protected]
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/1389builtin/fsmonitor--daemon.c | 4 ++-- run-command.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)builtin/fsmonitor--daemon.c +2
-2@@ -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;run-command.c +1
-1@@ -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; }
Quoted 8 lines
Quoted 1 line
base-commit:
805265fcf7a737664a8321aaf4a0587b78435184This 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.
Quoted 15 lines
* Call
die_errno()(good)
* Calldie(),error()etc., but with a manualstrerror()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...
Quoted 5 lines · Ævar Arnfjörð Bjarmason
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:
Quoted 7 lines
* Call
die_errno()(good)
* Calldie(),error()etc., but with a manualstrerror()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.
Quoted 3 lines
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.
Signature · 2 lines
brian m. carlson (he/him or they/them) Toronto, Ontario, CA
Quoted 21 lines · brian m. carlson
[[PGP Signed Part:Undecided]]
Quoted 5 lines · Ævar Arnfjörð Bjarmason
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:Quoted 7 lines
* Call
die_errno()(good)
* Calldie(),error()etc., but with a manualstrerror()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.
Quoted 3 lines
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()).
Quoted 5 lines
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.
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 5 lines · brian m. carlson
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.
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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.
Signature · 2 lines
brian m. carlson (he/him or they/them) Toronto, Ontario, CA