linux-kernel - Re: [PATCH v2] checkpatch: warn about multi-line comments without an empty /* line

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 26 Sep 2023 13:39:34 -0700
From:   Joe Perches <joe@...ches.com>
To:     Petr Tesařík <petr@...arici.cz>
Cc:     Andy Whitcroft <apw@...onical.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>
Subject: Re: [PATCH v2] checkpatch: warn about multi-line comments without
 an empty /* line

On Tue, 2023-09-26 at 22:16 +0200, Petr Tesařík wrote:
> On Tue, 26 Sep 2023 12:56:33 -0700
> Joe Perches <joe@...ches.com> wrote:
> 
> > On Tue, 2023-09-26 at 21:24 +0200, Petr Tesarik wrote:
> > > According to Documentation/process/coding-style.rst, the preferred style
> > > for multi-line comments outside net/ and drivers/net/ is:
> > > 
> > > .. code-block:: c
> > > 
> > >         /*
> > >          * This is the preferred style for multi-line
> > >          * comments in the Linux kernel source code.
> > >          * Please use it consistently.
> > >          *
> > >          * Description:  A column of asterisks on the left side,
> > >          * with beginning and ending almost-blank lines.
> > >          */
> > > 
> > > Signed-off-by: Petr Tesarik <petr@...arici.cz>  
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl  
> > []
> > > @@ -4006,6 +4006,14 @@ sub process {
> > >  			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> > >  		}
> > >  
> > > +# Non-networking without an initial /*
> > > +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
> > > +		    $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ &&
> > > +		    $rawline =~ /^\+[ \t]*\*/) {
> > > +			WARN("BLOCK_COMMENT_STYLE",
> > > +			     "multi-line block comments should start with an empty /* line\n" . $hereprev);
> > > +		}
> > > +
> > >  # Block comments use * on subsequent lines
> > >  		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
> > >  		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*  
> > 
> > Still nack.  Too many existing instances.
> > 
> > $ git grep '/\*.*' -- '*.[ch]' | \
> >   grep -v '/\*.*\*/' | \
> >   grep -v -P "/\*\s*$" | \
> >   grep -v '/\*\*' | \
> >   grep -v "SPDX-License" | \
> >   grep -v -P '^drivers/net|^net/' | \
> >   wc -l
> > 51834
> 
> Um. Not everything that is currently found in the source tree would be
> accepted as new code by today's standards...
> 
> As it stands, the script checks the special case for net/ and
> drivers/net/ but does not help prevent unnecessary respins, like this
> one:
> 
> https://lore.kernel.org/linux-iommu/ZRMgObTMkfq8Bjbe@arm.com/
> 
> OTOH if we don't want to warn about multi-line comments, maybe we don't
> want to call it the preferred style, and the corresponding paragraph
> should be removed from coding-style.rst. Do you suggest I try that
> instead?

If you really want to bring it up as a coding style issue
go ahead, but consider that the link above is a 'nitpick'
and not an actual issue.

If you _really_ want, but I am not at all sure it's useful,
I suggest you change the message to a CHK so that it is only
emitted with --strict and not a WARN and only emit anything
when the thing being scanned is a patch and _not_ a file.

Something like:

# Non-networking without an initial /*
		if (!$file &&
		    $realfile !~ m@^(?:drivers/net/|net/)@ &&
		    $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ &&
		    $rawline =~ /^\+[ \t]*\*/) {
			CHK("BLOCK_COMMENT_STYLE",
			    "multi-line block comments should start with an empty /* line\n" . $hereprev);

But you still want to examine some of the false positives
this would create like:

/* ------------------------
 * block message
 * ------------------------ */

and

struct foo {
	int a;  /* some desriptor */
	int b;	/* some descriptor
		   on multiple lines */
};