Section 4.2: wrong error reporting for pthread functions

Section 4.2: wrong error reporting for pthread functions

[Elad Lahav]

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

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.

--Elad

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

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");
-			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");
-			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");
-			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");
-			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;
 }

Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa]

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

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

On Tue, Jul 17, 2018 at 12:42:57AM +0900, Akira Yokosawa wrote:
> Hi Paul,
> 
> See inline comments below for a few nits and suggestions.

I fixed the perror() calls straightforwardly, thank you!
Queued and pushed with both your and Elad's Reported-by.

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

[ . . . ]

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

Does it make sense to pull the "if" into the handle_error_en() macro
as well, perhaps like this?

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

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

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

This approach involves labeling lines that are referred to in the text?
If those labels could be introduced as comments in the original code,
that could be really nice!

							Thanx, Paul

Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa]

On 2018/07/16 09:39:24 -0700, Paul E. McKenney wrote:
> On Tue, Jul 17, 2018 at 12:42:57AM +0900, Akira Yokosawa wrote:
>> Hi Paul,
>>
>> See inline comments below for a few nits and suggestions.
> 
> I fixed the perror() calls straightforwardly, thank you!
> Queued and pushed with both your and Elad's Reported-by.
> 
>> 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:
> 
> [ . . . ]
> 
>> 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.
> 
> Does it make sense to pull the "if" into the handle_error_en() macro
> as well, perhaps like this?
> 
> #define handle_error_en(en, msg) \
> 	do { if (!en) break; errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)
> 
> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
> 			   &thread_start, &tinfo[tnum]);
> 	handle_error_en(s, "pthread_create");
> 

This version of handle_error_en() can return.
As per Elad's suggestion, if we want to make fatal_en() not to return,
we can't pull the "if".

It looks to me by keeping the "if" out of helper funcs, fatal-error
conditions can be made more obvious.

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

I prefer this approach.

>> 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.)
> 
> This approach involves labeling lines that are referred to in the text?
> If those labels could be introduced as comments in the original code,
> that could be really nice!

By using "fancyvrb" package instead of "verbatimbox", this is mostly
possible.  by "mostly possible", I mean label within comments can
be made invisible in the output, but "/*", "*/", and "//" will remain
in the output.

For example, original source code of Listing 4.1 (using fatal() helper)
would look like:

	pid = fork(); //%label[ln:toolsoftrade:fork:fork]
	if (pid == 0) { //%label[ln:toolsoftrade:fork:if]
		/* child */ //%label[ln:toolsoftrade:fork:child]
	} else if (pid < 0) { //%label[ln:toolsoftrade:fork:else]
		/* parent, upon error */ //%label[ln:toolsoftrade:fork:errora]
		fatal("fork"); //%label[ln:toolsoftrade:fork:errorb]
	} else {
		/* parent, pid == child ID */ //%label[ln:toolsoftrade:fork:parent]
	}

Corresponding source of the code snippet (after removal of "//") would
look like:

  pid = fork();%label[ln:toolsoftrade:fork:fork]
  if (pid == 0) {%label[ln:toolsoftrade:fork:if]
    /* child */%label[ln:toolsoftrade:fork:child]
  } else if (pid < 0) {%label[ln:toolsoftrade:fork:else]
    /* parent, upon error */%label[ln:toolsoftrade:fork:errora]
    fatal("fork");%label[ln:toolsoftrade:fork:errorb]
  } else {
    /* parent, pid == child ID */%label[ln:toolsoftrade:fork:parent]
  }

Note that in this example, "%" represents "\" after escaping to LaTeX,
"[" for "{", "]" for "}". These 3 escaping characters need to be
chosen for each snippet so that they do not appear in the unescaped code.
"[" and "]" can not be used for snippets that used array reference,
for example.

So in theory, we can do what you want, but need somewhat ad-hoc
manual tweaks. Still, it might be possible to write a script or two
to do such tweaks in a semi-automated way.

If you'd like to see what the code snippet and reference to labels
would be,  I can prepare a experimental branch which is relative to
commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
the Linux kernel").

Thoughts?

        Thanks, Akira

> 
> 							Thanx, Paul
>

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

On Wed, Jul 18, 2018 at 12:43:16AM +0900, Akira Yokosawa wrote:
> On 2018/07/16 09:39:24 -0700, Paul E. McKenney wrote:
> > On Tue, Jul 17, 2018 at 12:42:57AM +0900, Akira Yokosawa wrote:
> >> Hi Paul,
> >>
> >> See inline comments below for a few nits and suggestions.
> > 
> > I fixed the perror() calls straightforwardly, thank you!
> > Queued and pushed with both your and Elad's Reported-by.
> > 
> >> 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:
> > 
> > [ . . . ]
> > 
> >> 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.
> > 
> > Does it make sense to pull the "if" into the handle_error_en() macro
> > as well, perhaps like this?
> > 
> > #define handle_error_en(en, msg) \
> > 	do { if (!en) break; errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)
> > 
> > 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
> > 			   &thread_start, &tinfo[tnum]);
> > 	handle_error_en(s, "pthread_create");
> > 
> 
> This version of handle_error_en() can return.
> As per Elad's suggestion, if we want to make fatal_en() not to return,
> we can't pull the "if".
> 
> It looks to me by keeping the "if" out of helper funcs, fatal-error
> conditions can be made more obvious.
> 
> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
> 			   &thread_start, &tinfo[tnum]);
> 	if (s != 0)
> 		fatal_en(s, "pthread_create");
> 
> I prefer this approach.

At this point, I believe that the right approach is for me to instead
make use of the existing api.h wrappers for the pthread_*() functions.
Smaller code and fewer places to change as opinions shift.  I was
thinking in terms of instead moving to the pthread_*() functions, but
this discussion has clearly shown the folly of that approach.  ;-)

The exceptions are of course the exposition of the pthread_*()
functions in the toolsoftrade chapter.

> >> 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.)
> > 
> > This approach involves labeling lines that are referred to in the text?
> > If those labels could be introduced as comments in the original code,
> > that could be really nice!
> 
> By using "fancyvrb" package instead of "verbatimbox", this is mostly
> possible.  by "mostly possible", I mean label within comments can
> be made invisible in the output, but "/*", "*/", and "//" will remain
> in the output.
> 
> For example, original source code of Listing 4.1 (using fatal() helper)
> would look like:
> 
> 	pid = fork(); //%label[ln:toolsoftrade:fork:fork]
> 	if (pid == 0) { //%label[ln:toolsoftrade:fork:if]
> 		/* child */ //%label[ln:toolsoftrade:fork:child]
> 	} else if (pid < 0) { //%label[ln:toolsoftrade:fork:else]
> 		/* parent, upon error */ //%label[ln:toolsoftrade:fork:errora]
> 		fatal("fork"); //%label[ln:toolsoftrade:fork:errorb]
> 	} else {
> 		/* parent, pid == child ID */ //%label[ln:toolsoftrade:fork:parent]
> 	}
> 
> Corresponding source of the code snippet (after removal of "//") would
> look like:
> 
>   pid = fork();%label[ln:toolsoftrade:fork:fork]
>   if (pid == 0) {%label[ln:toolsoftrade:fork:if]
>     /* child */%label[ln:toolsoftrade:fork:child]
>   } else if (pid < 0) {%label[ln:toolsoftrade:fork:else]
>     /* parent, upon error */%label[ln:toolsoftrade:fork:errora]
>     fatal("fork");%label[ln:toolsoftrade:fork:errorb]
>   } else {
>     /* parent, pid == child ID */%label[ln:toolsoftrade:fork:parent]
>   }
> 
> Note that in this example, "%" represents "\" after escaping to LaTeX,
> "[" for "{", "]" for "}". These 3 escaping characters need to be
> chosen for each snippet so that they do not appear in the unescaped code.
> "[" and "]" can not be used for snippets that used array reference,
> for example.
> 
> So in theory, we can do what you want, but need somewhat ad-hoc
> manual tweaks. Still, it might be possible to write a script or two
> to do such tweaks in a semi-automated way.

I already use scripts to do the auto-numbering and auto-intenting
for the old-style listings, so why not?  ;-)

If I haven't already made these available, please let me know and
I can send them on.  They aren't exactly profound.

> If you'd like to see what the code snippet and reference to labels
> would be,  I can prepare a experimental branch which is relative to
> commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
> the Linux kernel").
> 
> Thoughts?

Sounds worth a try, thank you!

							Thanx, Paul

Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa]

On 2018/07/17 09:15:31 -0700, Paul E. McKenney wrote:
> On Wed, Jul 18, 2018 at 12:43:16AM +0900, Akira Yokosawa wrote:
>> On 2018/07/16 09:39:24 -0700, Paul E. McKenney wrote:
>>> On Tue, Jul 17, 2018 at 12:42:57AM +0900, Akira Yokosawa wrote:
>>>> Hi Paul,
>>>>
>>>> See inline comments below for a few nits and suggestions.
>>>
>>> I fixed the perror() calls straightforwardly, thank you!
>>> Queued and pushed with both your and Elad's Reported-by.
>>>
>>>> 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:
>>>
>>> [ . . . ]
>>>
>>>> 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.
>>>
>>> Does it make sense to pull the "if" into the handle_error_en() macro
>>> as well, perhaps like this?
>>>
>>> #define handle_error_en(en, msg) \
>>> 	do { if (!en) break; errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)
>>>
>>> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
>>> 			   &thread_start, &tinfo[tnum]);
>>> 	handle_error_en(s, "pthread_create");
>>>
>>
>> This version of handle_error_en() can return.
>> As per Elad's suggestion, if we want to make fatal_en() not to return,
>> we can't pull the "if".
>>
>> It looks to me by keeping the "if" out of helper funcs, fatal-error
>> conditions can be made more obvious.
>>
>> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
>> 			   &thread_start, &tinfo[tnum]);
>> 	if (s != 0)
>> 		fatal_en(s, "pthread_create");
>>
>> I prefer this approach.
> 
> At this point, I believe that the right approach is for me to instead
> make use of the existing api.h wrappers for the pthread_*() functions.
> Smaller code and fewer places to change as opinions shift.  I was
> thinking in terms of instead moving to the pthread_*() functions, but
> this discussion has clearly shown the folly of that approach.  ;-)
> 
> The exceptions are of course the exposition of the pthread_*()
> functions in the toolsoftrade chapter.

Sounds reasonable to me.

> 
>>>> 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.)
>>>
>>> This approach involves labeling lines that are referred to in the text?
>>> If those labels could be introduced as comments in the original code,
>>> that could be really nice!
>>
>> By using "fancyvrb" package instead of "verbatimbox", this is mostly
>> possible.  by "mostly possible", I mean label within comments can
>> be made invisible in the output, but "/*", "*/", and "//" will remain
>> in the output.
>>
>> For example, original source code of Listing 4.1 (using fatal() helper)
>> would look like:
>>
>> 	pid = fork(); //%label[ln:toolsoftrade:fork:fork]
>> 	if (pid == 0) { //%label[ln:toolsoftrade:fork:if]
>> 		/* child */ //%label[ln:toolsoftrade:fork:child]
>> 	} else if (pid < 0) { //%label[ln:toolsoftrade:fork:else]
>> 		/* parent, upon error */ //%label[ln:toolsoftrade:fork:errora]
>> 		fatal("fork"); //%label[ln:toolsoftrade:fork:errorb]
>> 	} else {
>> 		/* parent, pid == child ID */ //%label[ln:toolsoftrade:fork:parent]
>> 	}
>>
>> Corresponding source of the code snippet (after removal of "//") would
>> look like:
>>
>>   pid = fork();%label[ln:toolsoftrade:fork:fork]
>>   if (pid == 0) {%label[ln:toolsoftrade:fork:if]
>>     /* child */%label[ln:toolsoftrade:fork:child]
>>   } else if (pid < 0) {%label[ln:toolsoftrade:fork:else]
>>     /* parent, upon error */%label[ln:toolsoftrade:fork:errora]
>>     fatal("fork");%label[ln:toolsoftrade:fork:errorb]
>>   } else {
>>     /* parent, pid == child ID */%label[ln:toolsoftrade:fork:parent]
>>   }
>>
>> Note that in this example, "%" represents "\" after escaping to LaTeX,
>> "[" for "{", "]" for "}". These 3 escaping characters need to be
>> chosen for each snippet so that they do not appear in the unescaped code.
>> "[" and "]" can not be used for snippets that used array reference,
>> for example.
>>
>> So in theory, we can do what you want, but need somewhat ad-hoc
>> manual tweaks. Still, it might be possible to write a script or two
>> to do such tweaks in a semi-automated way.
> 
> I already use scripts to do the auto-numbering and auto-intenting
> for the old-style listings, so why not?  ;-)
> 
> If I haven't already made these available, please let me know and
> I can send them on.  They aren't exactly profound.

So, there are 3 symbolic links under utilities/ in the Git repo:

    c2latex.sh -> /home/paulmck/bin/c2latex.sh
    extractClatex.sh -> /home/paulmck/bin/extractClatex.sh
    latex2c.sh -> /home/paulmck/bin/latex2c.sh

Now I know why I have no idea how you manage code snippets. ;-)

> 
>> If you'd like to see what the code snippet and reference to labels
>> would be,  I can prepare a experimental branch which is relative to
>> commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
>> the Linux kernel").
>>
>> Thoughts?
> 
> Sounds worth a try, thank you!

OK.
I'll send a pseudo pull request when it is ready.
Hopefully in a couple of days.

      Thanks, Akira

> 
> 							Thanx, Paul
> 

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

On Wed, Jul 18, 2018 at 09:15:12PM +0900, Akira Yokosawa wrote:
> On 2018/07/17 09:15:31 -0700, Paul E. McKenney wrote:
> > On Wed, Jul 18, 2018 at 12:43:16AM +0900, Akira Yokosawa wrote:
> >> On 2018/07/16 09:39:24 -0700, Paul E. McKenney wrote:
> >>> On Tue, Jul 17, 2018 at 12:42:57AM +0900, Akira Yokosawa wrote:
> >>>> Hi Paul,
> >>>>
> >>>> See inline comments below for a few nits and suggestions.
> >>>
> >>> I fixed the perror() calls straightforwardly, thank you!
> >>> Queued and pushed with both your and Elad's Reported-by.
> >>>
> >>>> 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:
> >>>
> >>> [ . . . ]
> >>>
> >>>> 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.
> >>>
> >>> Does it make sense to pull the "if" into the handle_error_en() macro
> >>> as well, perhaps like this?
> >>>
> >>> #define handle_error_en(en, msg) \
> >>> 	do { if (!en) break; errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)
> >>>
> >>> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
> >>> 			   &thread_start, &tinfo[tnum]);
> >>> 	handle_error_en(s, "pthread_create");
> >>>
> >>
> >> This version of handle_error_en() can return.
> >> As per Elad's suggestion, if we want to make fatal_en() not to return,
> >> we can't pull the "if".
> >>
> >> It looks to me by keeping the "if" out of helper funcs, fatal-error
> >> conditions can be made more obvious.
> >>
> >> 	s = pthread_create(&tinfo[tnum].thread_id, &attr,
> >> 			   &thread_start, &tinfo[tnum]);
> >> 	if (s != 0)
> >> 		fatal_en(s, "pthread_create");
> >>
> >> I prefer this approach.
> > 
> > At this point, I believe that the right approach is for me to instead
> > make use of the existing api.h wrappers for the pthread_*() functions.
> > Smaller code and fewer places to change as opinions shift.  I was
> > thinking in terms of instead moving to the pthread_*() functions, but
> > this discussion has clearly shown the folly of that approach.  ;-)
> > 
> > The exceptions are of course the exposition of the pthread_*()
> > functions in the toolsoftrade chapter.
> 
> Sounds reasonable to me.
> 
> > 
> >>>> 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.)
> >>>
> >>> This approach involves labeling lines that are referred to in the text?
> >>> If those labels could be introduced as comments in the original code,
> >>> that could be really nice!
> >>
> >> By using "fancyvrb" package instead of "verbatimbox", this is mostly
> >> possible.  by "mostly possible", I mean label within comments can
> >> be made invisible in the output, but "/*", "*/", and "//" will remain
> >> in the output.
> >>
> >> For example, original source code of Listing 4.1 (using fatal() helper)
> >> would look like:
> >>
> >> 	pid = fork(); //%label[ln:toolsoftrade:fork:fork]
> >> 	if (pid == 0) { //%label[ln:toolsoftrade:fork:if]
> >> 		/* child */ //%label[ln:toolsoftrade:fork:child]
> >> 	} else if (pid < 0) { //%label[ln:toolsoftrade:fork:else]
> >> 		/* parent, upon error */ //%label[ln:toolsoftrade:fork:errora]
> >> 		fatal("fork"); //%label[ln:toolsoftrade:fork:errorb]
> >> 	} else {
> >> 		/* parent, pid == child ID */ //%label[ln:toolsoftrade:fork:parent]
> >> 	}
> >>
> >> Corresponding source of the code snippet (after removal of "//") would
> >> look like:
> >>
> >>   pid = fork();%label[ln:toolsoftrade:fork:fork]
> >>   if (pid == 0) {%label[ln:toolsoftrade:fork:if]
> >>     /* child */%label[ln:toolsoftrade:fork:child]
> >>   } else if (pid < 0) {%label[ln:toolsoftrade:fork:else]
> >>     /* parent, upon error */%label[ln:toolsoftrade:fork:errora]
> >>     fatal("fork");%label[ln:toolsoftrade:fork:errorb]
> >>   } else {
> >>     /* parent, pid == child ID */%label[ln:toolsoftrade:fork:parent]
> >>   }
> >>
> >> Note that in this example, "%" represents "\" after escaping to LaTeX,
> >> "[" for "{", "]" for "}". These 3 escaping characters need to be
> >> chosen for each snippet so that they do not appear in the unescaped code.
> >> "[" and "]" can not be used for snippets that used array reference,
> >> for example.
> >>
> >> So in theory, we can do what you want, but need somewhat ad-hoc
> >> manual tweaks. Still, it might be possible to write a script or two
> >> to do such tweaks in a semi-automated way.
> > 
> > I already use scripts to do the auto-numbering and auto-intenting
> > for the old-style listings, so why not?  ;-)
> > 
> > If I haven't already made these available, please let me know and
> > I can send them on.  They aren't exactly profound.
> 
> So, there are 3 symbolic links under utilities/ in the Git repo:
> 
>     c2latex.sh -> /home/paulmck/bin/c2latex.sh
>     extractClatex.sh -> /home/paulmck/bin/extractClatex.sh
>     latex2c.sh -> /home/paulmck/bin/latex2c.sh
> 
> Now I know why I have no idea how you manage code snippets. ;-)

Heh!  I extract the code by hand and remove comments and any resulting
extraneous blank lines.  I run it through c2latex.sh, and occasionally
need to manually fix indentation.  Then I do any needed renumbering
manually in the text.

And latex2c.sh is more or less the inverse of c2latex.sh.

> >> If you'd like to see what the code snippet and reference to labels
> >> would be,  I can prepare a experimental branch which is relative to
> >> commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
> >> the Linux kernel").
> >>
> >> Thoughts?
> > 
> > Sounds worth a try, thank you!
> 
> OK.
> I'll send a pseudo pull request when it is ready.
> Hopefully in a couple of days.

Sounds good!

							Thanx, Paul

Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa]

On 2018/07/18 06:14:48 -0700, Paul E. McKenney wrote:
> On Wed, Jul 18, 2018 at 09:15:12PM +0900, Akira Yokosawa wrote:
>> On 2018/07/17 09:15:31 -0700, Paul E. McKenney wrote:
>>> On Wed, Jul 18, 2018 at 12:43:16AM +0900, Akira Yokosawa wrote:
[...]
>>>> So in theory, we can do what you want, but need somewhat ad-hoc
>>>> manual tweaks. Still, it might be possible to write a script or two
>>>> to do such tweaks in a semi-automated way.
>>>
>>> I already use scripts to do the auto-numbering and auto-intenting
>>> for the old-style listings, so why not?  ;-)
>>>
>>> If I haven't already made these available, please let me know and
>>> I can send them on.  They aren't exactly profound.
>>
>> So, there are 3 symbolic links under utilities/ in the Git repo:
>>
>>     c2latex.sh -> /home/paulmck/bin/c2latex.sh
>>     extractClatex.sh -> /home/paulmck/bin/extractClatex.sh
>>     latex2c.sh -> /home/paulmck/bin/latex2c.sh
>>
>> Now I know why I have no idea how you manage code snippets. ;-)
> 
> Heh!  I extract the code by hand and remove comments and any resulting
> extraneous blank lines.  I run it through c2latex.sh, and occasionally
> need to manually fix indentation.  Then I do any needed renumbering
> manually in the text.
> 
> And latex2c.sh is more or less the inverse of c2latex.sh.

Well, I mean if you don't mind, I'd like to see those scripts
in the Git repo rather than the symbolic links.

        Thanks, Akira

> 
>>>> If you'd like to see what the code snippet and reference to labels
>>>> would be,  I can prepare a experimental branch which is relative to
>>>> commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
>>>> the Linux kernel").
>>>>
>>>> Thoughts?
>>>
>>> Sounds worth a try, thank you!
>>
>> OK.
>> I'll send a pseudo pull request when it is ready.
>> Hopefully in a couple of days.
> 
> Sounds good!
> 
> 							Thanx, Paul
> 
> --
> 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
> 

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

On Wed, Jul 18, 2018 at 10:42:45PM +0900, Akira Yokosawa wrote:
> On 2018/07/18 06:14:48 -0700, Paul E. McKenney wrote:
> > On Wed, Jul 18, 2018 at 09:15:12PM +0900, Akira Yokosawa wrote:
> >> On 2018/07/17 09:15:31 -0700, Paul E. McKenney wrote:
> >>> On Wed, Jul 18, 2018 at 12:43:16AM +0900, Akira Yokosawa wrote:
> [...]
> >>>> So in theory, we can do what you want, but need somewhat ad-hoc
> >>>> manual tweaks. Still, it might be possible to write a script or two
> >>>> to do such tweaks in a semi-automated way.
> >>>
> >>> I already use scripts to do the auto-numbering and auto-intenting
> >>> for the old-style listings, so why not?  ;-)
> >>>
> >>> If I haven't already made these available, please let me know and
> >>> I can send them on.  They aren't exactly profound.
> >>
> >> So, there are 3 symbolic links under utilities/ in the Git repo:
> >>
> >>     c2latex.sh -> /home/paulmck/bin/c2latex.sh
> >>     extractClatex.sh -> /home/paulmck/bin/extractClatex.sh
> >>     latex2c.sh -> /home/paulmck/bin/latex2c.sh
> >>
> >> Now I know why I have no idea how you manage code snippets. ;-)
> > 
> > Heh!  I extract the code by hand and remove comments and any resulting
> > extraneous blank lines.  I run it through c2latex.sh, and occasionally
> > need to manually fix indentation.  Then I do any needed renumbering
> > manually in the text.
> > 
> > And latex2c.sh is more or less the inverse of c2latex.sh.
> 
> Well, I mean if you don't mind, I'd like to see those scripts
> in the Git repo rather than the symbolic links.

Hmmm...  I must admit that those symbolic links are a bit useless
outside of my laptop.  I have replaced them with the scripts themselves.
Please note that I haven't really used extractClatex.sh, though it did
seem like a good idea at the time.  :-/

							Thanx, Paul

>         Thanks, Akira
> 
> > 
> >>>> If you'd like to see what the code snippet and reference to labels
> >>>> would be,  I can prepare a experimental branch which is relative to
> >>>> commit f2b9d37d3b95 ("count: Expand on gap between C11 atomics and
> >>>> the Linux kernel").
> >>>>
> >>>> Thoughts?
> >>>
> >>> Sounds worth a try, thank you!
> >>
> >> OK.
> >> I'll send a pseudo pull request when it is ready.
> >> Hopefully in a couple of days.
> > 
> > Sounds good!
> > 
> > 							Thanx, Paul
> > 
> > --
> > 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
> > 
> 

Re: Section 4.2: wrong error reporting for pthread functions

[Akira Yokosawa]

Hi Elad,

On 2018/07/17 1:35, Elad Lahav wrote:
> On Mon, Jul 16, 2018 at 11:42 AM, Akira Yokosawa <akiyks@gmail.com> wrote:
>> 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?
> 
> You are, of course, free to do as you want with your book, but I would
> advise against the proposal. Novice software developers will often
> copy patterns from books, which means that the examples need to be
> held to a higher standard. I do agree that error handling is not the
> point of these examples, so you shouldn't spend too much time on it,
> but at least at one point it should show the correct pattern. The rest
> of the code can just have a "// Report error and exit" comment.
> If you do want a helper, then a better solution would be:
> 
> static inline void __attribute__((noreturn))
> fatal(char const * const msg, int const err)
> {
>     fprintf(stderr, "%s: %s\n", msg, strerror(err));
>     exit(EXIT_FAILURE);
> }
> 
> The name 'fatal' better conveys to the reader of the code the fact
> that the call doesn't return. The snippet also demonstrates a more
> modern approach to C code (use of inline functions, const, function
> attributes).

Thank you for your feedback!

I agree with you that inline functions are better choice.
You might also want to update the example in the man pages? ;-)

Would function names of "fatal_en()" and "fatal()" as defined below
work with you?

static inline void __attribute__((noreturn))
fatal_en(char const * const msg, int const err)
{
    fprintf(stderr, "%s: %s\n", msg, strerror(err));
    exit(EXIT_FAILURE);
}

static inline void __attribute__((noreturn))
fatal(char const * const msg)
{
    perror(msg);
    exit(EXIT_FAILURE);
}

      Thanks, Akira
> 
> --Elad
> 

Re: Section 4.2: wrong error reporting for pthread functions

[Elad Lahav]

Sure, that works for me ;-)

One more nitpick - on sub-section 4.2.7 there is an unmatched left
parenthesis before "and":

"POSIX supplies the pthread_key_create() function to create
a per-thread variable (and return the corresponding key, ..."

--Elad


On Mon, Jul 16, 2018 at 7:51 PM, Akira Yokosawa <akiyks@gmail.com> wrote:
> Hi Elad,
>
> On 2018/07/17 1:35, Elad Lahav wrote:
>> On Mon, Jul 16, 2018 at 11:42 AM, Akira Yokosawa <akiyks@gmail.com> wrote:
>>> 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?
>>
>> You are, of course, free to do as you want with your book, but I would
>> advise against the proposal. Novice software developers will often
>> copy patterns from books, which means that the examples need to be
>> held to a higher standard. I do agree that error handling is not the
>> point of these examples, so you shouldn't spend too much time on it,
>> but at least at one point it should show the correct pattern. The rest
>> of the code can just have a "// Report error and exit" comment.
>> If you do want a helper, then a better solution would be:
>>
>> static inline void __attribute__((noreturn))
>> fatal(char const * const msg, int const err)
>> {
>>     fprintf(stderr, "%s: %s\n", msg, strerror(err));
>>     exit(EXIT_FAILURE);
>> }
>>
>> The name 'fatal' better conveys to the reader of the code the fact
>> that the call doesn't return. The snippet also demonstrates a more
>> modern approach to C code (use of inline functions, const, function
>> attributes).
>
> Thank you for your feedback!
>
> I agree with you that inline functions are better choice.
> You might also want to update the example in the man pages? ;-)
>
> Would function names of "fatal_en()" and "fatal()" as defined below
> work with you?
>
> static inline void __attribute__((noreturn))
> fatal_en(char const * const msg, int const err)
> {
>     fprintf(stderr, "%s: %s\n", msg, strerror(err));
>     exit(EXIT_FAILURE);
> }
>
> static inline void __attribute__((noreturn))
> fatal(char const * const msg)
> {
>     perror(msg);
>     exit(EXIT_FAILURE);
> }
>
>       Thanks, Akira
>>
>> --Elad
>>
>

Re: Section 4.2: wrong error reporting for pthread functions

[Paul E. McKenney]

On Tue, Jul 17, 2018 at 07:05:03AM -0400, Elad Lahav wrote:
> Sure, that works for me ;-)
> 
> One more nitpick - on sub-section 4.2.7 there is an unmatched left
> parenthesis before "and":
> 
> "POSIX supplies the pthread_key_create() function to create
> a per-thread variable (and return the corresponding key, ..."

Good eyes, but SeongJae Park beat you to this one:

97bae8658482 ("toolsoftrade: Close uncompleted parentheses")

Hmmm...  That was fixed in January 2017.  I suggest upgrading to the
most recent release (November 2017), which may be found here:

https://kernel.org/pub/linux/kernel/people/paulmck/perfbook/perfbook.html

Or, if you wish, build from the current git archive, which may be cloned
like this:

git clone git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/perfbook.git

							Thanx, Paul

> --Elad
> 
> 
> On Mon, Jul 16, 2018 at 7:51 PM, Akira Yokosawa <akiyks@gmail.com> wrote:
> > Hi Elad,
> >
> > On 2018/07/17 1:35, Elad Lahav wrote:
> >> On Mon, Jul 16, 2018 at 11:42 AM, Akira Yokosawa <akiyks@gmail.com> wrote:
> >>> 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?
> >>
> >> You are, of course, free to do as you want with your book, but I would
> >> advise against the proposal. Novice software developers will often
> >> copy patterns from books, which means that the examples need to be
> >> held to a higher standard. I do agree that error handling is not the
> >> point of these examples, so you shouldn't spend too much time on it,
> >> but at least at one point it should show the correct pattern. The rest
> >> of the code can just have a "// Report error and exit" comment.
> >> If you do want a helper, then a better solution would be:
> >>
> >> static inline void __attribute__((noreturn))
> >> fatal(char const * const msg, int const err)
> >> {
> >>     fprintf(stderr, "%s: %s\n", msg, strerror(err));
> >>     exit(EXIT_FAILURE);
> >> }
> >>
> >> The name 'fatal' better conveys to the reader of the code the fact
> >> that the call doesn't return. The snippet also demonstrates a more
> >> modern approach to C code (use of inline functions, const, function
> >> attributes).
> >
> > Thank you for your feedback!
> >
> > I agree with you that inline functions are better choice.
> > You might also want to update the example in the man pages? ;-)
> >
> > Would function names of "fatal_en()" and "fatal()" as defined below
> > work with you?
> >
> > static inline void __attribute__((noreturn))
> > fatal_en(char const * const msg, int const err)
> > {
> >     fprintf(stderr, "%s: %s\n", msg, strerror(err));
> >     exit(EXIT_FAILURE);
> > }
> >
> > static inline void __attribute__((noreturn))
> > fatal(char const * const msg)
> > {
> >     perror(msg);
> >     exit(EXIT_FAILURE);
> > }
> >
> >       Thanks, Akira
> >>
> >> --Elad
> >>
> >
>