On 12/10/20 11:47 am, Joe Perches wrote: > On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote: >> 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. > >> >> 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. > >> Signed-off-by: Ujjwal Kumar <ujjwalkumar0...@gmail.com> >> --- >> 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 > [] >> @@ -1795,6 +1795,23 @@ sub get_stat_here { >> return $herectx; >> } > > First some style trivia: > >> +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 > >> + if (defined $1 && $1 == 1) { >> + $shebang = raw_line($linenr, 4); >> + $shebang = substr $shebang, 1; > > parentheses around substr please. > >> + } >> + } >> + >> + 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. > >> + >> 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 > >> 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 > >> + $shebang !~ /^#!\s*(\/\w)+.*/ && > > unnecessary capture group > > and add > > $mode =~ /[1357]/ && this > >> $realfile !~ /\.(py|pl|awk|sh)$/) { > > No need for a a capture group here either. (existing defect) and this. > >> 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? Thanks Ujjwal Kumar