Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa <akiyks@gmail.com>]

Hi Paul,

See inline comments below for a few nits and suggestions.

On 2018/07/14 16:33:13 -0700, Paul E. McKenney wrote:
> On Sat, Jul 14, 2018 at 08:59:48AM -0400, Elad Lahav wrote:
>> Hello,
>>
>> Throughout section 4.2 the following pattern is used for reporting
>> errors returned by various pthread functions:
>>
>> if (pthread_func() == 0) {
>>     perror("pthread_func");
>>     exit(-1);
>> }
>>
>> This is wrong, as pthread functions return the error number on failure
>> and do not set errno, as demonstrated by the following program, which
>> attempts to join with a detached thread:
>>
>> #include <stdio.h>
>> #include <errno.h>
>> #include <pthread.h>
>>
>> static void *
>> my_thread(void *arg)
>> {
>>     return NULL;
>> }
>>
>> int
>> main()
>> {
>>     pthread_attr_t  attr;
>>     pthread_t       tid;
>>
>>     pthread_attr_init(&attr);
>>     pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
>>
>>     errno = 0;
>>
>>     if (pthread_create(&tid, &attr, my_thread, NULL) != 0) {
>>         perror("pthread_create");
>>         return 1;
>>     }
>>
>>     if (pthread_join(tid, NULL) != 0) {
>>         perror("pthread_join");
>>         return 1;
>>     }
>>
>>     return 0;
>> }
>>
>> The result:
>> # ./pthread_error
>> pthread_join: Success
>>
>> The correct way to report errors is to use strerror() on the returned code:
>>
>> #include <string.h>
>> ...
>>     int rc;
>>     rc = pthread_join(tid, NULL);
>>     if (rc != 0) {
>>         fprintf(stderr, "pthread_join: %s\n", strerror(rc));
>>         return 1;
>>     }
>>
>> # ./pthread_error
>> pthread_join: Invalid argument
> 
> Huh.  I haven't seen a failure.
> 
> But you are right, the documentation clearly states that these calls
> return zero for success or errno otherwise.  Some systems explicitly
> say that errno is unaffected, others guarantee setting errno as I was
> naively expecting.  The standard is silent on the treatment of errno.
> Some people suggest assigning the return value of the pthread_*()
> functions to errno, while others argue that any use whatsoever of errno
> is signal-unsafe (looks like saving and restoring errno in your signal
> handlers is the right approach, which is a timely reminder for me).
> 
> Well, portability is worth a few lines of code, so I will make these
> changes with your Reported-by.
> 
> Just out of curiosity, did you find these by inspection, or do they
> actually fail in your environment?
> 
>> Also, using exit(-1) is likely to produce unexpected results for the
>> parent process. Error codes returned from processes are small
>> integers, with the range being platform-specific. This means that -1
>> will get truncated to some unspecified value. It is customary to use 1
>> to indicate a failure from a process (if no other specific code is
>> defined). stdlib.h provides EXIT_SUCCESS and EXIT_FAILURE, which may
>> be easier to read.
> 
> Also a fair point.
> 
> And yes, I did start C programming long before there was such a thing
> as stdlib.h.  Why do you ask?  ;-)
> 
> Please see below for the first of a number of patches.  Thoughts?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 2568586a3b09fa8f497e724e0e31ba020a37275a
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Sat Jul 14 16:29:34 2018 -0700
> 
>     toolsoftrade: Standard pthread error handling and exit() arguments
>     
>     This commit uses the pthread library calls' return value to do error
>     checking and handling, instead of the old reliance on errno, which for
>     these functions is not guaranteed by the relevant standards.  This commit
>     also uses EXIT_SUCCESS and EXIT_FAILURE to indicate exit status.
>     
>     Reported-by: Elad Lahav <e2lahav@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/CodeSamples/toolsoftrade/forkjoin.c b/CodeSamples/toolsoftrade/forkjoin.c
> index d578221f6f40..b03b545f2987 100644
> --- a/CodeSamples/toolsoftrade/forkjoin.c
> +++ b/CodeSamples/toolsoftrade/forkjoin.c
> @@ -34,7 +34,7 @@ int main(int argc, char *argv[])
>  
>  	if (argc != 2) {
>  		fprintf(stderr, "Usage: %s nforks\n", argv[0]);
> -		exit(-1);
> +		exit(EXIT_FAILURE);
>  	}
>  	nforks = atoi(argv[1]);
>  	printf("%d fork()s\n", nforks);
> @@ -52,7 +52,7 @@ int main(int argc, char *argv[])
>  		}
>  		if (pid < 0) { /* parent, upon error */
>  			perror("fork");
> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  	}
>  
> @@ -66,5 +66,5 @@ int main(int argc, char *argv[])
>  		fprintf(stderr, "system(\"date\") failed: %x\n", stat_val);
>  	}
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/forkjoinperf.c b/CodeSamples/toolsoftrade/forkjoinperf.c
> index c1438886e58a..19bad3e19acb 100644
> --- a/CodeSamples/toolsoftrade/forkjoinperf.c
> +++ b/CodeSamples/toolsoftrade/forkjoinperf.c
> @@ -34,7 +34,7 @@ int main(int argc, char *argv[])
>  
>  	if (argc != 2) {
>  		fprintf(stderr, "Usage: %s nforks\n", argv[0]);
> -		exit(-1);
> +		exit(EXIT_FAILURE);
>  	}
>  	nforks = atoi(argv[1]);
>  	printf("%d fork()s\n", nforks);
> @@ -44,11 +44,11 @@ int main(int argc, char *argv[])
>  	for (i = 0; i < nforks; i++) {
>  		pid = fork();
>  		if (pid == 0) { /* child */
> -			exit(0);
> +			exit(EXIT_SUCCESS);
>  		}
>  		if (pid < 0) { /* parent, upon error */
>  			perror("fork");
> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  		for (;;) {
>  			pid = wait(&status);
> @@ -56,7 +56,7 @@ int main(int argc, char *argv[])
>  				if (errno == ECHILD)
>  					break;
>  				perror("wait");
> -				exit(-1);
> +				exit(EXIT_FAILURE);
>  			}
>  		}
>  	}
> @@ -64,5 +64,5 @@ int main(int argc, char *argv[])
>  	fflush(stdout);
>  	i = system("date");
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/forkjoinvar.c b/CodeSamples/toolsoftrade/forkjoinvar.c
> index 638558b726db..571acc14d8db 100644
> --- a/CodeSamples/toolsoftrade/forkjoinvar.c
> +++ b/CodeSamples/toolsoftrade/forkjoinvar.c
> @@ -34,11 +34,11 @@ int main(int argc, char *argv[])
>  	if (pid == 0) { /* child */
>  		x = 1;
>  		printf("Child process set x=1\n");
> -		exit(0);
> +		exit(EXIT_SUCCESS);
>  	}
>  	if (pid < 0) { /* parent, upon error */
>  		perror("fork");
> -		exit(-1);
> +		exit(EXIT_FAILURE);
>  	}
>  
>  	/* parent */
> @@ -46,5 +46,5 @@ int main(int argc, char *argv[])
>  	waitall();
>  	printf("Parent process sees x=%d\n", x);
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/lock.c b/CodeSamples/toolsoftrade/lock.c
> index c5ad6ebf37f3..6bb3c23b43ba 100644
> --- a/CodeSamples/toolsoftrade/lock.c
> +++ b/CodeSamples/toolsoftrade/lock.c
> @@ -33,14 +33,16 @@ int x = 0;
>  
>  void *lock_reader(void *arg)
>  {
> +	int en;
>  	int i;
>  	int newx = -1;
>  	int oldx = -1;
>  	pthread_mutex_t *pmlp = (pthread_mutex_t *)arg;
>  
> -	if (pthread_mutex_lock(pmlp) != 0) {
> -		perror("lock_reader:pthread_mutex_lock");
> -		exit(-1);
> +	if ((en = pthread_mutex_lock(pmlp)) != 0) {
> +		fprintf(stderr, "lock_reader:pthread_mutex_lock: %s\n",
> +			strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  	for (i = 0; i < 100; i++) {
>  		newx = READ_ONCE(x);
> @@ -50,75 +52,80 @@ void *lock_reader(void *arg)
>  		oldx = newx;
>  		poll(NULL, 0, 1);
>  	}
> -	if (pthread_mutex_unlock(pmlp) != 0) {
> -		perror("lock_reader:pthread_mutex_unlock");
> -		exit(-1);
> +	if ((en = pthread_mutex_unlock(pmlp)) != 0) {
> +		fprintf(stderr, "lock_reader:pthread_mutex_lock: %s\n",
> +			strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  	return NULL;
>  }
>  
>  void *lock_writer(void *arg)
>  {
> +	int en;
>  	int i;
>  	pthread_mutex_t *pmlp = (pthread_mutex_t *)arg;
>  
> -	if (pthread_mutex_lock(pmlp) != 0) {
> -		perror("lock_writer:pthread_mutex_lock");
> -		exit(-1);
> +	if ((en = pthread_mutex_lock(pmlp)) != 0) {
> +		fprintf(stderr, "lock_writer:pthread_mutex_lock: %s\n",
> +			strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  	for (i = 0; i < 3; i++) {
>  		WRITE_ONCE(x, READ_ONCE(x) + 1);
>  		poll(NULL, 0, 5);
>  	}
> -	if (pthread_mutex_unlock(pmlp) != 0) {
> -		perror("lock_writer:pthread_mutex_unlock");
> -		exit(-1);
> +	if ((en = pthread_mutex_unlock(pmlp)) != 0) {
> +		fprintf(stderr, "lock_writer:pthread_mutex_lock: %s\n",
> +			strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  	return NULL;
>  }
>  
>  int main(int argc, char *argv[])
>  {
> +	int en; 
>  	pthread_t tid1;
>  	pthread_t tid2;
>  	void *vp;
>  
>  	printf("Creating two threads using same lock:\n");
> -	if (pthread_create(&tid1, NULL, lock_reader, &lock_a) != 0) {
> -		perror("pthread_create");
> -		exit(-1);
> +	if ((en = pthread_create(&tid1, NULL, lock_reader, &lock_a)) != 0) {
> +		fprintf(stderr, "pthread_create: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_create(&tid2, NULL, lock_writer, &lock_a) != 0) {
> -		perror("pthread_create");
> -		exit(-1);
> +	if ((en = pthread_create(&tid2, NULL, lock_writer, &lock_a)) != 0) {
> +		fprintf(stderr, "pthread_create: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_join(tid1, &vp) != 0) {
> -		perror("pthread_join");
> -		exit(-1);
> +	if ((en = pthread_join(tid1, &vp)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_join(tid2, &vp) != 0) {
> -		perror("pthread_join");
> -		exit(-1);
> +	if ((en = pthread_join(tid2, &vp)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  
>  	printf("Creating two threads w/different locks:\n");
>  	x = 0;
> -	if (pthread_create(&tid1, NULL, lock_reader, &lock_a) != 0) {
> -		perror("pthread_create");
> -		exit(-1);
> +	if ((en = pthread_create(&tid1, NULL, lock_reader, &lock_a)) != 0) {
> +		fprintf(stderr, "pthread_create: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_create(&tid2, NULL, lock_writer, &lock_b) != 0) {
> -		perror("pthread_create");
> -		exit(-1);
> +	if ((en = pthread_create(&tid2, NULL, lock_writer, &lock_b)) != 0) {
> +		fprintf(stderr, "pthread_create: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_join(tid1, &vp) != 0) {
> -		perror("pthread_join");
> -		exit(-1);
> +	if ((en = pthread_join(tid1, &vp)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
> -	if (pthread_join(tid2, &vp) != 0) {
> -		perror("pthread_join");
> -		exit(-1);
> +	if ((en = pthread_join(tid2, &vp)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/mytrue.c b/CodeSamples/toolsoftrade/mytrue.c
> index a6ee655c802c..59cbe48d32d3 100644
> --- a/CodeSamples/toolsoftrade/mytrue.c
> +++ b/CodeSamples/toolsoftrade/mytrue.c
> @@ -23,5 +23,5 @@
>  
>  int main(int argc, char *argv[])
>  {
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/pcreate.c b/CodeSamples/toolsoftrade/pcreate.c
> index feca73c4faf6..5241f8f96c76 100644
> --- a/CodeSamples/toolsoftrade/pcreate.c
> +++ b/CodeSamples/toolsoftrade/pcreate.c
> @@ -37,21 +37,22 @@ void *mythread(void *arg)
>  
>  int main(int argc, char *argv[])
>  {
> +	int en;
>  	pthread_t tid;
>  	void *vp;
>  
> -	if (pthread_create(&tid, NULL, mythread, NULL) != 0) {
> -		perror("pthread_create");
> -		exit(-1);
> +	if ((en = pthread_create(&tid, NULL, mythread, NULL)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  
>  	/* parent */
>  
> -	if (pthread_join(tid, &vp) != 0) {
> -		perror("pthread_join");
> -		exit(-1);
> +	if ((en = pthread_join(tid, &vp)) != 0) {
> +		fprintf(stderr, "pthread_join: %s\n", strerror(en));
> +		exit(EXIT_FAILURE);
>  	}
>  	printf("Parent process sees x=%d\n", x);
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> diff --git a/CodeSamples/toolsoftrade/rwlockscale.c b/CodeSamples/toolsoftrade/rwlockscale.c
> index 1bf487e7099c..136b799a160b 100644
> --- a/CodeSamples/toolsoftrade/rwlockscale.c
> +++ b/CodeSamples/toolsoftrade/rwlockscale.c
> @@ -39,6 +39,7 @@ char goflag = GOFLAG_INIT;
>  
>  void *reader(void *arg)
>  {
> +	int en;
>  	int i;
>  	long long loopcnt = 0;
>  	long me = (long)arg;
> @@ -48,16 +49,16 @@ void *reader(void *arg)
>  		continue;
>  	}
>  	while (READ_ONCE(goflag) == GOFLAG_RUN) {
> -		if (pthread_rwlock_rdlock(&rwl) != 0) {
> +		if ((en = pthread_rwlock_rdlock(&rwl)) != 0) {
>  			perror("pthread_rwlock_rdlock");

This perror() also needs update.

> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  		for (i = 1; i < holdtime; i++) {
>  			barrier();
>  		}
> -		if (pthread_rwlock_unlock(&rwl) != 0) {
> +		if ((en = pthread_rwlock_unlock(&rwl)) != 0) {
>  			perror("pthread_rwlock_unlock");

Ditto.

> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  		for (i = 1; i < thinktime; i++) {
>  			barrier();
> @@ -70,6 +71,7 @@ void *reader(void *arg)
>  
>  int main(int argc, char *argv[])
>  {
> +	int en;
>  	long i;
>  	int nthreads;
>  	long long sum = 0LL;
> @@ -79,7 +81,7 @@ int main(int argc, char *argv[])
>  	if (argc != 4) {
>  		fprintf(stderr,
>  			"Usage: %s nthreads holdloops thinkloops\n", argv[0]);
> -		exit(-1);
> +		exit(EXIT_FAILURE);
>  	}
>  	nthreads = strtoul(argv[1], NULL, 0);
>  	holdtime = strtoul(argv[2], NULL, 0);
> @@ -88,12 +90,13 @@ int main(int argc, char *argv[])
>  	tid = malloc(nthreads * sizeof(tid[0]));
>  	if (readcounts == NULL || tid == NULL) {
>  		fprintf(stderr, "Out of memory\n");
> -		exit(-1);
> +		exit(EXIT_FAILURE);
>  	}
>  	for (i = 0; i < nthreads; i++) {
> -		if (pthread_create(&tid[i], NULL, reader, (void *)i) != 0) {
> +		en = pthread_create(&tid[i], NULL, reader, (void *)i);
> +		if (en != 0) {
>  			perror("pthread_create");

Ditto.

> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  	}
>  	while (READ_ONCE(nreadersrunning) < nthreads) {
> @@ -104,9 +107,9 @@ int main(int argc, char *argv[])
>  	goflag = GOFLAG_STOP;
>  
>  	for (i = 0; i < nthreads; i++) {
> -		if (pthread_join(tid[i], &vp) != 0) {
> +		if ((en = pthread_join(tid[i], &vp)) != 0) {
>  			perror("pthread_join");

Ditto.

> -			exit(-1);
> +			exit(EXIT_FAILURE);
>  		}
>  	}
>  	for (i = 0; i < nthreads; i++) {
> @@ -116,5 +119,5 @@ int main(int argc, char *argv[])
>  	printf("%s n: %d  h: %d t: %d  sum: %lld\n",
>  	       argv[0], nthreads, holdtime, thinktime, sum);
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe perfbook" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

I see you have already updated most of the code samples under CodeSamples/,
but let me suggest an alternative way not to increase line counts
(or even to decrease line counts).

"pthread_create(3)" man page gives you an example code.

First, two helpers are defined as follows:

       #define handle_error_en(en, msg) \
               do { errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)

       #define handle_error(msg) \
               do { perror(msg); exit(EXIT_FAILURE); } while (0)

Then, one of the call sites looks as follows:

               s = pthread_create(&tinfo[tnum].thread_id, &attr,
                                  &thread_start, &tinfo[tnum]);
               if (s != 0)
                   handle_error_en(s, "pthread_create");

If we employ this pattern, one of the hunks in your patch will look like:

-	if (pthread_mutex_lock(pmlp) != 0) {
-		perror("lock_reader:pthread_mutex_lock");
-		exit(-1);
-	}
+	if ((en = pthread_mutex_lock(pmlp)) != 0)
+		handle_error_en(en, "lock_reader:pthread_mutex_lock");

Thoughts?

I think these error cases are not our main topic, and to hide the
details inside helpers sounds reasonable.

Also, wouldn't it be a good idea to employ auto-numbering scheme as
mentioned in Section D.3.1.1 of Style Guide when updating code snippets?
This update will involve a lot of renumbering of line numbers otherwise.

If you feel OK with this approach, I can prepare a patch series
on behalf of you. (Can take a little while, though.)

                  Thanks, Akira