linux-kernel - Re: [PATCH v1] checkpatch: test missing initial blank line in 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]

Message-ID: <654b3487-23a1-4766-4f42-73fa41fd7c9a@st.com>
Date:   Wed, 5 Apr 2017 13:26:48 +0000
From:   Hugues FRUCHET <hugues.fruchet@...com>
To:     Joe Perches <joe@...ches.com>, Andy Whitcroft <apw@...onical.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] checkpatch: test missing initial blank line in block
 comment



On 04/05/2017 11:55 AM, Joe Perches wrote:
> On Wed, 2017-04-05 at 09:43 +0000, Hugues FRUCHET wrote:
>>
>> On 04/05/2017 10:35 AM, Joe Perches wrote:
>>> On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
>>>> Hi Joe, thanks for reviewing,
>>>
>>> Hello Hugues
>>>
>>>> I have run the command you advice on the entire kernel code, modifying
>>>> the script to only match the newly introduced check case.
>>>> There was 14389 hits, quite huge, so I cannot 100% certify that there
>>>> are no false positives, but I have checked the output carefully and
>>>> found 2 limit cases:
>>>>
>>>> 1) space character placed just after "/*"
>>>> WARNING: Block comments starts with an empty /*
>>>> #330: FILE: arch/alpha/kernel/core_irongate.c:330:
>>>> +	/*
>>>> +	 * Check for within the AGP aperture...
>>>> => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
>>>>
>>>> 2) // style comment followed by pointer dereference
>>>> WARNING: Block comments starts with an empty /*
>>>> #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
>>>> +	// success
>>>> +	*tupleType = _tupleType;
>>>> => 4 hits
>>>>
>>>> Anyway this reveal comment style related issues, so I would say that we
>>>> can keep script as it is, what do you think about ?
>>>
>>> Glancing at the output, there is also the comment
>>> in a multiline macro case:
>>>
>>> WARNING: Block comments starts with an empty /*
>>> #354: FILE: arch/mips/include/asm/processor.h:354:
>>> +	/*							\
>>> +	 * Other stuff associated with the process		\
>>>
>>> Dunno how common that is, but maybe the test
>>> should be changed to avoid those.
>>>
>>
>> Here is a proposal that remove this macro case:Per
>>
>> # Missing initial /*
>> if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
>>      $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
>>      $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
>> +   $prevrawline !~ /^\+[ \t]\/\*+[ \t]+\\$/ &&#no macro /*<tab><\>
>>      $rawline =~ /^\+[ \t]*\*/ &&
>>      $realline > 2) {
>
> Perhaps it's better to change this to
>
> 	$prevrawline !~ /^\+\s*\/\*.*\\$/

KO with this line, I suspect you meant "\s" instead of "." in above 
expression, so I've changed to:
        $prevrawline !~ /^\+\s*\/\*\s*\\$/
this one is OK

>
> Also perhaps the
> 	// foo
> 	*bar = baz;
>
> case could be avoided by adding tests for the
> comment character $; on $prevline and $line
> and not looking only at $prevrawline and $rawline.
>

Sorry for my poor understanding of the script but I don't catch what you 
meant regarding "raw" and non "raw" variables, so I've done the job 
simply by excluding the lines starting with "//":
       $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&	#no inline //

Which gives finally:

# Missing initial /*
if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
   $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
   $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
+ $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&	#no inline //
+ $prevrawline !~ /^\+\s*\/\*\s*\\$/ &&	#no macro /*<whitespace><\>
   $rawline =~ /^\+[ \t]*\*/ &&
   $realline > 2) {
  	WARN("MISSING_INITIAL_BLOCK_COMMENT_STYLE",
  	     "Block comments starts with an empty /*\n" . $hereprev);
     }


BR,
Hugues.