lkml.org 
[lkml]   [2009]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

On Fri, 8 May 2009, Li Zefan wrote:

> Frederic Weisbecker wrote:
> > On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
> >> Add a helper function __ftrace_set_clr_event(), and replace some
> >> ftrace_set_clr_event() calls with this helper, thus we don't need any
> >> kstrdup() or kmalloc().
> >>
> >> As a side effect, this patch fixes an issue in self tests code, which is
> >> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> >> ("tracing: append ":*" to internal setting of system events")

Actually, what does this patch solve that the above mentioned commit does
not?

> >>
> >> It's a small issue and won't cause any bug in fact, but we should do things
> >> right anyway.
> >>
> >> [ Impact: clean up ]
> >
> > If this fixes an issue like you described, then it's more than a cleanup :)
> >
>
> That issue causes no bug, and that's why I call it a cleanup.
>
> How about (mainly stealed from commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96):
>
> [ Impact: prevent accidental enabling of events with same name as a system in self tests ]
>
> But it excceeds 80 char..
>
> I sometimes feel it hard to write Impact line (one of the reason is my limit
> English skill). I've explained the impact of this patch in detail, but I'm
> still required to add a one-line summary. :(

I find the Impact line much easier to write when I think of it as the
reason for the patch (think, why am I writing this). Here I would have
written:

[ Impact: remove use of kmalloc and kstrdup with cleaner code ]

>
> >
> ...
> >> + if (event && strcmp(event, call->name) != 0)
> >> + continue;
> >
> >
> > Neat: You can simply use !strcmp(...)
> >
>
> Actually it's arguable which is better, and both styles are used in kernel code.
>
> And that 'if (!ptr)' vs 'if (ptr == NULL)'..
>

I preferer if (!ptr) over if (ptr == NULL), but this has nothing to do
with the reason for the "strcmp(...) != 0". It is totally different, and
it fooled you too ;-)
As I mentioned to Frederic, !strcmp() actually means "these strings
match". Which fools the human brain all too easy. Thus we see,
"strcmp(...) == 0" as "is a match" and "strcmp(...) != 0" as not a match,
because our brain focuses on the "==" and the "!=". Those that program in
C, default "!" as not.

Note, strcmp being 0 for match has nothing to do with the C convention of
0 being non error. But because it is used in sorting algorithms. strcmp
will return > 0 if it deems str1 > str2, or it will return < 0 if it deems
str1 < str2, and of course it returns 0 on match.
-- Steve



\
 
 \ /
  Last update: 2009-05-08 14:03    [W:0.079 / U:0.488 seconds]
©2003-2014 Jasper Spaans. Advertise on this site