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()

* 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


\
 
 \ /
  Last update: 2009-05-08 14:09    [W:0.097 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site