[lkml]   [2017]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

On 27/11/17 11:57 AM, Joe Perches wrote:
> It may or not be correct.

It's absolutely not correct in that it either requires that a subsequent
KERN_CONT/pr_cont or a '\n' at the end and it has neither.

> Without inter-function call code flow analysis,
> it's not possible to be correct.

But how many cases actually have the pr_cont/KERN_cont called in
different functions? This appears to be exceedingly rare to me.

> If you can get the false positive & false negative
> rate higher, I'll listen.

The only two classes of false positives that you've pointed out or that
I'm aware of:

1) The case where call did not either end in a '\n' or have a
KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears)
that a warning is appropriate here and this is not a false positive
because it absolutely is incorrect one way or the other. Coccinnelle
will also suffer from this issue because it can no better decide whether
the developer intended for the next call to be a continuation or for a
'\n' to end the line.

2) Cases where the pr_cont/KERN_CONT is not in sufficient context for
the script to detect. These are impossible to fix (and it's likely also
impossible for Coccinelle to be 100% accurate here). However, I'd expect
these to be *very* rare and I'm only actually aware of one case where
this has actually happened (lib/locking-selftest.c:1189) and (mostly by
luck) my v2 patch does not flag this where Coccinelle did. Not to
mention that continuation usage is discouraged in new code so this
should be even rarer on the majority of what checkpatch is used for.

(also 3. would be the %pV case, but I've removed those in what could be
a v3 of the patch -- I'd also be happy to address other false positives
classes if I could find them)

False negatives are much harder to quantify or improve. But given that I
detect nearly 6000 errors in the existing kernel it can't be *that*
high. Also, these false negatives do nothing to negate the benefit of
having this functionality seeing the vast majority of developers are
doing simple things with pr_* and dev_*.

Coccinelle may very well be able to do better at false negatives. But in
this case, it would still be great to have both because checkpatch will
flag a significant subset of the errors much earlier in the development
cycle and save developers a bunch of time.

So, in my opinion, I think focusing too hard on the false negatives
deprives developers of what could be a useful check.

> I think the Coccinelle script has a better chance
> to be more correct.

And yet, you have not pointed out any false positives that my patch
gives which Coccinelle does/would not. It really feels to me like your
biases are guiding your decision here and you aren't really looking at
the results.

Another thought I've had is that the dev_ functions don't have any form
of continuation. So we could potentially limit checkpatch to looking for
those to avoid the issues with continuations. It's not high coverage but
at least a lot of the driver patches would be checked with no chance of
false positives. I think there would be value in doing that.


 \ /
  Last update: 2017-11-27 22:18    [W:0.130 / U:2.512 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site