[v8] checkpatches.sh: Add checks for ABI symbol addition
diff mbox series

Message ID 20180614133020.15604-1-nhorman@tuxdriver.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • [v8] checkpatches.sh: Add checks for ABI symbol addition
Related show

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Neil Horman June 14, 2018, 1:30 p.m. UTC
Recently, some additional patches were added to allow for programmatic
marking of C symbols as experimental.  The addition of these markers is
dependent on the manual addition of exported symbols to the EXPERIMENTAL
section of the corresponding libraries version map file.  The consensus
on review is that, in addition to mandating the addition of symbols to
the EXPERIMENTAL version in the map, we need a mechanism to enforce our
documented process of mandating that addition when they are introduced.
To that end, I am proposing this change.  It is an addition to the
checkpatches script, which scan incoming patches for additions and
removals of symbols to the map file, and warns the user appropriately

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: thomas@monjalon.net
CC: john.mcnamara@intel.com
CC: bruce.richardson@intel.com
CC: Ferruh Yigit <ferruh.yigit@intel.com>
CC: Stephen Hemminger <stephen@networkplumber.org>

---
Change notes

v2)
 * Cleaned up and documented awk script (shemminger)
 * fixed sort/uniq usage (shemminger)
 * moved checking to new script (tmonjalon)
 * added maintainer entry (tmonjalon)
 * added license (tmonjalon)

v3)
 * Changed symbol check script name (tmonjalon)
 * Trapped exit to clean temp file (tmonjalon)
 * Honored verbose command (tmonjalon)
 * Cleaned left over debug bits (tmonjalon)
 * Updated location in MAINTAINERS file (tmonjalon)

v4)
 * Updated maintainers file (tmonjalon)

v5)
 * undo V4 (tmojalon)

v6)
 * Cleaning up more nits (tmonjalon)
 * Combining some lines (tmonjalon)
 * Fixing error print condition (tmonjalon)
 * Redirect stdin to a file to allow rewinding for
   Multiple passes on tools (nhorman)

v7)
 * More nits (tmonjalon)
 * consoloidating some common report lines (tmonjalon)
 * move SPDX identifier to line 2 (nhorman)
 * fix some checkpatch errors

v8)
 * spelling fix (nhorman)
 * found a way to eliminate the use of filterdiff (new awk rules)
---
 MAINTAINERS                     |   1 +
 devtools/check-symbol-change.sh | 161 ++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  50 ++++++++--
 3 files changed, 205 insertions(+), 7 deletions(-)
 create mode 100755 devtools/check-symbol-change.sh

Comments

Thomas Monjalon June 25, 2018, 11:04 p.m. UTC | #1
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
Neil Horman June 27, 2018, 5:58 p.m. UTC | #2
On Tue, Jun 26, 2018 at 01:04:16AM +0200, Thomas Monjalon wrote:
> 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.
> 
I prefer echo, just because its what I've always used.  I'm open to changing it
if there is a compelling reason to do so.

> [...]
> > +exit $exit_code
> > +
> > +
> 
> Ironically, this patch doesn't pass checkpatch test because of
> the trailing new lines.
> 
Patchwork reports only a single error:
https://patches.dpdk.org/patch/41139/

Is the CI checkpatch different from the checkpatch we provide here?  If so, why?

> [...]
> > +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).
> 
Sure

> What the grep is supposed to match?
The name of the temp file created.  TMPINPUT can take on two classes of values:
1) The name of a passed in patch file
2) The name of a temp file created to hold a patch streamed in on stdin

when we clean up on exit, we want to delete class 2, but never class 1, so we
match on the temp file root name 'checkpatch'

> (side note, there is a typo: checkpaches -> checkpatches)
will fix

> Is it to remove file only in case of mktemp?
yes

> I think it is a risky pattern matching. I suggest '^checkpatches\.'
fine

> 
> > +	if [ $? -eq 0 ]; then
> 
> Could be easier to read if combining "if" and "grep":
> 	if echo $tmpinput | grep -q '^checkpatches\.' ; then
That seems fairly out of line with the style of the rest of the file

> 
> > +		rm -f $TMPINPUT
> > +	fi
> > +}
> 
> [...]
> > +		TMPINPUT=$(mktemp checkpatches.XXXXXX)
> 
> Open to discussion: do we prefer local dir or /tmp?
> Some tools are using /tmp.
> 
I don't think thats our call.  If an end user wants to specify the location of
temp files, they can do so by setting $TMPDIR.  We don't need to mandate it.

> [...]
> > +	report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
> > +
> 
> Please, no blank line between command and test.
> 
very well

> > +	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?
> 
Because it was in keeping with the style that I was working with.  I can revert
it

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

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index 4667fa7fb..7b1180fe0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -122,6 +122,7 @@  M: Neil Horman <nhorman@tuxdriver.com>
 F: lib/librte_compat/
 F: doc/guides/rel_notes/deprecation.rst
 F: devtools/validate-abi.sh
+F: devtools/check-symbol-change.sh
 F: buildtools/check-experimental-syms.sh
 
 Driver information
diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
new file mode 100755
index 000000000..6d7d83db4
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,161 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2018 Neil Horman <nhorman@tuxdriver.com>
+
+build_map_changes()
+{
+	local fname=$1
+	local mapdb=$2
+
+	cat $fname | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=0; in_map=0}
+
+		# Anything that starts with + or -, followed by an a
+		# and ends in the string .map is the name of our map file
+		# This may appear multiple times in a patch if multiple
+		# map files are altered, and all section/symbol names
+		# appearing between a triggering of this rule and the
+		# next trigger of this rule are associated with this file
+		/[-+] a\/.*\.map/ {map=$2; in_map=1}
+
+		# Same pattern as above, only it matches on anything that
+		# doesnt end in 'map', indicating we have left the map chunk.
+		# When we hit this, turn off the in_map variable, which
+		# supresses the subordonate rules below
+		/[-+] a\/.*\.^(map)/ {in_map=0}
+
+		# Triggering this rule, which starts a line with a + and ends it
+		# with a { identifies a versioned section.  The section name is
+		# the rest of the line with the + and { symbols remvoed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/+.*{/ {gsub("+","");
+			if (in_map == 1) {
+				sec=$1; in_sec=1;
+			}
+		}
+
+		# This rule idenfies the end of a section, and disables the
+		# symbol rule
+		/.*}/ {in_sec=0}
+
+		# This rule matches on a + followed by any characters except a :
+		# (which denotes a global vs local segment), and ends with a ;.
+		# The semicolon is removed and the symbol is printed with its
+		# association file name and version section, along with an
+		# indicator that the symbol is a new addition.  Note this rule
+		# only works if we have found a version section in the rule
+		# above (hence the in_sec check) And found a map file (the
+		# in_map check).  If we are not in a map chunk, do nothing.  If
+		# we are in a map chunk but not a section chunk, record it as
+		# unknown.
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			if (in_map == 1) {
+				if (in_sec == 1) {
+					print map " " sym " " sec " add"
+				} else {
+					print map " " sym " unknown add"
+				}
+			}
+		}
+
+		# This is the same rule as above, but the rule matches on a
+		# leading - rather than a +, denoting that the symbol is being
+		# removed.
+		/^-[^}].*[^:*];/ {gsub(";","");sym=$2;
+			if (in_map == 1) {
+				if (in_sec == 1) {
+					print map " " sym " " sec " del"
+				} else {
+					print map " " sym " unknown del"
+				}
+			}
+		}' > ./$mapdb
+
+		sort -u $mapdb > ./$mapdb.2
+		mv -f $mapdb.2 $mapdb
+
+}
+
+check_for_rule_violations()
+{
+	local mapdb=$1
+	local mname
+	local symname
+	local secname
+	local ar
+	local ret=0
+
+	while read mname symname secname ar
+	do
+		if [ "$ar" == "add" ]
+		then
+
+			if [ "$secname" == "unknown" ]
+			then
+				# 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"
+				continue
+			fi
+
+			if [ "$secname" != "EXPERIMENTAL" ]
+			then
+				# Symbols that are getting added in a section
+				# other than the experimental section
+				# to be moving from an already supported
+				# section or its a violation
+				grep -q \
+				"$mname $symname [^EXPERIMENTAL] del" $mapdb
+				if [ $? -ne 0 ]
+				then
+					echo -n "ERROR: symbol $symname "
+					echo -n "is added in a section "
+					echo -n "other than the EXPERIMENTAL "
+					echo "section of the version map"
+					ret=1
+				fi
+			fi
+		else
+
+			if [ "$secname" != "EXPERIMENTAL" ]
+			then
+				# Just inform users that non-experimenal
+				# symbols need to go through a deprecation
+				# process
+				echo -n "INFO: symbol $symname is being "
+				echo -n "removed, ensure that it has "
+				echo "gone through the deprecation process"
+			fi
+		fi
+	done < $mapdb
+
+	return $ret
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=`mktemp mapdb.XXXXXX`
+patch=$1
+exit_code=1
+
+clean_and_exit_on_sig()
+{
+	rm -f $mapfile
+	exit $exit_code
+}
+
+build_map_changes $patch $mapfile
+check_for_rule_violations $mapfile
+exit_code=$?
+
+rm -f $mapfile
+
+exit $exit_code
+
+
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 663b7c426..10ba35340 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -7,6 +7,9 @@ 
 # - DPDK_CHECKPATCH_LINE_LENGTH
 . $(dirname $(readlink -e $0))/load-devel-config
 
+
+VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
+
 length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
@@ -21,6 +24,15 @@  SPLIT_STRING,LONG_LINE_STRING,\
 LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\
 NEW_TYPEDEFS,COMPARISON_TO_NULL"
 
+clean_tmp_files() {
+	echo $TMPINPUT | grep -q checkpaches
+	if [ $? -eq 0 ]; then
+		rm -f $TMPINPUT
+	fi
+}
+
+trap "clean_tmp_files" SIGINT
+
 print_usage () {
 	cat <<- END_OF_HELP
 	usage: $(basename $0) [-q] [-v] [-nX|patch1 [patch2] ...]]
@@ -58,19 +70,43 @@  total=0
 status=0
 
 check () { # <patch> <commit> <title>
+	local ret=0
+
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
-		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
+		TMPINPUT=$1
 	elif [ -n "$2" ] ; then
-		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
-			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
+		TMPINPUT=$(mktemp checkpatches.XXXXXX)
+		git format-patch --find-renames \
+		--no-stat --stdout -1 $commit > ./$TMPINPUT
 	else
-		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
+		TMPINPUT=$(mktemp checkpatches.XXXXXX)
+		cat > ./$TMPINPUT
+	fi
+
+	report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
+
+	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")
+
+	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
-	$verbose || printf '\n### %s\n\n' "$3"
-	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
 	status=$(($status + 1))
 }