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@xxxxxxxxx> wrote:
> > Hi Elad,
> >
> > On 2018/07/17 1:35, Elad Lahav wrote:
> >> On Mon, Jul 16, 2018 at 11:42 AM, Akira Yokosawa <akiyks@xxxxxxxxx> 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
> >>
> >
>
--
To unsubscribe from this list: send the line "unsubscribe perfbook" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
