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

  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(-)
  

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?
  

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.
  

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.
  

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
  

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?
...
  

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
  

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?
  

> 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
  

> 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
  

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