[PATCH] maintenance: compare output of pthread functions for inequality with 0

[PATCH] maintenance: compare output of pthread functions for inequality with 0

[Rose via GitGitGadget]

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
-- 
gitgitgadget

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[Jeff Hostetler]

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

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[Ævar Arnfjörð Bjarmason]

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...

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[Jeff Hostetler]

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...
> 

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH] maintenance: compare output of pthread functions for inequality with 0

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]