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

[Ingo Molnar <mingo@elte.hu>]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

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

Thanks, i used this minor variant of it:

[ Impact: prevent spurious event-enabling in tracing self-tests ]

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

Yeah. There's two ends of the spectrum. The too terse:

 [ Impact: fix ]

that one is unhelpful beacause it's largely meaningless.

The too verbose:

 [ Impact: fix crash in foo while bar was setting baz to more
           than alice when charlie saw the full moon, by setting
           blah to bleh ]

Which is unhelpful because long impact lines tend to include 
implementational details while the goal is a 'quick practical impact 
on system summary'.

Best one is to find some middle ground. In any case, dont worry 
about getting it wrong either, it's an iterative process.

	Ingo