Re: [dpdk-dev] [PATCH v8] checkpatches.sh: Add checks for ABI symbol addition

[Thomas Monjalon <thomas@monjalon.net>]

14/06/2018 15:30, Neil Horman:
>  * found a way to eliminate the use of filterdiff (new awk rules)

Thanks a lot for not requiring filterdiff dependency.

[...]
> +				# Just inform the user of this occurrence, but
> +				# don't flag it as an error
> +				echo -n "INFO: symbol $syname is added but "
> +				echo -n "patch has insuficient context "
> +				echo -n "to determine the section name "
> +				echo -n "please ensure the version is "
> +				echo "EXPERIMENTAL"

For info, I think nowadays "printf" is preferred over "echo -n"
But if you prefer "echo -n" for any reason, no problem.

[...]
> +exit $exit_code
> +
> +

Ironically, this patch doesn't pass checkpatch test because of
the trailing new lines.

[...]
> +clean_tmp_files() {
> +	echo $TMPINPUT | grep -q checkpaches

Two comments here.

Since TMPINPUT is not supposed to be overwritten by environment,
I think it is better to make it lowercase (kind of convention).

What the grep is supposed to match?
(side note, there is a typo: checkpaches -> checkpatches)
Is it to remove file only in case of mktemp?
I think it is a risky pattern matching. I suggest '^checkpatches\.'

> +	if [ $? -eq 0 ]; then

Could be easier to read if combining "if" and "grep":
	if echo $tmpinput | grep -q '^checkpatches\.' ; then

> +		rm -f $TMPINPUT
> +	fi
> +}

[...]
> +		TMPINPUT=$(mktemp checkpatches.XXXXXX)

Open to discussion: do we prefer local dir or /tmp?
Some tools are using /tmp.

[...]
> +	report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
> +

Please, no blank line between command and test.

> +	if [ $? -ne 0 ]
> +	then
> +		$verbose || printf '\n### %s\n\n' "$3"
> +		printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> +		ret=1
> +	fi
> +
> +	! $verbose || printf '\nChecking API additions/removals:\n'
> +
> +	report=$($VALIDATE_NEW_API "$TMPINPUT")
> +

Same comments about blank lines.

> +	if [ $? -ne 0 ]; then
> +		printf '%s\n' "$report"
> +		ret=1
> +	fi
> +
> +	clean_tmp_files
> +	if [ $ret -eq 0 ]; then
> +		return 0
>  	fi
> -	[ $? -ne 0 ] || return 0

Why replacing this oneliner by a longer "if" block?

After this review, I think I won't have any more comment.
Thanks Neil