checkpatch: don't complain about SPDX tag format

RFC Applied Stephen Hemminger 7 participants 11 messages Apr 17, 2018 – Jun 8, 2018

Share & tools

Stephen Hemminger

author

2018-04-17 21:49 UTC #0 lore raw

RFC 1/1 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 <[email protected]>

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

devtools/checkpatches.sh +1 -1

index 7676a6b50141..b438fbcd6737 100755
@@ -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,\

Signature · 1 line

2.17.0

Thomas Monjalon

2018-04-17 22:06 UTC #1 lore raw

17/04/2018 23:49, Stephen Hemminger:

Quoted 2 lines

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?

Scott Branden

2018-04-17 22:11 UTC #2 lore raw

Quoted 7 lines · Thomas Monjalon

17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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.

Thomas Monjalon

2018-04-17 22:19 UTC #3 lore raw

18/04/2018 00:11, Scott Branden:

Quoted 11 lines · Thomas Monjalon

Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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.

Bruce Richardson

2018-04-18 08:56 UTC #4 lore raw

Quoted 10 lines · Thomas Monjalon

18/04/2018 00:11, Scott Branden:
Quoted 9 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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?

Quoted 6 lines

Quoted 2 lines
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

Kuusisaari, Juhamatti

2018-04-18 10:49 UTC #5 lore raw

Hello,

Quoted 23 lines · Thomas Monjalon

Quoted 10 lines
18/04/2018 00:11, Scott Branden:
Quoted 9 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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.

Quoted 5 lines

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.

Quoted 1 line

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

Bruce Richardson

2018-04-18 13:28 UTC #6 lore raw

Quoted 52 lines · Juhamatti

Hello,
Quoted 23 lines · Thomas Monjalon
Quoted 10 lines
18/04/2018 00:11, Scott Branden:
Quoted 9 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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.
Quoted 1 line
/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?
                        ;
        }

Quoted 5 lines

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

Thomas Monjalon

2018-04-18 13:50 UTC #7 lore raw

In reply to Bruce Richardson

18/04/2018 10:56, Bruce Richardson:

Quoted 14 lines · Thomas Monjalon

Quoted 13 lines
18/04/2018 00:11, Scott Branden:
Quoted 11 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 5 lines

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?

Wiles, Keith

2018-04-18 15:25 UTC #8 lore raw

Quoted 27 lines · Thomas Monjalon

18/04/2018 10:56, Bruce Richardson:
Quoted 22 lines · Thomas Monjalon
Quoted 10 lines
18/04/2018 00:11, Scott Branden:
Quoted 9 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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.

Quoted 9 lines

+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.
Quoted 4 lines
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.

Quoted 5 lines

Quoted 1 line
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

Hemant Agrawal

2018-04-19 12:42 UTC #9 lore raw

In reply to Thomas Monjalon

Quoted 33 lines

18/04/2018 10:56, Bruce Richardson:
Quoted 14 lines · Thomas Monjalon
Quoted 13 lines
18/04/2018 00:11, Scott Branden:
Quoted 11 lines · Thomas Monjalon
Quoted 8 lines
17/04/2018 23:49, Stephen Hemminger:
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.

Quoted 6 lines

Quoted 1 line
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

Thomas Monjalon

Applied

2018-06-08 19:41 UTC #10 lore raw

In reply to Stephen Hemminger

17/04/2018 23:49, Stephen Hemminger:

Quoted 4 lines

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 <[email protected]>

Applied, thanks