[v12] devtools: alert on new instances of rte_panic and rte_exit
Checks
Commit Message
This patch adds a new function that is called
per every checked patch,
and alerts for new instances of rte_panic/rte_exit.
The check excludes comments, and alerts in the case
of a positive balance between additions and removals.
Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
Tested-by: Kevin Traynor <ktraynor@redhat.com>
---
v11
- Use Neils tmpfile infrastructure to consume the patches
- Allow different calls to the awk script decide if they want
to fail the check or just warn
v12
- Restored Tested message
devtools/checkpatches.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)
Comments
Hi,
I am sorry, I always give low priority to tooling review.
I was going to apply this one, but I've seen some details to improve.
16/07/2018 14:44, Arnon Warshavsky:
> +check_forbidden_additions() { # <file>
This function looks to work with stdin, not a file.
Better to remove the comment about a <file>.
> + # ---------------------------------
A split line at the beginning is visually strange.
> + #This awk script receives a list of expressions to monitor
> + #and a list of folders to search these expressions in
Please insert a space after #
> + # - No search is done inside comments
> + # - Both additions and removals of the expressions are checked
> + # A positive balance of additions fails the check
> + read -d '' awk_script << 'EOF'
EOF must be quoted?
> + BEGIN{
Please insert a space before {
> + split(FOLDERS,deny_folders," ");
> + split(EXPRESSIONS,deny_expr," ");
> + in_file=0;
> + in_comment=0;
> + count=0;
> + comment_start="/*"
> + comment_end="*/"
> + }
> + # search for add/remove instances in current file
> + # state machine assumes the comments structure is enforced by
> + # checkpatches.pl
> + (in_file) {
> + # comment start
> + if (index($0,comment_start) > 0){
> + in_comment = 1
> + }
> + # non comment code
> + if (in_comment == 0) {
> + for (i in deny_expr) {
> + forbidden_added = "^\+.*" deny_expr[i];
> + forbidden_removed="^-.*" deny_expr[i];
> + current = expressions[deny_expr[i]]
> + if ($0 ~ forbidden_added) {
> + count = count + 1;
> + expressions[deny_expr[i]] = current + 1
> + }
> + if ($0 ~ forbidden_removed) {
> + count = count - 1;
> + expressions[deny_expr[i]] = current - 1
> + }
> + }
> + }
> +
This is the only blank line in the awk script.
Can be removed.
> + # comment end
> + if (index($0,comment_end) > 0) {
> + in_comment = 0
> + }
> + }
> + # switch to next file , check if the balance of add/remove
> + # of previous filehad new additions
> + ($0 ~ "^\+\+\+ b/") {
> + in_file = 0;
> + if (count > 0){
> + exit;
> + }
> + for (i in deny_folders){
> + re = "^\+\+\+ b/" deny_folders[i];
> + if ($0 ~ deny_folders[i]) {
> + in_file = 1
> + last_file = $0
> + }
> + }
> + }
> + END{
> + if (count > 0){
> + warnText = "\\033[1;31m" "Warning:" "\\033[0m"
Please no color in the warning.
It will look strange in a file.
> + print warnText " in " substr(last_file,6) ":"
> + print "are you sure you want to add the following:"
> + for (key in expressions) {
> + if (expressions[key] > 0) {
> + print key
> + }
> + }
> + exit RET_ON_FAIL
> + }
> + }
> +EOF
> + # ---------------------------------
> +
> + # refrain from new additions of rte_panic() and rte_exit()
> + # under lib and net
> + # multiple folders and expressions are separated by spaces
> + awk -v FOLDERS="lib net" \
Why not looking in drivers directory?
> + -v EXPRESSIONS="rte_panic\\\( rte_exit\\\(" \
> + -v RET_ON_FAIL=0 \
> + "$awk_script" -
> +}
> +
> number=0
> quiet=false
> verbose=false
> @@ -97,6 +185,13 @@ check () { # <patch> <commit> <title>
> ret=1
> fi
>
> + ! $verbose || printf '\nChecking forbidden tokens additions/removals:\n'
> + report=$(cat $tmpinput | check_forbidden_additions)
Another way of writing it without cat:
check_forbidden_additions <"tmpinput"
In any case, please quote "tmpinput" to prevent from space.
> + if [ $? -ne 0 ] ; then
> + ret=1
> + fi
> + printf '%s\n' "$report"
You are printing the report, no matter of the result? Why?
Is it because a warning does not return as an error?
There is maybe an improvement required here.
> > +check_forbidden_additions() { # <file>
>
> This function looks to work with stdin, not a file.
> Better to remove the comment about a <file>.
>
It can actually work with both but you are right. The comment is not
beneficial there
Will fix with the rest of the list below
...
>
> > + if [ $? -ne 0 ] ; then
> > + ret=1
> > + fi
> > + printf '%s\n' "$report"
>
> You are printing the report, no matter of the result? Why?
> Is it because a warning does not return as an error?
>
There is maybe an improvement required here.
>
Yes this is indeed the reason.
Sticking to a simple success/fail in the return value with the price of
printing an empty string in the case of no-warning seemed better than
handling multiple (>2) return codes .
Do you have a preferred way here?
26/07/2018 22:57, Arnon Warshavsky:
> > > + if [ $? -ne 0 ] ; then
> > > + ret=1
> > > + fi
> > > + printf '%s\n' "$report"
> >
> > You are printing the report, no matter of the result? Why?
> > Is it because a warning does not return as an error?
> >
> There is maybe an improvement required here.
> >
>
> Yes this is indeed the reason.
> Sticking to a simple success/fail in the return value with the price of
> printing an empty string in the case of no-warning seemed better than
> handling multiple (>2) return codes .
> Do you have a preferred way here?
My preference is to return an error if there is a warning.
The script itself must return an error code.
Thank you
> > + read -d '' awk_script << 'EOF'
>
> EOF must be quoted?
>
Missed that one. Yes, the quotes are there to prevent parameter expansion
of the awk variables.
The script breaks without them
> 16/07/2018 14:44, Arnon Warshavsky:
> > + ! $verbose || printf '\nChecking forbidden tokens additions/removals:\n'
> > + report=$(cat $tmpinput | check_forbidden_additions)
One more nit: the title says "additions/removals" but the function is
only about additions.
>
> One more nit: the title says "additions/removals" but the function is
> only about additions.
>
>
> Yup. No removals there..
@@ -43,6 +43,94 @@ print_usage () {
END_OF_HELP
}
+
+check_forbidden_additions() { # <file>
+ # ---------------------------------
+ #This awk script receives a list of expressions to monitor
+ #and a list of folders to search these expressions in
+ # - No search is done inside comments
+ # - Both additions and removals of the expressions are checked
+ # A positive balance of additions fails the check
+ read -d '' awk_script << 'EOF'
+ BEGIN{
+ split(FOLDERS,deny_folders," ");
+ split(EXPRESSIONS,deny_expr," ");
+ in_file=0;
+ in_comment=0;
+ count=0;
+ comment_start="/*"
+ comment_end="*/"
+ }
+ # search for add/remove instances in current file
+ # state machine assumes the comments structure is enforced by
+ # checkpatches.pl
+ (in_file) {
+ # comment start
+ if (index($0,comment_start) > 0){
+ in_comment = 1
+ }
+ # non comment code
+ if (in_comment == 0) {
+ for (i in deny_expr) {
+ forbidden_added = "^\+.*" deny_expr[i];
+ forbidden_removed="^-.*" deny_expr[i];
+ current = expressions[deny_expr[i]]
+ if ($0 ~ forbidden_added) {
+ count = count + 1;
+ expressions[deny_expr[i]] = current + 1
+ }
+ if ($0 ~ forbidden_removed) {
+ count = count - 1;
+ expressions[deny_expr[i]] = current - 1
+ }
+ }
+ }
+
+ # comment end
+ if (index($0,comment_end) > 0) {
+ in_comment = 0
+ }
+ }
+ # switch to next file , check if the balance of add/remove
+ # of previous filehad new additions
+ ($0 ~ "^\+\+\+ b/") {
+ in_file = 0;
+ if (count > 0){
+ exit;
+ }
+ for (i in deny_folders){
+ re = "^\+\+\+ b/" deny_folders[i];
+ if ($0 ~ deny_folders[i]) {
+ in_file = 1
+ last_file = $0
+ }
+ }
+ }
+ END{
+ if (count > 0){
+ warnText = "\\033[1;31m" "Warning:" "\\033[0m"
+ print warnText " in " substr(last_file,6) ":"
+ print "are you sure you want to add the following:"
+ for (key in expressions) {
+ if (expressions[key] > 0) {
+ print key
+ }
+ }
+ exit RET_ON_FAIL
+ }
+ }
+EOF
+ # ---------------------------------
+
+ # refrain from new additions of rte_panic() and rte_exit()
+ # under lib and net
+ # multiple folders and expressions are separated by spaces
+ awk -v FOLDERS="lib net" \
+ -v EXPRESSIONS="rte_panic\\\( rte_exit\\\(" \
+ -v RET_ON_FAIL=0 \
+ "$awk_script" -
+}
+
number=0
quiet=false
verbose=false
@@ -97,6 +185,13 @@ check () { # <patch> <commit> <title>
ret=1
fi
+ ! $verbose || printf '\nChecking forbidden tokens additions/removals:\n'
+ report=$(cat $tmpinput | check_forbidden_additions)
+ if [ $? -ne 0 ] ; then
+ ret=1
+ fi
+ printf '%s\n' "$report"
+
clean_tmp_files
[ $ret -eq 0 ] && return 0