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: <1472404557.26978.84.camel@perches.com>
Date:   Sun, 28 Aug 2016 10:15:57 -0700
From:   Joe Perches <joe@...ches.com>
To:     "Levin, Alexander" <alexander.levin@...izon.com>
Cc:     Sasha Levin <levinsasha928@...il.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, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:

> Would you agree that by default we shouldn't show anything that's
> not an error/defect?

Not particularly, no.

> That doesn't deal with newlines people add to hide the 80 character stuff, nor it
> deals with the "harder to read" part.

Harder to read is almost all habituation.

80 columns is simply silly when dealing with either
long identifiers or many levels of indentation.

One thing that 80 column limit does do is encourage
shorter identifiers and fewer levels of indentation.

Generally, both of those are good things.

> > > By default you should only get the most critical warnings we have in the
> > > kernel like missing S-O-B or corrupt patch.
> > I don't think so, but if you do, add a filter for ERROR only.
> I could, but the problem is the people who see the default output as "holy".

Personally, I think the "my first kernel patch" beginners were
overly encouraged to produce these checkpatch whitespace type
changes by a couple things:

o Greg KH's TuxRadar article back in 2010
  http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
o The Eudyptula Challenge
  http://eudyptula-challenge.org/

I don't know if the Eudyptula scripts are specific to
drivers/staging and most of those beginners haven't read his
email from 2015 that essentially says "don't do that" on
anything other than drivers/staging.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-July/014699.html

Outreach is hard.  Those efforts were perhaps worthwhile, but
has even a single productive kernel developer been produced
from one of those two outreach efforts?

> > > 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. 
> > Name some please.
> Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID).
> checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

That bit is in Documentation/SubmttingPatches

12 was Linus' length after the original 7 was too short.
12 will still be long enough for a few years yet.
https://lkml.org/lkml/2010/10/28/287

> > I think almost all of it is regexes and most people
> > aren't very good at those.
> > 
> > So it wouldn't matter if it was perl or python.
> > 
> > spatch isn't the right tool.
> > 
> > What would you suggest instead?
> This is a good topic to talk about, making checkpatch accessible to us
> commoners could be useful, we just need to figure out how.

I'm not sure that matters much at all.

I'm sure if you tried, you could produce that
"ERRORS" only patch for checkpatch.

There would still be some issues categorizing the
various tests at the appropriate level to taste.