[dpdk-dev,v4,11/11] devtools: prevent new instances of rte_panic and rte_exit

  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>
---
 devtools/checkpatches.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)
  

Arnon Warshavsky <arnon@qwilt.com> writes:

> 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>
> ---
>  devtools/checkpatches.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 7676a6b..fb37838 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -61,6 +61,90 @@ 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){
> +			print "Forbidden expressions in " substr(last_file,6) ":"
> +			for (key in expressions) {
> +				if (expressions[key] > 0) {
> +					print key
> +				}
> +			}
> +			exit 1
> +		}
> +	}
> +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\\\(" \

I don't think rte_panic should be considered forbidden.  Rather their
use should be flagged (as this patch does).  However, the 'exit 1'
(which will return a failure for the automatic checkpatch script bot)
might end up problematic as maintainers might consider it a patch that
is not ready.

I wouldn't object to this patch, but just think you might want to change
the print to something like:

  WARN: Are you sure you meant to use "$expression"?

> +		"$awk_script" -
> +}
> +
>  number=0
>  quiet=false
>  verbose=false
> @@ -89,11 +173,19 @@ check () { # <patch> <commit> <title>
>  	total=$(($total + 1))
>  	! $verbose || printf '\n### %s\n\n' "$3"
>  	if [ -n "$1" ] ; then
> +		cat "$1" | check_forbidden_additions
> +		[ $? -eq 0 ] || return 0
>  		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
>  	elif [ -n "$2" ] ; then
> -		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> +		params=$(echo "--find-renames --no-stat --stdout -1")
> +		body=$(git format-patch $params $commit)
> +		echo "$body" | check_forbidden_additions
> +		[ $? -eq 0 ] || return 0
> +		report=$(echo "$body" |
>  			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>  	else
> +		check_forbidden_additions -
> +		[ $? -eq 0 ] || return 0
>  		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>  	fi
>  	[ $? -ne 0 ] || return 0
  

>
> I don't think rte_panic should be considered forbidden.  Rather their
> use should be flagged (as this patch does).  However, the 'exit 1'
> (which will return a failure for the automatic checkpatch script bot)
> might end up problematic as maintainers might consider it a patch that
> is not ready.
>
> I wouldn't object to this patch, but just think you might want to change
> the print to something like:
>
>   WARN: Are you sure you meant to use "$expression"?
>

I can definitely change the warning text , but I think the whole point is
actually to prevent future panic calls.
If it were a recommendation, one can never converge with the attempt to get
rid of them, as not throwing a panic is a lot harder.

Thanks
Arnon
  

On 20-Apr-18 3:01 PM, Arnon Warshavsky wrote:
> 
> 
>     I don't think rte_panic should be considered forbidden.  Rather their
>     use should be flagged (as this patch does).  However, the 'exit 1'
>     (which will return a failure for the automatic checkpatch script bot)
>     might end up problematic as maintainers might consider it a patch that
>     is not ready.
> 
>     I wouldn't object to this patch, but just think you might want to change
>     the print to something like:
> 
>        WARN: Are you sure you meant to use "$expression"?
> 
> 
> I can definitely change the warning text , but I think the whole point 
> is actually to prevent future panic calls.
> If it were a recommendation, one can never converge with the attempt to 
> get rid of them, as not throwing a panic is a lot harder.
> 
> Thanks
> Arnon
> 

Warning is enough to get it automatically flagged on review. Final 
decision is up to maintainers/committers as to whether accept such code.