[dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Stephen Hemminger]

Since DPDK developers have decided to use a different tag format
than the kernel developers, ignore warnings about SPDX tags.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
IMHO would have been better to use the kernel SPDX style and
keep the check but that appears to be a minority opinion.

 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 7676a6b50141..b438fbcd6737 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -42,7 +42,7 @@ options="--no-tree"
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\
-FILE_PATH_CHANGES,MAINTAINERS_STYLE,\
+FILE_PATH_CHANGES,MAINTAINERS_STYLE,SPDX_LICENSE_TAG,\
 VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
 PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\
 SPLIT_STRING,LONG_LINE_STRING,\
-- 
2.17.0

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Thomas Monjalon]

17/04/2018 23:49, Stephen Hemminger:
> IMHO would have been better to use the kernel SPDX style and
> keep the check but that appears to be a minority opinion.

I think it is better to work on checkpatch itself.
When defining our SPDX style, Linux one was not definitive.
Do you think we can ask the Linux community to support our SPDX style?

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Scott Branden]

On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> 17/04/2018 23:49, Stephen Hemminger:
>> IMHO would have been better to use the kernel SPDX style and
>> keep the check but that appears to be a minority opinion.
> I think it is better to work on checkpatch itself.
> When defining our SPDX style, Linux one was not definitive.
> Do you think we can ask the Linux community to support our SPDX style?
>
I think it better to simply follow the Linux community defacto style 
rather than go your own way.

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Thomas Monjalon]

18/04/2018 00:11, Scott Branden:
> On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > 17/04/2018 23:49, Stephen Hemminger:
> >> IMHO would have been better to use the kernel SPDX style and
> >> keep the check but that appears to be a minority opinion.
> > 
> > I think it is better to work on checkpatch itself.
> > When defining our SPDX style, Linux one was not definitive.
> > Do you think we can ask the Linux community to support our SPDX style?
> >
> I think it better to simply follow the Linux community defacto style 
> rather than go your own way.

But our way is better! :)
And it has been decided in the Technical Board.

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Bruce Richardson]

On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
> 18/04/2018 00:11, Scott Branden:
> > On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > > 17/04/2018 23:49, Stephen Hemminger:
> > >> IMHO would have been better to use the kernel SPDX style and
> > >> keep the check but that appears to be a minority opinion.
> > > 
> > > I think it is better to work on checkpatch itself.
> > > When defining our SPDX style, Linux one was not definitive.
> > > Do you think we can ask the Linux community to support our SPDX style?
> > >
> > I think it better to simply follow the Linux community defacto style 
> > rather than go your own way.
> 
> But our way is better! :)
> And it has been decided in the Technical Board.
> 

As a general issue, I think we could do with having our own checkpatch-like
script for performing addition DPDK-specific code-checks *after* Linux
checkpatch ones. That is, reuse Linux check patch checks as much as
possible, but have other checks too.

For example, check for use of strcpy or strncpy (or snprintf with "%s") and
suggest replacing with strlcpy. If we did have our own extension script, we
could put our own SPDX format check there too.

Thoughts, or any volunteers to look into this?

/Bruce

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Kuusisaari, Juhamatti]

Hello,

> On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
> > 18/04/2018 00:11, Scott Branden:
> > > On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > > > 17/04/2018 23:49, Stephen Hemminger:
> > > >> IMHO would have been better to use the kernel SPDX style and keep
> > > >> the check but that appears to be a minority opinion.
> > > >
> > > > I think it is better to work on checkpatch itself.
> > > > When defining our SPDX style, Linux one was not definitive.
> > > > Do you think we can ask the Linux community to support our SPDX style?
> > > >
> > > I think it better to simply follow the Linux community defacto style
> > > rather than go your own way.
> >
> > But our way is better! :)
> > And it has been decided in the Technical Board.
> >
> 
> As a general issue, I think we could do with having our own checkpatch-like
> script for performing addition DPDK-specific code-checks *after* Linux
> checkpatch ones. That is, reuse Linux check patch checks as much as possible,
> but have other checks too.
> 
> For example, check for use of strcpy or strncpy (or snprintf with "%s") and
> suggest replacing with strlcpy. If we did have our own extension script, we
> could put our own SPDX format check there too.
> 
> Thoughts, or any volunteers to look into this?

In addition, the checkpatches.sh could be improved so that it actually checks that a proper file is found behind the selected env variable. I am planning to add this check (as it bite me just yesterday).

Speaking of strlcpy, I do think that it has a caveat* that everybody should be aware of: depending on implementation, it may read unintended memory regions when the source is not properly null terminated (like in Unix domain sockets, or just by other mistake). It may be a bad idea just blindly replace everything with strlcpy, without making sure that copied buffers are really null-terminated in the first place or making sure the strlcpy version is really a one that does not have this problem. As it depends on dynamic libraries, making sure may be difficult.
 
Some may argue that this is unlikely and thus irrelevant. Why do I know about it then? :) Needless to say, strncpy or snprintf do not have _this_ problem, although they have their own issues. Internally without dynamic libs DPDK rte_strlcpy uses snprintf which should be safe, though.

> /Bruce

--
 Juhamatti

 * A caveat on some implementations: 
 ...
        /* Not enough room in dst, add NUL and traverse rest of src */
        if (n == 0) {
                if (siz != 0)
                        *d = '\0';              /* NUL-terminate dst */
                while (*s++) <- what happens when s is not null-terminated?
                        ;
        }
...
  Another one:
...
    return n + strlen (src); <- what happens when src is not null-terminated?
...

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Bruce Richardson]

On Wed, Apr 18, 2018 at 10:49:07AM +0000, Kuusisaari, Juhamatti wrote:
> 
> Hello,
> 
> > On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
> > > 18/04/2018 00:11, Scott Branden:
> > > > On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > > > > 17/04/2018 23:49, Stephen Hemminger:
> > > > >> IMHO would have been better to use the kernel SPDX style and keep
> > > > >> the check but that appears to be a minority opinion.
> > > > >
> > > > > I think it is better to work on checkpatch itself.
> > > > > When defining our SPDX style, Linux one was not definitive.
> > > > > Do you think we can ask the Linux community to support our SPDX style?
> > > > >
> > > > I think it better to simply follow the Linux community defacto style
> > > > rather than go your own way.
> > >
> > > But our way is better! :)
> > > And it has been decided in the Technical Board.
> > >
> > 
> > As a general issue, I think we could do with having our own checkpatch-like
> > script for performing addition DPDK-specific code-checks *after* Linux
> > checkpatch ones. That is, reuse Linux check patch checks as much as possible,
> > but have other checks too.
> > 
> > For example, check for use of strcpy or strncpy (or snprintf with "%s") and
> > suggest replacing with strlcpy. If we did have our own extension script, we
> > could put our own SPDX format check there too.
> > 
> > Thoughts, or any volunteers to look into this?
> 
> In addition, the checkpatches.sh could be improved so that it actually checks that a proper file is found behind the selected env variable. I am planning to add this check (as it bite me just yesterday).
> 
> Speaking of strlcpy, I do think that it has a caveat* that everybody should be aware of: depending on implementation, it may read unintended memory regions when the source is not properly null terminated (like in Unix domain sockets, or just by other mistake). It may be a bad idea just blindly replace everything with strlcpy, without making sure that copied buffers are really null-terminated in the first place or making sure the strlcpy version is really a one that does not have this problem. As it depends on dynamic libraries, making sure may be difficult.
>  
> Some may argue that this is unlikely and thus irrelevant. Why do I know about it then? :) Needless to say, strncpy or snprintf do not have _this_ problem, although they have their own issues. Internally without dynamic libs DPDK rte_strlcpy uses snprintf which should be safe, though.
> 
> > /Bruce
> 
> --
>  Juhamatti
> 
>  * A caveat on some implementations: 
>  ...
>         /* Not enough room in dst, add NUL and traverse rest of src */
>         if (n == 0) {
>                 if (siz != 0)
>                         *d = '\0';              /* NUL-terminate dst */
>                 while (*s++) <- what happens when s is not null-terminated?
>                         ;
>         }
> ...
>   Another one:
> ...
>     return n + strlen (src); <- what happens when src is not null-terminated?
> ...

Thanks for pointing that out. It's good to be aware of these caveats. I
suspect in most cases the replacement is safe, but we should not blindly
replace one thing with another without checking for possible unintended
side effects.

/Bruce

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Thomas Monjalon]

18/04/2018 10:56, Bruce Richardson:
> On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
> > 18/04/2018 00:11, Scott Branden:
> > > On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > > > 17/04/2018 23:49, Stephen Hemminger:
> > > >> IMHO would have been better to use the kernel SPDX style and
> > > >> keep the check but that appears to be a minority opinion.
> > > > 
> > > > I think it is better to work on checkpatch itself.
> > > > When defining our SPDX style, Linux one was not definitive.
> > > > Do you think we can ask the Linux community to support our SPDX style?
> > > >
> > > I think it better to simply follow the Linux community defacto style 
> > > rather than go your own way.
> > 
> > But our way is better! :)
> > And it has been decided in the Technical Board.
> > 
> 
> As a general issue, I think we could do with having our own checkpatch-like
> script for performing addition DPDK-specific code-checks *after* Linux
> checkpatch ones. That is, reuse Linux check patch checks as much as
> possible, but have other checks too.

+1 to call more scripts in checkpatches.sh.
We need to find the right language to do code checks.
Coccinelle looks to be a good candidate for some checks.

> For example, check for use of strcpy or strncpy (or snprintf with "%s") and
> suggest replacing with strlcpy. If we did have our own extension script, we
> could put our own SPDX format check there too.
> 
> Thoughts, or any volunteers to look into this?

I am not volunteer to start the work but I would be glad to contribute later.

Any motivated volunteer?

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Wiles, Keith]

> On Apr 18, 2018, at 8:50 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> 18/04/2018 10:56, Bruce Richardson:
>> On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
>>> 18/04/2018 00:11, Scott Branden:
>>>> On 18-04-17 03:06 PM, Thomas Monjalon wrote:
>>>>> 17/04/2018 23:49, Stephen Hemminger:
>>>>>> IMHO would have been better to use the kernel SPDX style and
>>>>>> keep the check but that appears to be a minority opinion.
>>>>> 
>>>>> I think it is better to work on checkpatch itself.
>>>>> When defining our SPDX style, Linux one was not definitive.
>>>>> Do you think we can ask the Linux community to support our SPDX style?
>>>>> 
>>>> I think it better to simply follow the Linux community defacto style 
>>>> rather than go your own way.
>>> 
>>> But our way is better! :)
>>> And it has been decided in the Technical Board.
>>> 
>> 
>> As a general issue, I think we could do with having our own checkpatch-like
>> script for performing addition DPDK-specific code-checks *after* Linux
>> checkpatch ones. That is, reuse Linux check patch checks as much as
>> possible, but have other checks too.

I too believe we need to support our own checkpatch to better detect and fix DPDK specific issues.

> 
> +1 to call more scripts in checkpatches.sh.
> We need to find the right language to do code checks.
> Coccinelle looks to be a good candidate for some checks.
> 
>> For example, check for use of strcpy or strncpy (or snprintf with "%s") and
>> suggest replacing with strlcpy. If we did have our own extension script, we
>> could put our own SPDX format check there too.
>> 
>> Thoughts, or any volunteers to look into this?
> 
> I am not volunteer to start the work but I would be glad to contribute later.
> 
> Any motivated volunteer?

Regards,
Keith

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Hemant Agrawal]

> 18/04/2018 10:56, Bruce Richardson:
> > On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote:
> > > 18/04/2018 00:11, Scott Branden:
> > > > On 18-04-17 03:06 PM, Thomas Monjalon wrote:
> > > > > 17/04/2018 23:49, Stephen Hemminger:
> > > > >> IMHO would have been better to use the kernel SPDX style and
> > > > >> keep the check but that appears to be a minority opinion.
> > > > >
> > > > > I think it is better to work on checkpatch itself.
> > > > > When defining our SPDX style, Linux one was not definitive.
> > > > > Do you think we can ask the Linux community to support our SPDX style?
> > > > >
> > > > I think it better to simply follow the Linux community defacto
> > > > style rather than go your own way.
> > >
> > > But our way is better! :)
> > > And it has been decided in the Technical Board.
> > >
> >
> > As a general issue, I think we could do with having our own
> > checkpatch-like script for performing addition DPDK-specific
> > code-checks *after* Linux checkpatch ones. That is, reuse Linux check
> > patch checks as much as possible, but have other checks too.
> 
> +1 to call more scripts in checkpatches.sh.
> We need to find the right language to do code checks.
> Coccinelle looks to be a good candidate for some checks.
> 
> > For example, check for use of strcpy or strncpy (or snprintf with
> > "%s") and suggest replacing with strlcpy. If we did have our own
> > extension script, we could put our own SPDX format check there too.
> >
> > Thoughts, or any volunteers to look into this?
> 
> I am not volunteer to start the work but I would be glad to contribute later.
> 
> Any motivated volunteer?
> 
[Hemant] yes, we need volunteer 😊 

In DPDK we have following requirements
1. Check the SPDX tag on the files.
2. Validate the valid license SPDX tag. (BSD-3 and Dual for all except kernel folder). 

Regards,
Hemant

Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format

[Thomas Monjalon]

17/04/2018 23:49, Stephen Hemminger:
> Since DPDK developers have decided to use a different tag format
> than the kernel developers, ignore warnings about SPDX tags.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Applied, thanks