Messages in this thread | | | Date | Fri, 8 May 2009 14:05:31 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event() |
| |
* 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
| |