linux-kernel - Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT

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]

Date:   Wed, 10 Jun 2020 14:42:51 -0700
From:   Joe Perches <joe@...ches.com>
To:     Scott Branden <scott.branden@...adcom.com>,
        Andy Whitcroft <apw@...onical.com>
Cc:     BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] checkpatch: add check for
 NONNETWORKING_BLOCK_COMMENT_STYLE

On Wed, 2020-06-10 at 14:33 -0700, Scott Branden wrote:
> Hi Joe,
> 
> On 2020-06-10 2:16 p.m., Joe Perches wrote:
> > On Wed, 2020-06-10 at 13:26 -0700, Scott Branden wrote:
> > > NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
> > > doesn't seem to be any check for the standard block comment style.
> > > Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
> > > on first line of non-networking block comments.
> > I think there are _way_ too many instances of this form
> > in non-networking code to enable this.
> > 
> > $ git grep -P '^\s*/\*\s*\S.*[^\*][^\\]\s*$' -- '*.[ch]' | \
> >    grep -v -P '^(drivers/net/|net/)' | \
> >    wc -l
> > 51407
> That is true about many things that checkpatch now checks for that 
> didn't previously.

Not in that quantity of uses it doesn't.

I specifically did _not_ add this very same test
when I added the other comment tests.

> But, by adding to checkpatch the coding style clearly outlined in 
> coding-style.rst can be followed:

Well, because there are _so_ many false positives
that don't need change, I'm not adding this.

As is, I'm nacking it.

If you need it for your use, you should keep it in
your own tree.

> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -3408,6 +3408,16 @@ sub process {
> > >   			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> > >   		}
> > >   
> > > +# Non-Networking with an empty initial /*
> > > +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
> > > +		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
> > > +		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
> > > +		    $rawline =~ /^\+[ \t]*\*/ &&
> > > +		    $realline > 2) {
> > > +			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
> > > +			     "non-networking block comments use an empty /* on first line\n" . $hereprev);

_Maybe_ this test _might_ be useful if it did a file/patch
test and used CHK on file, but even then I'm very dubious.

			my $msg_level = \&WARN;
			$msg_level = \&CHK if ($file);
			&{msg_level}(etc...)