linux-kernel - Re: checkkpatch (in)sanity ?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160828005636.GB19088@sasha-lappy>
Date:   Sat, 27 Aug 2016 21:06:13 -0400
From:   "Levin, Alexander" <alexander.levin@...izon.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Sasha Levin <levinsasha928@...il.com>,
        "Levin, Alexander" <alexander.levin@...izon.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "ksummit-discuss@...ts.linuxfoundation.org" 
        <ksummit-discuss@...ts.linuxfoundation.org>
Subject: Re: checkkpatch (in)sanity ?

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> >>    - Making checkpatch check for (some) of the stable kernel rules
> >>    (and possibly recommend adding the stable@ tag in certain cases?).
> >>      - Depends on: making checkpatch sane again.>This sounds interesting.  What do you mean by "sane"?
> 
> Sasha, can you expand your thoughts here please?

Sure. I have 2.5 concerns about the state of checkpatch:

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: 
> Most all of the trivial spacing stuff can easily be
> ignored either by a human determining what's important
> or by using command line options like --ignore=spacing

1.
This is the wrong default. By default checkpatch shouldn't be showing trivial
issues that encourage folks to try and work around them and as a result
produce worse code.

Look at the 80 character limit warning for example, what good does it do? It
encourages people to do even stupider things to work around it and results in
a bunch of "fix checkpatch warning" that touch existing code just to make the
result harder to read and make 'git blame' harder to work with.

By default you should only get the most critical warnings we have in the
kernel like missing S-O-B or corrupt patch.

2. A "who wrote these rules?": there seems to be a disconnect between the rules
checkpatch is trying to enforce and the accepted coding style enforced by
maintainers. 

Do a git-format-patch on all of the commits Linus authored in the past year or
two and see how many of them fail checkpatch (or do the same for any of the
commits that passed through and were accepted by the top maintainers),
according to checkpatch we need to make those guys stop touching the kernel.

3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
perl code that a fair amount of people don't know how to deal with. In 4.8 it's
6142 lines, making it the 124th largest source file in the kernel, well within
the top 1% of source files in the kernel.

This combination of size/language pushes people away from being involved in
what is supposed to be a central tool and gives them a reason to never use
it again after they see results they don't agree with (rather than fixing it).

-- 

Thanks,
Sasha