Re: Section 4.2: wrong error reporting for pthread functions

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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