[Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

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 <ujjwalkumar0501@gmail.com>
---
Changes in v2:
  - Spelling correction and add example to commit
    message
  - Code style changes
  - Remove unncessary function argument
  - Use non-capturing group in regexp

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fab38b493cef..7ebbee9c3672 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1795,6 +1795,23 @@ sub get_stat_here {
 	return $herectx;
 }

+sub get_shebang {
+	my ($linenr) = @_;
+	my $rawline = "";
+	my $shebang = "";
+
+	$rawline = raw_line($linenr, 3);
+	if (defined($rawline) &&
+	    $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
+		if (defined($1) && $1 == 1) {
+			$shebang = raw_line($linenr, 4);
+			$shebang = substr($shebang, 1);
+		}
+	}
+
+	return $shebang;
+}
+
 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}$/) {
 			my $permhere = $here . "FILE: $realfile\n";
+			my $shebang = get_shebang($linenr);
 			if ($realfile !~ m@scripts/@ &&
+			    $shebang !~ /^#!\s*(?:\/\w)+.*/ &&
 			    $realfile !~ /\.(py|pl|awk|sh)$/) {
 				ERROR("EXECUTE_PERMISSIONS",
 				      "do not set execute permissions for source files\n" . $permhere);

base-commit: 148fdf990dee4efd23c1114811b205de9c966680
--
2.26.2

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 13/10/20 5:31 pm, 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 <ujjwalkumar0501@gmail.com>
> ---
> Changes in v2:
>   - Spelling correction and add example to commit
>     message
>   - Code style changes
>   - Remove unncessary function argument
>   - Use non-capturing group in regexp
> 
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fab38b493cef..7ebbee9c3672 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1795,6 +1795,23 @@ sub get_stat_here {
>  	return $herectx;
>  }
> 
> +sub get_shebang {
> +	my ($linenr) = @_;
> +	my $rawline = "";
> +	my $shebang = "";
> +
> +	$rawline = raw_line($linenr, 3);

I'm wondering if the range information can be at a
different offset from the 'new mode line'.

> +	if (defined($rawline) &&
> +	    $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
> +		if (defined($1) && $1 == 1) {
> +			$shebang = raw_line($linenr, 4);
> +			$shebang = substr($shebang, 1);
> +		}
> +	}
> +
> +	return $shebang;
> +}
> +
>  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}$/) {
>  			my $permhere = $here . "FILE: $realfile\n";
> +			my $shebang = get_shebang($linenr);
>  			if ($realfile !~ m@scripts/@ &&
> +			    $shebang !~ /^#!\s*(?:\/\w)+.*/ &&
>  			    $realfile !~ /\.(py|pl|awk|sh)$/) {

Consider the following case:
a python script file with '.py' filename extension but without
a shebang line. Would it be meaningful to allow execute permission
on such a file?

>  				ERROR("EXECUTE_PERMISSIONS",
>  				      "do not set execute permissions for source files\n" . $permhere);
> 
> base-commit: 148fdf990dee4efd23c1114811b205de9c966680
> --
> 2.26.2
> 

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Tue, 2020-10-13 at 17:43 +0530, Ujjwal Kumar wrote:
> Consider the following case:
> a python script file with '.py' filename extension but without
> a shebang line. Would it be meaningful to allow execute permission
> on such a file?

More the question I think is for a patch to
that file, how do you tell if it has one?


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Tue, 13 Oct 2020, 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.
>

I get the intent of your addition. However:

I would bet that you only find one or two in a million commits, that would 
actually benefit for this special check of a special case of a special 
rule...

So given the added complexity of yet another 19 lines in checkpatch with 
little benefit of lowering false positive reports, I would be against 
inclusion.

You can provide convincing arguments with an evaluation, where you show 
on how many commits this change would really make a difference...

It is probably better and simpler to just have a script checking for
execute bits on all files in the repository on linux-next (with a list of 
known intended executable files) and just report to you and then to the 
developers when a new file with unintentional execute bit appeared.

Keep up the good work. I just fear this patch is a dead end.

There is still a lot of other issues you can contribute to.

Just one bigger project example: Comparing clang-format suggestions on 
patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
the actual kernel style.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 07:46 +0200, Lukas Bulwahn wrote:
> Just one bigger project example: Comparing clang-format suggestions on 
> patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
> the actual kernel style.

Eek no.

Mindless use of either tool isn't a great thing.

Linux source code has generally be created with
human readability in mind by humans, not scripts.

Please don't try to replace human readable code
with mindless tools.

If there's something inappropriate in checkpatch,
please mention it.

There is a _lot_ of relatively inappropriate
output in how clang-format changes existing code
in the kernel.

Try it and look at the results.

Improving how .clang-format is created and its
mechanisms (for example: continually out of date
ForEachMacros lists) could be reasonably be done.


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 07:46 +0200, Lukas Bulwahn wrote:
> > Just one bigger project example: Comparing clang-format suggestions on 
> > patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
> > the actual kernel style.
> 
> Eek no.
> 
> Mindless use of either tool isn't a great thing.
>

I did not suggest applying the tool to the source code and just changing 
the code... that is not a good idea as it is not helping anyone and just 
causing churn and distraction.
  
> Linux source code has generally be created with
> human readability in mind by humans, not scripts.
> 
> Please don't try to replace human readable code
> with mindless tools.
>

The goal is to run both tools on the code base and with the comparison see
how both tools can be improved.

We basically assume that the code is in the style it is intended to be.
What does checkpatch.pl warn about and what does clang-format still warn 
about, which is generally accepted okay as style in the kernel?
 
Then, we can improve the checkpatch and clang-format rules.

> If there's something inappropriate in checkpatch,
> please mention it.
> 
> There is a _lot_ of relatively inappropriate
> output in how clang-format changes existing code
> in the kernel.
>

Agree.
 
> Try it and look at the results.
>

Agree, that was the proposal. Nothing else.
 
> Improving how .clang-format is created and its
> mechanisms (for example: continually out of date
> ForEachMacros lists) could be reasonably be done.
> 
>

And that is something I would hope that somebody looking at the results 
would spot and start improving.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> What does checkpatch.pl warn about and what does clang-format still warn 
> about, which is generally accepted okay as style in the kernel?

clang-format doesn't warn at all, it just reformats.

checkpatch using the --in-place can reformat a
patch or file with an explanation of each change
it makes.


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > What does checkpatch.pl warn about and what does clang-format still warn 
> > about, which is generally accepted okay as style in the kernel?
> 
> clang-format doesn't warn at all, it just reformats.
>

You can run clang-format with --dry-run and then it would just state the 
proposed changes, right?

I am not the clang-format expert, though.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> 
> On Tue, 13 Oct 2020, Joe Perches wrote:
> 
> > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > about, which is generally accepted okay as style in the kernel?
> > 
> > clang-format doesn't warn at all, it just reformats.
> > 
> You can run clang-format with --dry-run and then it would just state the 
> proposed changes, right?

clang-format through at least version 10 does not have
a --dry-run option.



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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > 
> > On Tue, 13 Oct 2020, Joe Perches wrote:
> > 
> > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > about, which is generally accepted okay as style in the kernel?
> > > 
> > > clang-format doesn't warn at all, it just reformats.
> > > 
> > You can run clang-format with --dry-run and then it would just state the 
> > proposed changes, right?
> 
> clang-format through at least version 10 does not have
> a --dry-run option.
> 
>

Just a quick check:

version 9 does not have the --dry-run option:

https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html

version 10 does:

https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html

and version 12 (under development) does keep it:

https://clang.llvm.org/docs/ClangFormat.html

I have not played around with that, though.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 08:47 +0200, Lukas Bulwahn wrote:
> 
> On Tue, 13 Oct 2020, Joe Perches wrote:
> 
> > On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > > On Tue, 13 Oct 2020, Joe Perches wrote:
> > > 
> > > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > > about, which is generally accepted okay as style in the kernel?
> > > > 
> > > > clang-format doesn't warn at all, it just reformats.
> > > > 
> > > You can run clang-format with --dry-run and then it would just state the 
> > > proposed changes, right?
> > 
> > clang-format through at least version 10 does not have
> > a --dry-run option.
> > 
> > 
> 
> Just a quick check:
> 
> version 9 does not have the --dry-run option:
> 
> https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html
> 
> version 10 does:
> 
> https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html

Perhaps some version 10 variants do, but 10.0.0 does not.

$ which clang-format
/usr/local/bin/clang-format
$ clang-format --version
clang-format version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)
$ clang-format --dry-run
clang-format: Unknown command line argument '--dry-run'.  Try: 'clang-format --help'
clang-format: Did you mean '  --debug'?



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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:47 +0200, Lukas Bulwahn wrote:
> > 
> > On Tue, 13 Oct 2020, Joe Perches wrote:
> > 
> > > On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > > > On Tue, 13 Oct 2020, Joe Perches wrote:
> > > > 
> > > > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > > > about, which is generally accepted okay as style in the kernel?
> > > > > 
> > > > > clang-format doesn't warn at all, it just reformats.
> > > > > 
> > > > You can run clang-format with --dry-run and then it would just state the 
> > > > proposed changes, right?
> > > 
> > > clang-format through at least version 10 does not have
> > > a --dry-run option.
> > > 
> > > 
> > 
> > Just a quick check:
> > 
> > version 9 does not have the --dry-run option:
> > 
> > https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html
> > 
> > version 10 does:
> > 
> > https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html
> 
> Perhaps some version 10 variants do, but 10.0.0 does not.
> 
> $ which clang-format
> /usr/local/bin/clang-format
> $ clang-format --version
> clang-format version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)
> $ clang-format --dry-run
> clang-format: Unknown command line argument '--dry-run'.  Try: 'clang-format --help'
> clang-format: Did you mean '  --debug'?
>

Hmm... either the documentation is wrong; or the clang-format version 
10.0.0 you are was an early version 10 during development before the 
release and did not have that feature yet? 

$ clang-format-10 --version
Ubuntu clang-format version 
10.0.1-++20200928083909+ef32c611aa2-1~exp1~20200928185400.194

$ clang-format-10 --help | grep 'dry-run'
  --dry-run                  - If set, do not actually make the formatting 
changes
  --ferror-limit=<uint>      - Set the maximum number of clang-format
    errors to emit before stopping (0 = no limit). Used only with --dry-run or -n
  -n                         - Alias for --dry-run


You have probably seen that clang/llvm-11 was released; I guess a good 
motivation for us to update our clang setup? :)

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 09:17 +0200, Lukas Bulwahn wrote:
> $ clang-format-10 --version
> Ubuntu clang-format version 
> 10.0.1-++20200928083909+ef32c611aa2-1~exp1~20200928185400.194
> 
> $ clang-format-10 --help | grep 'dry-run'
>   --dry-run                  - If set, do not actually make the formatting 
> changes
>   --ferror-limit=<uint>      - Set the maximum number of clang-format
>     errors to emit before stopping (0 = no limit). Used only with --dry-run or -n
>   -n                         - Alias for --dry-run

OK, maybe so.

However I think the clang-format --dry-run output doesn't
currently contain particularly useful information.  Maybe
later versions are better than version 10.

For instance:

$ /usr/bin/clang-format-10 --version
clang-format version 10.0.0-4ubuntu1 

$ /usr/bin/clang-format-10 --dry-run drivers/net/ethernet/intel/igb/igb_main.c 2>&1 | head -25
drivers/net/ethernet/intel/igb/igb_main.c:54:40: warning: code should be clang-formatted [-Wclang-format-violations]
static const char igb_driver_string[] =
                                       ^
drivers/net/ethernet/intel/igb/igb_main.c:56:36: warning: code should be clang-formatted [-Wclang-format-violations]
static const char igb_copyright[] =
                                   ^
drivers/net/ethernet/intel/igb/igb_main.c:100:3: warning: code should be clang-formatted [-Wclang-format-violations]
        {0, }
         ^
drivers/net/ethernet/intel/igb/igb_main.c:100:5: warning: code should be clang-formatted [-Wclang-format-violations]
        {0, }
           ^
drivers/net/ethernet/intel/igb/igb_main.c:163:58: warning: code should be clang-formatted [-Wclang-format-violations]
static int igb_ndo_set_vf_vlan(struct net_device *netdev,
                                                         ^
drivers/net/ethernet/intel/igb/igb_main.c:164:28: warning: code should be clang-formatted [-Wclang-format-violations]
                               int vf, u16 vlan, u8 qos, __be16 vlan_proto);
                                                ^
drivers/net/ethernet/intel/igb/igb_main.c:189:50: warning: code should be clang-formatted [-Wclang-format-violations]
        SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
                                                        ^
drivers/net/ethernet/intel/igb/igb_main.c:190:21: warning: code should be clang-formatted [-Wclang-format-violations]
        SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
                           ^
drivers/net/ethernet/intel/igb/igb_main.c:190:61: warning: code should be clang-formatted [-Wclang-format-violations]

vs

$ ./scripts/checkpatch.pl -f drivers/net/ethernet/intel/igb/igb_main.c 2>&1 | head -25
WARNING: externs should be avoided in .c files
#113: FILE: drivers/net/ethernet/intel/igb/igb_main.c:113:
+int igb_open(struct net_device *);

WARNING: function definition argument 'struct net_device *' should also have an identifier name
#113: FILE: drivers/net/ethernet/intel/igb/igb_main.c:113:
+int igb_open(struct net_device *);

WARNING: externs should be avoided in .c files
#114: FILE: drivers/net/ethernet/intel/igb/igb_main.c:114:
+int igb_close(struct net_device *);

WARNING: function definition argument 'struct net_device *' should also have an identifier name
#114: FILE: drivers/net/ethernet/intel/igb/igb_main.c:114:
+int igb_close(struct net_device *);

CHECK: Alignment should match open parenthesis
#191: FILE: drivers/net/ethernet/intel/igb/igb_main.c:191:
+	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
+			igb_runtime_idle)

CHECK: Please use a blank line after function/struct/union/enum declarations
#193: FILE: drivers/net/ethernet/intel/igb/igb_main.c:193:
+};
+static void igb_shutdown(struct pci_dev *);


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 14/10/20 11:16 am, Lukas Bulwahn wrote:
> 
> 
> On Tue, 13 Oct 2020, 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.
>>
> 
> I get the intent of your addition. However:
> 
> I would bet that you only find one or two in a million commits, that would 
> actually benefit for this special check of a special case of a special 
> rule...
> 
> So given the added complexity of yet another 19 lines in checkpatch with 
> little benefit of lowering false positive reports, I would be against 
> inclusion.

Yes, it is a subtle change.

> 
> You can provide convincing arguments with an evaluation, where you show 
> on how many commits this change would really make a difference...

Some statistics:

I aggregated commits which involved 'mode change' on script files.
Totaling to 478 (looked for logs of only executable files in the repo).

At current state,
checkpatch reports 26 ERRORS (false positives)
with 'hashbang' test we have 5 false positives.

Without 'scripts/' directory test, 
checkpatch reports 82 ERRORS (false positives)
with 'hashbang' test we have 35 false positives.

> 
> It is probably better and simpler to just have a script checking for
> execute bits on all files in the repository on linux-next (with a list of 
> known intended executable files) and just report to you and then to the 
> developers when a new file with unintentional execute bit appeared.
> 
> Keep up the good work. I just fear this patch is a dead end.
> 
> There is still a lot of other issues you can contribute to.
> 
> Just one bigger project example: Comparing clang-format suggestions on 
> patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
> the actual kernel style.
> 
> Lukas
> 

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Miguel Ojeda]

On Wed, Oct 14, 2020 at 10:40 AM Joe Perches <joe@perches.com> wrote:
>
> Eek no.
>
> Mindless use of either tool isn't a great thing.

That is up to opinion. I (and others) definitely want to get to the
point the kernel sources are automatically formatted, because it has
significant advantages. The biggest is that it saves a lot of time for
everyone involved.

> Linux source code has generally be created with
> human readability in mind by humans, not scripts.

Code readability is by definition for humans, and any code formatter
(like clang-format) is written for them.

> Please don't try to replace human readable code
> with mindless tools.

Please do :-)

> There is a _lot_ of relatively inappropriate
> output in how clang-format changes existing code
> in the kernel.

Sure, but note that:

  - Code that should be specially-formatted should be in a
clang-format-off section to begin with, so it doesn't count.

  - Some people like to tweak identifiers or refactor code to make it
fit in the line limit beautifully. If you are doing that, then you
should do that for clang-format too. It is not fair to compare both
outputs when the tool is restricted on the transformations it can
perform. You can help the tool, too, the same way you can help
yourself.

  - Some style differences may be worth ignoring if the advantage is
getting automatic formatting.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 19:45 +0200, Miguel Ojeda wrote:
>   - Code that should be specially-formatted should be in a
> clang-format-off section to begin with, so it doesn't count.

clang-format is not the end-all tool.
Any 'formatting off/on' marker should be tool agnostic.


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Miguel Ojeda]

On Wed, Oct 14, 2020 at 8:05 PM Joe Perches <joe@perches.com> wrote:
>
> Any 'formatting off/on' marker should be tool agnostic.

Agreed, they should have used a compiler-agnostic name for the marker.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Joe Perches]

On Wed, 2020-10-14 at 20:32 +0200, Miguel Ojeda wrote:
> On Wed, Oct 14, 2020 at 8:05 PM Joe Perches <joe@perches.com> wrote:
> > Any 'formatting off/on' marker should be tool agnostic.
> 
> Agreed, they should have used a compiler-agnostic name for the marker.

It means to me that linux has to invent one and any
clang-format use has to use an equivalent of the sphinx
.rst macro conversion scripts pre and post format.


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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 14/10/20 11:16 am, Lukas Bulwahn wrote:
> 
> 
> On Tue, 13 Oct 2020, 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.
>>
> 
> I get the intent of your addition. However:
> 
> I would bet that you only find one or two in a million commits, that would 
> actually benefit for this special check of a special case of a special 
> rule...
> 
> So given the added complexity of yet another 19 lines in checkpatch with 
> little benefit of lowering false positive reports, I would be against 
> inclusion.
> 
> You can provide convincing arguments with an evaluation, where you show 
> on how many commits this change would really make a difference...
> 
> It is probably better and simpler to just have a script checking for
> execute bits on all files in the repository on linux-next (with a list of 
> known intended executable files) and just report to you and then to the 
> developers when a new file with unintentional execute bit appeared.
> 
> Keep up the good work. I just fear this patch is a dead end.
> 
> There is still a lot of other issues you can contribute to.

Lukas, Ayush didn't follow up with next iteration of patches for 
GIT_COMMIT_ID related issues. And I had discovered a bug in his 
implementation. Is it okay if I look into the issue?

> 
> Just one bigger project example: Comparing clang-format suggestions on 
> patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
> the actual kernel style.

I'm not familiar with how clang-format tool is used in the kernel 
repository. But I would like to know about it more and how I can 
contribute here. If there are any tasks that would help me get a 
direction please mention.

> 
> Lukas
> 

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

>
> Lukas, Ayush didn't follow up with next iteration of patches for
> GIT_COMMIT_ID related issues. And I had discovered a bug in his
> implementation. Is it okay if I look into the issue?
>

Dwaipayan already wanted to pick up the work on the GIT_COMMIT_ID issues.
So, he is working on that already.

> >
> > Just one bigger project example: Comparing clang-format suggestions on
> > patches against checkpatch.pl suggestions are fine-tuning both of them to fit to
> > the actual kernel style.
>
> I'm not familiar with how clang-format tool is used in the kernel
> repository. But I would like to know about it more and how I can
> contribute here. If there are any tasks that would help me get a
> direction please mention.
>

This is probably much more interesting to get involved in...

Some background information is in the kernel documentation; some
further information can be found on this presentation,
https://www.youtube.com/watch?v=FFjV9f_Ub9o, starting at 2:09.


You can start with running clang-format on the kernel sources just to
get a first feeling on what the tool does and where it completely
changes the code, and especially those cases where it changes code
that was perfectly okay before, we should do something to
adjust/configure clang-format.

Mid term goal: If we find a directory that needs no changes with
clang-format (after configuring), then it would be worth informing the
maintainers that clang-format works for them.

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Dwaipayan Ray]

On Fri, Oct 16, 2020 at 3:41 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> >
> > Lukas, Ayush didn't follow up with next iteration of patches for
> > GIT_COMMIT_ID related issues. And I had discovered a bug in his
> > implementation. Is it okay if I look into the issue?
> >
>
> Dwaipayan already wanted to pick up the work on the GIT_COMMIT_ID issues.
> So, he is working on that already.
>
> > >
> > > Just one bigger project example: Comparing clang-format suggestions on
> > > patches against checkpatch.pl suggestions are fine-tuning both of them to fit to
> > > the actual kernel style.
> >
> > I'm not familiar with how clang-format tool is used in the kernel
> > repository. But I would like to know about it more and how I can
> > contribute here. If there are any tasks that would help me get a
> > direction please mention.
> >
>
> This is probably much more interesting to get involved in...
>
> Some background information is in the kernel documentation; some
> further information can be found on this presentation,
> https://www.youtube.com/watch?v=FFjV9f_Ub9o, starting at 2:09.
>
>
> You can start with running clang-format on the kernel sources just to
> get a first feeling on what the tool does and where it completely
> changes the code, and especially those cases where it changes code
> that was perfectly okay before, we should do something to
> adjust/configure clang-format.
>
> Mid term goal: If we find a directory that needs no changes with
> clang-format (after configuring), then it would be worth informing the
> maintainers that clang-format works for them.
>
> Lukas

Hi Lukas, Hi Ujjwal,
This seems pretty interesting to me. I could help out with the
evaluation work as well considering the kernel source is very
vast.

So currently if we run both checkpatch and clang-format on the
kernel sources and notice substantial differences between the two,
which one is preferable? Or do we just go with intuition and take
whatever the kernel programming style generally suggests?

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

Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Lukas Bulwahn]

On Fri, Oct 16, 2020 at 12:24 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Fri, Oct 16, 2020 at 3:41 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> > >
> > > Lukas, Ayush didn't follow up with next iteration of patches for
> > > GIT_COMMIT_ID related issues. And I had discovered a bug in his
> > > implementation. Is it okay if I look into the issue?
> > >
> >
> > Dwaipayan already wanted to pick up the work on the GIT_COMMIT_ID issues.
> > So, he is working on that already.
> >
> > > >
> > > > Just one bigger project example: Comparing clang-format suggestions on
> > > > patches against checkpatch.pl suggestions are fine-tuning both of them to fit to
> > > > the actual kernel style.
> > >
> > > I'm not familiar with how clang-format tool is used in the kernel
> > > repository. But I would like to know about it more and how I can
> > > contribute here. If there are any tasks that would help me get a
> > > direction please mention.
> > >
> >
> > This is probably much more interesting to get involved in...
> >
> > Some background information is in the kernel documentation; some
> > further information can be found on this presentation,
> > https://www.youtube.com/watch?v=FFjV9f_Ub9o, starting at 2:09.
> >
> >
> > You can start with running clang-format on the kernel sources just to
> > get a first feeling on what the tool does and where it completely
> > changes the code, and especially those cases where it changes code
> > that was perfectly okay before, we should do something to
> > adjust/configure clang-format.
> >
> > Mid term goal: If we find a directory that needs no changes with
> > clang-format (after configuring), then it would be worth informing the
> > maintainers that clang-format works for them.
> >
> > Lukas
>
> Hi Lukas, Hi Ujjwal,
> This seems pretty interesting to me. I could help out with the
> evaluation work as well considering the kernel source is very
> vast.
>
> So currently if we run both checkpatch and clang-format on the
> kernel sources and notice substantial differences between the two,
> which one is preferable? Or do we just go with intuition and take
> whatever the kernel programming style generally suggests?
>

Basic assumption probably needs to be:

Both tools are wrong ;) and first intuition is maybe the best guess,
and still wrong ;) Only the maintainer really knows the preferred
style...

But when we start looking at it, we will adjust tools and ask
maintainers about their preferences and then we simply document those
preferences and create patches for kernel documentation. This
contributes directly to the maintainer entry profiles (something that
is still not very common, though...).

Getting a good overview on what both tools report will give some
interesting initial insights into this topic.

Let us get all the ongoing work on kernel patches done and then start
looking into this...

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