[PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[Jakub Kicinski]

ENOTSUPP often feels like the right error code to use, but it's
in fact not a standard Unix error. E.g.:

$ python
>>> import errno
>>> errno.errorcode[errno.ENOTSUPP]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'errno' has no attribute 'ENOTSUPP'

There were numerous commits converting the uses back to EOPNOTSUPP
but in some cases we are stuck with the high error code for backward
compatibility reasons.

Let's try prevent more ENOTSUPPs from getting into the kernel.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Hi Joe, I feel like I already talked to you about this, but I lost
my email archive, so appologies if you already said no.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eac40f0abd56..254877620bd8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4199,6 +4199,14 @@ sub process {
 			     "ENOSYS means 'invalid syscall nr' and nothing else\n" . $herecurr);
 		}
 
+# ENOTSUPP is not a standard error code and should be avoided.
+# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
+# Similarly to ENOSYS warning a small number of false positives is expected.
+		if ($line =~ /\bENOTSUPP\b/) {
+			WARN("ENOTSUPP",
+			     "ENOTSUPP is not a standard error code, prefer EOPNOTSUPP.\n" . $herecurr);
+		}
+
 # function brace can't be on same line, except for #defines of do while,
 # or if closed on same line
 		if ($perl_version_ok &&
-- 
2.25.4

Re: [PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[Andrew Lunn]

On Sun, May 10, 2020 at 11:51:48AM -0700, Jakub Kicinski wrote:
> ENOTSUPP often feels like the right error code to use, but it's
> in fact not a standard Unix error. E.g.:

Hi Jakub

You said ENOTSUPP is for NFS? Would it make sense to special case
fs/nfs* files and not warn there? I assume that would reduce the number
of false positives?

   Andrew

Re: [PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[Jakub Kicinski]

On Sun, 10 May 2020 21:04:32 +0200 Andrew Lunn wrote:
> On Sun, May 10, 2020 at 11:51:48AM -0700, Jakub Kicinski wrote:
> > ENOTSUPP often feels like the right error code to use, but it's
> > in fact not a standard Unix error. E.g.:  
> 
> Hi Jakub
> 
> You said ENOTSUPP is for NFS? Would it make sense to special case
> fs/nfs* files and not warn there? I assume that would reduce the number
> of false positives?

That's what Ben Hutchings once said, but I have no proof of that,
actually. The code pre-dates git.

I believe the real test would be "can this error code leak to user
space?" AFAIU those high error codes are for kernel's internal use.
So any code can use them, if done carefully.

Re: [PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[Joe Perches]

On Sun, 2020-05-10 at 11:51 -0700, Jakub Kicinski wrote:
> ENOTSUPP often feels like the right error code to use, but it's
> in fact not a standard Unix error. E.g.:

It is SUSv3 though.

> $ python
> > > > import errno
> > > > errno.errorcode[errno.ENOTSUPP]
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> AttributeError: module 'errno' has no attribute 'ENOTSUPP'
> 
> There were numerous commits converting the uses back to EOPNOTSUPP
> but in some cases we are stuck with the high error code for backward
> compatibility reasons.
> 
> Let's try prevent more ENOTSUPPs from getting into the kernel.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Hi Joe, I feel like I already talked to you about this, but I lost
> my email archive, so appologies if you already said no.

Not so far as I can tell.

This seems OK to me, but using checkpatch -f should probably
not show this error.

You might include a link to where Andrew Lunn suggested it
in the commit message.  I didn't find it with a trivial search.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4199,6 +4199,14 @@ sub process {
>  			     "ENOSYS means 'invalid syscall nr' and nothing else\n" . $herecurr);
>  		}
>  
> +# ENOTSUPP is not a standard error code and should be avoided.
> +# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
> +# Similarly to ENOSYS warning a small number of false positives is expected.
> +		if ($line =~ /\bENOTSUPP\b/) {

So to avoid having newbies and others trying to convert
existing uses in files using checkpatch.pl -f, maybe:
---
 scripts/checkpatch.pl | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e5b6b9aa21d6..f1bc81d4d97c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4244,6 +4244,17 @@ sub process {
 			     "ENOSYS means 'invalid syscall nr' and nothing else\n" . $herecurr);
 		}
 
+# ENOTSUPP is not a standard error code and should be avoided in new patches.
+# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
+# Similarly to ENOSYS warning a small number of false positives is expected.
+		if (~$file && $line =~ /\bENOTSUPP\b/) {
+			if (WARN("ENOTSUPP",
+				 "ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/\bENOTSUPP\b/EOPNOTSUPP/;
+			}
+		}
+
 # function brace can't be on same line, except for #defines of do while,
 # or if closed on same line
 		if ($perl_version_ok &&

Re: [PATCH net-next] checkpatch: warn about uses of ENOTSUPP

[Jakub Kicinski]

On Sun, 10 May 2020 12:56:53 -0700 Joe Perches wrote:
> On Sun, 2020-05-10 at 11:51 -0700, Jakub Kicinski wrote:
> > Hi Joe, I feel like I already talked to you about this, but I lost
> > my email archive, so appologies if you already said no.  
> 
> Not so far as I can tell.
> 
> This seems OK to me, but using checkpatch -f should probably
> not show this error.
> 
> You might include a link to where Andrew Lunn suggested it
> in the commit message.  I didn't find it with a trivial search.

Will do.

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl  
> []
> > @@ -4199,6 +4199,14 @@ sub process {
> >  			     "ENOSYS means 'invalid syscall nr' and nothing else\n" . $herecurr);
> >  		}
> >  
> > +# ENOTSUPP is not a standard error code and should be avoided.
> > +# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
> > +# Similarly to ENOSYS warning a small number of false positives is expected.
> > +		if ($line =~ /\bENOTSUPP\b/) {  
> 
> So to avoid having newbies and others trying to convert
> existing uses in files using checkpatch.pl -f, maybe:

Sounds good, thanks!