Re: [dpdk-dev] [PATCH v12] devtools: alert on new instances of rte_panic and rte_exit

[Thomas Monjalon <thomas@monjalon.net>]

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.