[PATCH] checkpatch: validate MODULE_LICENSE content

[PATCH] checkpatch: validate MODULE_LICENSE content

[Bjorn Andersson]

There is a well defined list of expected values for MODULE_LICENSE so
warn the user upon usage of unknown values.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d124359..7087b28 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5354,6 +5354,22 @@ sub process {
 				}
 			}
 		}
+
+		if ($line =~ /MODULE_LICENSE\(($String)\)/) {
+			my $extracted_string = get_quoted_string($line, $rawline);
+			my $valid_licenses = qr{
+						GPL|
+						GPL\ v2|
+						GPL\ and\ additional\ rights|
+						Dual\ BSD/GPL|
+						Dual\ MIT/GPL|
+						Dual\ MPL/GPL|
+						Proprietary
+					}x;
+			if ($extracted_string !~ /^"(?:$valid_licenses)"$/x) {
+				WARN("MODULE_LICENSE", "unknown module license " . $extracted_string . "\n" . $herecurr);
+			}
+		}
 	}
 
 	# If we have no input at all, then there is nothing to report on
-- 
1.8.2.2

Re: [PATCH] checkpatch: validate MODULE_LICENSE content

[Joe Perches]

On Mon, 2015-04-06 at 11:43 -0700, Bjorn Andersson wrote:
> There is a well defined list of expected values for MODULE_LICENSE so
> warn the user upon usage of unknown values.

Hello Bjorn

A few nits:

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5354,6 +5354,22 @@ sub process {
>  				}
>  			}
>  		}
> +
> +		if ($line =~ /MODULE_LICENSE\(($String)\)/) {

As there are uses with spaces, this would be better as:

		if ($line =~ /\bMODULE_LICENSE\s*\(\s*($String)\s*\)/) {

> +			my $extracted_string = get_quoted_string($line, $rawline);
> +			my $valid_licenses = qr{
> +						GPL|
> +						GPL\ v2|
> +						GPL\ and\ additional\ rights|
> +						Dual\ BSD/GPL|
> +						Dual\ MIT/GPL|
> +						Dual\ MPL/GPL|
> +						Proprietary

Why add "Proprietary" ?

This is the list I get in the current tree:
(after collapsing spaces in a few places)

   5920 MODULE_LICENSE("GPL")
   1211 MODULE_LICENSE("GPL v2")
    187 MODULE_LICENSE("Dual BSD/GPL")
     37 MODULE_LICENSE("GPL and additional rights")
     26 MODULE_LICENSE("Dual MPL/GPL")
      1 MODULE_LICENSE("Dual MIT/GPL")
      1 MODULE_LICENSE("BSD")

> +					}x;
> +			if ($extracted_string !~ /^"(?:$valid_licenses)"$/x) {
> +				WARN("MODULE_LICENSE", "unknown module license " . $extracted_string . "\n" . $herecurr);

I'd write this on 2 lines in the same fashion as
all the other warnings:

				WARN("MODULE_LICENSE",
				     "unknown module license $extracted_string\n" . $herecurr);

Other than those minor issues, it seems sensible enough.

thanks,  Joe

[PATCH v2] checkpatch: validate MODULE_LICENSE content

[Bjorn Andersson]

There is a well defined list of expected values for MODULE_LICENSE so
warn the user upon usage of unknown values.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Thanks for the review Joe!

"Proprietary" is in the list because this is the full list of "valid" values
mentioned in include/linux/module.h. So it's the list of licenses valid for
submission, but rather the list of valid values.

Changes since v1:
- Fixed nits pointed out by Joe
- Added comment to clarify the purpose, as well as origin of the values

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9a8b2bd..ca10d79 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5431,6 +5431,24 @@ sub process {
 				}
 			}
 		}
+
+# validate content of MODULE_LICENSE against list from include/linux/module.h
+		if ($line =~ /\bMODULE_LICENSE\s*\(\s*($String)\s*\)/) {
+			my $extracted_string = get_quoted_string($line, $rawline);
+			my $valid_licenses = qr{
+						GPL|
+						GPL\ v2|
+						GPL\ and\ additional\ rights|
+						Dual\ BSD/GPL|
+						Dual\ MIT/GPL|
+						Dual\ MPL/GPL|
+						Proprietary
+					}x;
+			if ($extracted_string !~ /^"(?:$valid_licenses)"$/x) {
+				WARN("MODULE_LICENSE",
+				     "unknown module license " . $extracted_string . "\n" . $herecurr);
+			}
+		}
 	}
 
 	# If we have no input at all, then there is nothing to report on
-- 
1.8.2.2

Re: [PATCH v2] checkpatch: validate MODULE_LICENSE content

[Joe Perches]

On Tue, 2015-04-07 at 12:37 -0700, Bjorn Andersson wrote:
> There is a well defined list of expected values for MODULE_LICENSE so
> warn the user upon usage of unknown values.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Thanks for the review Joe!

No charge.

> "Proprietary" is in the list because this is the full list of "valid" values
> mentioned in include/linux/module.h. So it's the list of licenses valid for
> submission, but rather the list of valid values.

So the "Proprietary" entry is only for non kernel-tree uses.
I wonder if the kernel checkpatch script should account for that.

I've no strong opinion though if Andrew wants to accept this as-is.

cheers, Joe

> Changes since v1:
> - Fixed nits pointed out by Joe
> - Added comment to clarify the purpose, as well as origin of the values
> 
>  scripts/checkpatch.pl | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9a8b2bd..ca10d79 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5431,6 +5431,24 @@ sub process {
>  				}
>  			}
>  		}
> +
> +# validate content of MODULE_LICENSE against list from include/linux/module.h
> +		if ($line =~ /\bMODULE_LICENSE\s*\(\s*($String)\s*\)/) {
> +			my $extracted_string = get_quoted_string($line, $rawline);
> +			my $valid_licenses = qr{
> +						GPL|
> +						GPL\ v2|
> +						GPL\ and\ additional\ rights|
> +						Dual\ BSD/GPL|
> +						Dual\ MIT/GPL|
> +						Dual\ MPL/GPL|
> +						Proprietary
> +					}x;
> +			if ($extracted_string !~ /^"(?:$valid_licenses)"$/x) {
> +				WARN("MODULE_LICENSE",
> +				     "unknown module license " . $extracted_string . "\n" . $herecurr);
> +			}
> +		}
>  	}
>  
>  	# If we have no input at all, then there is nothing to report on

Re: [PATCH v2] checkpatch: validate MODULE_LICENSE content

[Bjorn Andersson]

On Tue 07 Apr 12:48 PDT 2015, Joe Perches wrote:

> On Tue, 2015-04-07 at 12:37 -0700, Bjorn Andersson wrote:
> > There is a well defined list of expected values for MODULE_LICENSE so
> > warn the user upon usage of unknown values.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> > 
> > Thanks for the review Joe!
> 
> No charge.
> 
> > "Proprietary" is in the list because this is the full list of "valid" values
> > mentioned in include/linux/module.h. So it's the list of licenses valid for
> > submission, but rather the list of valid values.
> 
> So the "Proprietary" entry is only for non kernel-tree uses.
> I wonder if the kernel checkpatch script should account for that.
> 

The way I read the comment in module.h is that if you're writing a
non-free kernel module then you should mark it with "Proprietary".

If people doing so are using checkpatch that's great, but it's just
included for completeness, not to argue about the concept of proprietary
kernel modules.

> I've no strong opinion though if Andrew wants to accept this as-is.
> 

Let me know and I'll drop it from the patch.

> cheers, Joe
> 

Thanks,
Bjorn

> > Changes since v1:
> > - Fixed nits pointed out by Joe
> > - Added comment to clarify the purpose, as well as origin of the values
> > 
> >  scripts/checkpatch.pl | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 9a8b2bd..ca10d79 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5431,6 +5431,24 @@ sub process {
> >  				}
> >  			}
> >  		}
> > +
> > +# validate content of MODULE_LICENSE against list from include/linux/module.h
> > +		if ($line =~ /\bMODULE_LICENSE\s*\(\s*($String)\s*\)/) {
> > +			my $extracted_string = get_quoted_string($line, $rawline);
> > +			my $valid_licenses = qr{
> > +						GPL|
> > +						GPL\ v2|
> > +						GPL\ and\ additional\ rights|
> > +						Dual\ BSD/GPL|
> > +						Dual\ MIT/GPL|
> > +						Dual\ MPL/GPL|
> > +						Proprietary
> > +					}x;
> > +			if ($extracted_string !~ /^"(?:$valid_licenses)"$/x) {
> > +				WARN("MODULE_LICENSE",
> > +				     "unknown module license " . $extracted_string . "\n" . $herecurr);
> > +			}
> > +		}
> >  	}
> >  
> >  	# If we have no input at all, then there is nothing to report on
> 
> 
> 

Re: [PATCH v2] checkpatch: validate MODULE_LICENSE content

[Andrew Morton]

On Tue, 7 Apr 2015 12:55:49 -0700 Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote:

> > I've no strong opinion though if Andrew wants to accept this as-is.
> > 
> 
> Let me know and I'll drop it from the patch.

I'd be inclined to keep it - if someone has used "Proprietary" then
they meant to do so, and they don't want checkpatch to keep telling them
they made a typo.

Re: [PATCH v2] checkpatch: validate MODULE_LICENSE content

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

Hi Joe and Andrew,

On 04/07/2015 10:16 PM, Andrew Morton wrote:
> On Tue, 7 Apr 2015 12:55:49 -0700 Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote:
> 
>>> I've no strong opinion though if Andrew wants to accept this as-is.
>>>
>>
>> Let me know and I'll drop it from the patch.
> 
> I'd be inclined to keep it - if someone has used "Proprietary" then
> they meant to do so, and they don't want checkpatch to keep telling them
> they made a typo.

Any update on this patch?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] checkpatch: validate MODULE_LICENSE content

[Joe Perches]

On Tue, 2015-06-09 at 14:11 +0200, Michal Simek wrote:
> Hi Joe and Andrew,
> 
> On 04/07/2015 10:16 PM, Andrew Morton wrote:
> > On Tue, 7 Apr 2015 12:55:49 -0700 Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote:
> > 
> >>> I've no strong opinion though if Andrew wants to accept this as-is.
> >>>
> >>
> >> Let me know and I'll drop it from the patch.
> > 
> > I'd be inclined to keep it - if someone has used "Proprietary" then
> > they meant to do so, and they don't want checkpatch to keep telling them
> > they made a typo.
> 
> Any update on this patch?

I forgot all about it after that discussion.
Please resend it so it's easier for Andrew to pick it up.

Thanks,