[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