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

[Frederic Weisbecker]

On Fri, May 08, 2009 at 07:16:08PM +0800, 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")
> >>
> >> 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. :(


Well, I also find hard to write straightforward and good matching
impact lines.
And I'm certainly not well suited to give any advices about how
to write good impact lines.

But IMHO you can sum up your above impact line.

[ Impact: prevent spurious events enabling in tracing selftests ]

They usually don't need more details, those details can be placed in
the changelog. It's more about the general pratical impact, not a
detailed one.

Frederic.


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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/