On Mon, 12 Oct 2020, Ujjwal Kumar wrote:

On 12/10/20 11:47 am, Joe Perches wrote:

quoted

On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:

quoted

checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
files. The script leverages filename extensions and its path in
the repository to decide whether to allow execute permissions on
the file or not.

Based on current check conditions, a perl script file having
execute permissions, without '.pl' extension in its filename
and not belonging to 'scripts/' directory is reported as ERROR
which is a false-positive.

Adding a shebang check along with current conditions will make
the check more generalised and improve checkpatch reports.
To do so, without breaking the core design decision of checkpatch,
we can fetch the first line from the patch itself and match it for
a shebang pattern.

There can be cases where the first line is not part of the patch.

For instance: a patch that only changes permissions
without changing any of the file content.

quoted

In that case there may be a false-positive report but in the end we
will have less false-positives as we will be handling some of the
unhandled cases.

quoted

Signed-off-by: Ujjwal Kumar <redacted>
---
Apologies, I forgot to include linux-kernel@vger.kernel.org so I'm
now resending.

 scripts/checkpatch.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

quoted

@@ -1795,6 +1795,23 @@ sub get_stat_here {
 	return $herectx;
 }

First some style trivia:

quoted

+sub get_shebang {
+	my ($linenr, $realfile) = @_;
+	my $rawline = "";
+	my $shebang = "";
+
+	$rawline = raw_line($linenr, 3);
+	if (defined $rawline &&
+		$rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {

alignment to open parenthesis please

quoted

+		if (defined $1 && $1 == 1) {
+			$shebang = raw_line($linenr, 4);
+			$shebang = substr $shebang, 1;

parentheses around substr please.

quoted

+		}
+	}
+
+	return $shebang;
+}

And some real notes:

$realfile isn't used in this function so there doesn't
seem to be a reason to have it as an function argument.

quoted

+
 sub cat_vet {
 	my ($vet) = @_;
 	my ($res, $coded);

@@ -2680,7 +2697,9 @@ sub process {
 # Check for incorrect file permissions
 		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {

probably better here to use a capture group for the permissions

		if ($line =~ /^new (?:file )?mode (\d+)$/) {
			my $mode = substr($1, -3);

This

quoted

quoted

 			my $permhere = $here . "FILE: $realfile\n";
+			my $shebang = get_shebang($linenr, $realfile);
 			if ($realfile !~ m@scripts/@ &&

Maybe remove the $realfile directory test as
there are many source files that are not scripts
in this directory and its subdirectories.

this

quoted

quoted

+			    $shebang !~ /^#!\s*(\/\w)+.*/ &&

unnecessary capture group

and add

			   $mode =~ /[1357]/ &&

this

quoted

quoted

 			    $realfile !~ /\.(py|pl|awk|sh)$/) {

No need for a a capture group here either. (existing defect)

and this.

quoted

quoted

 				ERROR("EXECUTE_PERMISSIONS",
 				      "do not set execute permissions for source files\n" . $permhere);

Should these new changes go as a separate patch or can they be
included in the next iteration of this patch?

Ujjwal, please consider the following 'strategy':

- Send one patch to clean up the existing implementation as Joe requested.

With those 'credit points' for cleaning up the implementation, you then:

- Send another clean patch for the additional functionality you propose

We can probably easily accept the first cleanup, and then dig into the 
review of the additional functionality.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees