Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Li Zefan <lizf@cn.fujitsu.com>]

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")
>>
>> 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. :(

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