Date:   Mon, 27 Nov 2017 21:49:09 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Logan Gunthorpe <logang@...tatee.com>
cc:     Joe Perches <joe@...ches.com>, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, Andy Whitcroft <apw@...onical.com>
Subject: Re: [PATCH v2] checkpatch: Add a warning for log messages that don't
 end in a new line



On Mon, 27 Nov 2017, Logan Gunthorpe wrote:

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

Perhaps if there is a possible flow from one print to another within a
single function and in both cases the format string is at least say 25
characters (completely random value), then it is pretty likely that a
newline is intended.

Alternatively, if the first format string doesn't end in a space and the
second one doesn't begin with a space, then a newline is also likely
intended.

julia

Powered by blists - more mailing lists