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

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

[Neil Horman]

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

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 7676a6b50..d0e2120fe 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -61,6 +61,77 @@ print_usage () {
 	END_OF_HELP
 }
 
+build_map_changes()
+{
+	local fname=$1
+	local mapdb=$2
+
+	cat $fname | filterdiff -i *.map | awk '
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=0}
+		/[-+] a\/.*\.map/ {map=$2}
+		/+.*{/ {gsub("+","");sec=$1; in_sec=1}
+		/.*}/ {in_sec=0}
+		/^+.*[^:*];/ {gsub(";","");sym=$2;
+			if (in_sec == 1) {
+				print map " " sym " " sec " add"
+			}
+		}
+		/^-.*[^:*];/ {gsub(";","");sym=$2;
+			if (in_sec == 1) {
+				print map " " sym " " sec " del"
+			}
+		}' > ./$mapdb
+
+		sort $mapdb | uniq > ./$mapdb.2
+		mv -f $mapdb.2 $mapdb
+
+}
+
+check_for_rule_violations()
+{
+	local mapdb=$1
+	local mname
+	local symname
+	local secname
+	local ar
+
+	cat $mapdb | while read mname symname secname ar
+	do
+		if [ "$ar" == "add" -a "$secname" != "EXPERIMENTAL" ]
+		then
+			# Symbols that are getting added in a section other
+			# Than the experimental section need to be moving
+			# from an already supported section or its a violation
+			grep -q "$mname $symname [^EXPERIMENTAL] del" $mapdb
+			if [ $? -ne 0 ]
+			then
+				echo "ERROR: symbol $symname is added in a section other than the EXPERIMENTAL section of the version map"
+			fi
+		fi
+
+		if [ "$ar" == "del" -a "$secname" != "EXPERIMENTAL" ]
+		then
+			# Just inform users that non-experimenal symbols need
+			# to go through a deprecation process
+			echo "INFO: symbol $symname is being removed, ensure that it has gone through the deprecation process"
+		fi
+
+	done
+}
+
+validate_api_changes()
+{
+	mapfile=`mktemp mapdb.XXXXXX`
+	patch=$1
+
+	build_map_changes $patch $mapfile
+	result=$(check_for_rule_violations $mapfile)
+
+	rm -f $mapfile
+
+	echo $result
+}
+
 number=0
 quiet=false
 verbose=false
@@ -96,9 +167,25 @@ check () { # <patch> <commit> <title>
 	else
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
-	[ $? -ne 0 ] || return 0
+	reta=$?
+
 	$verbose || printf '\n### %s\n\n' "$3"
 	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	echo
+	echo "Checking API additions/removals:"
+	if [ -n "$1" ] ; then
+		report=$(validate_api_changes $1)
+	elif [ -n "$2" ] ; then
+		report=$(git format-patch \
+			 --find-renames --no-stat --stdout -1 $commit |
+			validate_api_changes /dev/stdin)
+	else
+		report=$(validate_api_changes /dev/stdin)
+	fi
+	[ $? -ne 0 -o $reta -ne 0 ] || return 0
+	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
 	status=$(($status + 1))
 }
 
-- 
2.14.3

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

[Thomas Monjalon]

15/01/2018 20:05, Neil Horman:
> 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

Thanks for working on this.
I won't pretend that I understand anything in this awk script :)

I think it would be better to put this code in a new script,
let's say check-symbol-change.sh, and call it in checkpatches.sh.
It would be just moving functions, add your copyright, and list the new
script in your MAINTAINERS section "ABI versioning".

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

[Neil Horman]

On Mon, Jan 15, 2018 at 10:52:25PM +0100, Thomas Monjalon wrote:
> 15/01/2018 20:05, Neil Horman:
> > 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
> 
> Thanks for working on this.
Sure.

> I won't pretend that I understand anything in this awk script :)
> 
Stephen suggested that I clean it up and document it a bit, which is probably a good idea.

> I think it would be better to put this code in a new script,
> let's say check-symbol-change.sh, and call it in checkpatches.sh.
> It would be just moving functions, add your copyright, and list the new
> script in your MAINTAINERS section "ABI versioning".
Yeah, I can do that.  New version in the AM
Neil

> 

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

[Stephen Hemminger]

On Mon, 15 Jan 2018 14:05:45 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

>  
> +build_map_changes()
> +{
> +	local fname=$1
> +	local mapdb=$2
> +
> +	cat $fname | filterdiff -i *.map | awk '

You don't need cat, use shell redirection same for later while loop.

> +		BEGIN {map="";sym="";ar="";sec=""; in_sec=0}
> +		/[-+] a\/.*\.map/ {map=$2}
> +		/+.*{/ {gsub("+","");sec=$1; in_sec=1}
Add some whitespace and indentation to awk?

> +		/.*}/ {in_sec=0}
> +		/^+.*[^:*];/ {gsub(";","");sym=$2;
> +			if (in_sec == 1) {
> +				print map " " sym " " sec " add"
> +			}
> +		}
> +		/^-.*[^:*];/ {gsub(";","");sym=$2;
> +			if (in_sec == 1) {
> +				print map " " sym " " sec " del"
> +			}
> +		}' > ./$mapdb
> +
> +		sort $mapdb | uniq > ./$mapdb.2

sort -u

> +		mv -f $mapdb.2 $mapdb
> +
> +}
> +

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

[Neil Horman]

On Mon, Jan 15, 2018 at 02:20:38PM -0800, Stephen Hemminger wrote:
> On Mon, 15 Jan 2018 14:05:45 -0500
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> >  
> > +build_map_changes()
> > +{
> > +	local fname=$1
> > +	local mapdb=$2
> > +
> > +	cat $fname | filterdiff -i *.map | awk '
> 
> You don't need cat, use shell redirection same for later while loop.
> 
This is likely moot given Thomas' request to put this in a separate script, but
point taken

> > +		BEGIN {map="";sym="";ar="";sec=""; in_sec=0}
> > +		/[-+] a\/.*\.map/ {map=$2}
> > +		/+.*{/ {gsub("+","");sec=$1; in_sec=1}
> Add some whitespace and indentation to awk? 
Yeah, I can document this block as a whole as well

> 
> > +		/.*}/ {in_sec=0}
> > +		/^+.*[^:*];/ {gsub(";","");sym=$2;
> > +			if (in_sec == 1) {
> > +				print map " " sym " " sec " add"
> > +			}
> > +		}
> > +		/^-.*[^:*];/ {gsub(";","");sym=$2;
> > +			if (in_sec == 1) {
> > +				print map " " sym " " sec " del"
> > +			}
> > +		}' > ./$mapdb
> > +
> > +		sort $mapdb | uniq > ./$mapdb.2
> 
> sort -u
Copy that.

> 
> > +		mv -f $mapdb.2 $mapdb
> > +
> > +}
> > +
> 

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

[Neil Horman]

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)
---
 MAINTAINERS                  |   2 +
 devtools/checkpatches.sh     |  22 ++++++-
 devtools/validate-new-api.sh | 138 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100755 devtools/validate-new-api.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index b51c2d096..d3a3c245e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -42,6 +42,7 @@ F: doc/
 
 Developers and Maintainers Tools
 M: Thomas Monjalon <thomas@monjalon.net>
+M: Neil Horman <nhorman@tuxdriver.com>
 F: MAINTAINERS
 F: devtools/check-dup-includes.sh
 F: devtools/check-maintainers.sh
@@ -52,6 +53,7 @@ F: devtools/get-maintainer.sh
 F: devtools/git-log-fixes.sh
 F: devtools/load-devel-config
 F: devtools/test-build.sh
+F: devtools/validate-new-api.sh
 F: license/
 
 
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 7676a6b50..cf5c67b09 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -35,6 +35,8 @@
 # - DPDK_CHECKPATCH_LINE_LENGTH
 . $(dirname $(readlink -e $0))/load-devel-config
 
+export VALIDATE_NEW_API=$(dirname $(readlink -e $0))/validate-new-api.sh
+
 length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
@@ -51,6 +53,7 @@ NEW_TYPEDEFS,COMPARISON_TO_NULL"
 
 print_usage () {
 	cat <<- END_OF_HELP
+	$(dirname $0)
 	usage: $(basename $0) [-q] [-v] [-nX|patch1 [patch2] ...]]
 
 	Run Linux kernel checkpatch.pl with DPDK options.
@@ -61,6 +64,7 @@ print_usage () {
 	END_OF_HELP
 }
 
+
 number=0
 quiet=false
 verbose=false
@@ -96,9 +100,25 @@ check () { # <patch> <commit> <title>
 	else
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
-	[ $? -ne 0 ] || return 0
+	reta=$?
+
 	$verbose || printf '\n### %s\n\n' "$3"
 	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	echo
+	echo "Checking API additions/removals:"
+	if [ -n "$1" ] ; then
+		report=$($VALIDATE_NEW_API $1)
+	elif [ -n "$2" ] ; then
+		report=$(git format-patch \
+			 --find-renames --no-stat --stdout -1 $commit |
+			$VALIDATE_NEW_API -)
+	else
+		report=$($VALIDATE_NEW_API -)
+	fi
+	[ $? -ne 0 -o $reta -ne 0 ] || return 0
+	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
 	status=$(($status + 1))
 }
 
diff --git a/devtools/validate-new-api.sh b/devtools/validate-new-api.sh
new file mode 100755
index 000000000..3499f5f8b
--- /dev/null
+++ b/devtools/validate-new-api.sh
@@ -0,0 +1,138 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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
+}
+
+
+mapfile=`mktemp mapdb.XXXXXX`
+patch=$1
+
+build_map_changes $patch $mapfile
+check_for_rule_violations $mapfile
+exit_code=$?
+
+rm -f $mapfile
+
+exit $exit_code
+
+
-- 
2.14.3

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

[Thomas Monjalon]

Hi,

16/01/2018 19:22, Neil Horman:
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
>  Developers and Maintainers Tools
>  M: Thomas Monjalon <thomas@monjalon.net>
> +M: Neil Horman <nhorman@tuxdriver.com>
>  F: MAINTAINERS
>  F: devtools/check-dup-includes.sh
>  F: devtools/check-maintainers.sh
> @@ -52,6 +53,7 @@ F: devtools/get-maintainer.sh
>  F: devtools/git-log-fixes.sh
>  F: devtools/load-devel-config
>  F: devtools/test-build.sh
> +F: devtools/validate-new-api.sh
>  F: license/

I really think it should be in the section "ABI versioning""

> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> +export VALIDATE_NEW_API=$(dirname $(readlink -e $0))/validate-new-api.sh

Why export?

>  print_usage () {
>  	cat <<- END_OF_HELP
> +	$(dirname $0)
>  	usage: $(basename $0) [-q] [-v] [-nX|patch1 [patch2] ...]]

This dirname is a debug leftover?

> @@ -96,9 +100,25 @@ check () { # <patch> <commit> <title>
>  	else
>  		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>  	fi
> -	[ $? -ne 0 ] || return 0
> +	reta=$?

What means reta?

> +
>  	$verbose || printf '\n### %s\n\n' "$3"
>  	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> +
> +	echo
> +	echo "Checking API additions/removals:"

You should respect $verbose before printing such header.

> +	if [ -n "$1" ] ; then
> +		report=$($VALIDATE_NEW_API $1)
> +	elif [ -n "$2" ] ; then
> +		report=$(git format-patch \
> +			 --find-renames --no-stat --stdout -1 $commit |
> +			$VALIDATE_NEW_API -)
> +	else
> +		report=$($VALIDATE_NEW_API -)
> +	fi
> +	[ $? -ne 0 -o $reta -ne 0 ] || return 0
> +	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> +
>  	status=$(($status + 1))
>  }

> --- /dev/null
> +++ b/devtools/validate-new-api.sh

About the file name, is it only for new API?
You don't like check-symbol-change.sh ?
It may be stupid, but I thought "validate" is more related to full test,
like validate-abi.sh does for the ABI, and "check" can be a partial
test like done in checkpatches.sh.

> +		}' > ./$mapdb
> +
> +		sort -u $mapdb > ./$mapdb.2
> +		mv -f $mapdb.2 $mapdb
[...]
> +mapfile=`mktemp mapdb.XXXXXX`
[...]
> +rm -f $mapfile

If you create temporary file, you should remove it in a trap cleanup,
in case of interrupted processing.
The best is to avoid temp file, but use variables instead.

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

[Neil Horman]

On Sun, Jan 21, 2018 at 09:29:18PM +0100, Thomas Monjalon wrote:
> Hi,
> 
> 16/01/2018 19:22, Neil Horman:
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> >  Developers and Maintainers Tools
> >  M: Thomas Monjalon <thomas@monjalon.net>
> > +M: Neil Horman <nhorman@tuxdriver.com>
> >  F: MAINTAINERS
> >  F: devtools/check-dup-includes.sh
> >  F: devtools/check-maintainers.sh
> > @@ -52,6 +53,7 @@ F: devtools/get-maintainer.sh
> >  F: devtools/git-log-fixes.sh
> >  F: devtools/load-devel-config
> >  F: devtools/test-build.sh
> > +F: devtools/validate-new-api.sh
> >  F: license/
> 
> I really think it should be in the section "ABI versioning""
> 
I can do that.

> > --- a/devtools/checkpatches.sh
> > +++ b/devtools/checkpatches.sh
> > +export VALIDATE_NEW_API=$(dirname $(readlink -e $0))/validate-new-api.sh
> 
> Why export?
> 
As I recall I had needed that in an earlier incantation of this script, but its
likely not needed any longer

> >  print_usage () {
> >  	cat <<- END_OF_HELP
> > +	$(dirname $0)
> >  	usage: $(basename $0) [-q] [-v] [-nX|patch1 [patch2] ...]]
> 
> This dirname is a debug leftover?
Yes.

> 
> > @@ -96,9 +100,25 @@ check () { # <patch> <commit> <title>
> >  	else
> >  		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> >  	fi
> > -	[ $? -ne 0 ] || return 0
> > +	reta=$?
> 
> What means reta?
> 
just a subindex on a return code.

> > +
> >  	$verbose || printf '\n### %s\n\n' "$3"
> >  	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > +
> > +	echo
> > +	echo "Checking API additions/removals:"
> 
> You should respect $verbose before printing such header.
> 
I can add a quiet/verbose mode option, but I didn't think it was needed here
since its being run automatically from within checkpatches.

> > +	if [ -n "$1" ] ; then
> > +		report=$($VALIDATE_NEW_API $1)
> > +	elif [ -n "$2" ] ; then
> > +		report=$(git format-patch \
> > +			 --find-renames --no-stat --stdout -1 $commit |
> > +			$VALIDATE_NEW_API -)
> > +	else
> > +		report=$($VALIDATE_NEW_API -)
> > +	fi
> > +	[ $? -ne 0 -o $reta -ne 0 ] || return 0
> > +	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > +
> >  	status=$(($status + 1))
> >  }
> 
> > --- /dev/null
> > +++ b/devtools/validate-new-api.sh
> 
> About the file name, is it only for new API?
> You don't like check-symbol-change.sh ?
> It may be stupid, but I thought "validate" is more related to full test,
> like validate-abi.sh does for the ABI, and "check" can be a partial
> test like done in checkpatches.sh.
> 
I can change the name, but to answer your question, its realy meant to validate
any changes to a version map, so change.sh suffixes might be more appropriate.

> > +		}' > ./$mapdb
> > +
> > +		sort -u $mapdb > ./$mapdb.2
> > +		mv -f $mapdb.2 $mapdb
> [...]
> > +mapfile=`mktemp mapdb.XXXXXX`
> [...]
> > +rm -f $mapfile
> 
> If you create temporary file, you should remove it in a trap cleanup,
> in case of interrupted processing.
> The best is to avoid temp file, but use variables instead.
I'm not going to be able to avoid a temp file, since the number of changes in a
map are inditerminate.  I can trap and clean up the temp files though.

I'm still in transit, so it will likely be a few days before I can get to this.

Neil

> 

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

[Thomas Monjalon]

22/01/2018 02:54, Neil Horman:
> On Sun, Jan 21, 2018 at 09:29:18PM +0100, Thomas Monjalon wrote:
> > >  	$verbose || printf '\n### %s\n\n' "$3"
> > >  	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > > +
> > > +	echo
> > > +	echo "Checking API additions/removals:"
> > 
> > You should respect $verbose before printing such header.
> > 
> I can add a quiet/verbose mode option, but I didn't think it was needed here
> since its being run automatically from within checkpatches.

I mean there is a verbose option already.
So you just have to take it into account when printing.
Thanks

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

[Neil Horman]

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)
---
 MAINTAINERS                     |   2 +
 devtools/check-symbol-change.sh | 146 ++++++++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  23 ++++++-
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100755 devtools/check-symbol-change.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index acd056134..417115f97 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -42,6 +42,7 @@ F: doc/
 
 Developers and Maintainers Tools
 M: Thomas Monjalon <thomas@monjalon.net>
+M: Neil Horman <nhorman@tuxdriver.com>
 F: MAINTAINERS
 F: devtools/check-dup-includes.sh
 F: devtools/check-maintainers.sh
@@ -86,6 +87,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..22b17e6f2
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,146 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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 7676a6b50..0b2b5f039 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -35,6 +35,8 @@
 # - 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
@@ -61,6 +63,7 @@ print_usage () {
 	END_OF_HELP
 }
 
+
 number=0
 quiet=false
 verbose=false
@@ -86,6 +89,7 @@ total=0
 status=0
 
 check () { # <patch> <commit> <title>
+	local reta
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
@@ -96,9 +100,26 @@ check () { # <patch> <commit> <title>
 	else
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
-	[ $? -ne 0 ] || return 0
+	reta=$?
+
 	$verbose || printf '\n### %s\n\n' "$3"
 	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	! $verbose || echo
+	! $verbose || echo "Checking API additions/removals:"
+
+	if [ -n "$1" ] ; then
+		report=$($VALIDATE_NEW_API $1)
+	elif [ -n "$2" ] ; then
+		report=$(git format-patch \
+			 --find-renames --no-stat --stdout -1 $commit |
+			$VALIDATE_NEW_API -)
+	else
+		report=$($VALIDATE_NEW_API -)
+	fi
+	[ $? -ne 0 -o $reta -ne 0 ] || return 0
+	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
 	status=$(($status + 1))
 }
 
-- 
2.14.3

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

[Thomas Monjalon]

31/01/2018 18:27, Neil Horman:
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -42,6 +42,7 @@ F: doc/
>  
>  Developers and Maintainers Tools
>  M: Thomas Monjalon <thomas@monjalon.net>
> +M: Neil Horman <nhorman@tuxdriver.com>
>  F: MAINTAINERS
>  F: devtools/check-dup-includes.sh
>  F: devtools/check-maintainers.sh

You don't need to add your name in this general section.
The new file is now in "ABI versioning" section that you already maintain.

Talking about maintenance, you are welcome to put your name
into "Driver information" section :)

> @@ -86,6 +87,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

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

[Neil Horman]

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)
---
 MAINTAINERS                     |   2 +
 devtools/check-symbol-change.sh | 146 ++++++++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  23 ++++++-
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100755 devtools/check-symbol-change.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index acd056134..d1ef43479 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -86,9 +86,11 @@ 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
+M: Neil Horman <nhorman@tuxdriver.com>
 F: buildtools/pmdinfogen/
 F: usertools/dpdk-pmdinfo.py
 F: doc/guides/tools/pmdinfo.rst
diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
new file mode 100755
index 000000000..22b17e6f2
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,146 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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 7676a6b50..0b2b5f039 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -35,6 +35,8 @@
 # - 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
@@ -61,6 +63,7 @@ print_usage () {
 	END_OF_HELP
 }
 
+
 number=0
 quiet=false
 verbose=false
@@ -86,6 +89,7 @@ total=0
 status=0
 
 check () { # <patch> <commit> <title>
+	local reta
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
@@ -96,9 +100,26 @@ check () { # <patch> <commit> <title>
 	else
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
-	[ $? -ne 0 ] || return 0
+	reta=$?
+
 	$verbose || printf '\n### %s\n\n' "$3"
 	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	! $verbose || echo
+	! $verbose || echo "Checking API additions/removals:"
+
+	if [ -n "$1" ] ; then
+		report=$($VALIDATE_NEW_API $1)
+	elif [ -n "$2" ] ; then
+		report=$(git format-patch \
+			 --find-renames --no-stat --stdout -1 $commit |
+			$VALIDATE_NEW_API -)
+	else
+		report=$($VALIDATE_NEW_API -)
+	fi
+	[ $? -ne 0 -o $reta -ne 0 ] || return 0
+	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
 	status=$(($status + 1))
 }
 
-- 
2.14.3

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

[Thomas Monjalon]

05/02/2018 18:29, Neil Horman:
>  Driver information
> +M: Neil Horman <nhorman@tuxdriver.com>
>  F: buildtools/pmdinfogen/
>  F: usertools/dpdk-pmdinfo.py
>  F: doc/guides/tools/pmdinfo.rst

This change deserves a separate patch announcing that you are
volunteer to maintain pmdinfo.

It is really not related to the rest of this patch,
but thank you for vounteering :)

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

[Neil Horman]

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)
---
 MAINTAINERS                     |   1 +
 devtools/check-symbol-change.sh | 146 ++++++++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  23 ++++++-
 3 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100755 devtools/check-symbol-change.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index acd056134..d9d2abff8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -86,6 +86,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..22b17e6f2
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,146 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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 7676a6b50..0b2b5f039 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -35,6 +35,8 @@
 # - 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
@@ -61,6 +63,7 @@ print_usage () {
 	END_OF_HELP
 }
 
+
 number=0
 quiet=false
 verbose=false
@@ -86,6 +89,7 @@ total=0
 status=0
 
 check () { # <patch> <commit> <title>
+	local reta
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
@@ -96,9 +100,26 @@ check () { # <patch> <commit> <title>
 	else
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
-	[ $? -ne 0 ] || return 0
+	reta=$?
+
 	$verbose || printf '\n### %s\n\n' "$3"
 	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	! $verbose || echo
+	! $verbose || echo "Checking API additions/removals:"
+
+	if [ -n "$1" ] ; then
+		report=$($VALIDATE_NEW_API $1)
+	elif [ -n "$2" ] ; then
+		report=$(git format-patch \
+			 --find-renames --no-stat --stdout -1 $commit |
+			$VALIDATE_NEW_API -)
+	else
+		report=$($VALIDATE_NEW_API -)
+	fi
+	[ $? -ne 0 -o $reta -ne 0 ] || return 0
+	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
 	status=$(($status + 1))
 }
 
-- 
2.14.3

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

[Thomas Monjalon]

Hi,

I wanted to push this patch in 18.02, but when looking more closely,
I see few things to improve.
As it is a tool, there is no harm to wait one more week and push it
early in 18.05.

09/02/2018 16:21, Neil Horman:
>  check () { # <patch> <commit> <title>
> +	local reta
>  	total=$(($total + 1))
>  	! $verbose || printf '\n### %s\n\n' "$3"
>  	if [ -n "$1" ] ; then
> @@ -96,9 +100,26 @@ check () { # <patch> <commit> <title>
>  	else
>  		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>  	fi
> -	[ $? -ne 0 ] || return 0

You are removing the return, so the report will be always printed.
You must print the report only in case of error.

> +	reta=$?
> +
>  	$verbose || printf '\n### %s\n\n' "$3"
>  	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> +
> +	! $verbose || echo
> +	! $verbose || echo "Checking API additions/removals:"

You can use printf to combine these lines.

> +
> +	if [ -n "$1" ] ; then
> +		report=$($VALIDATE_NEW_API $1)

Beware of spaces in file names: use quoted "$1".

> +	elif [ -n "$2" ] ; then
> +		report=$(git format-patch \
> +			 --find-renames --no-stat --stdout -1 $commit |
> +			$VALIDATE_NEW_API -)
> +	else
> +		report=$($VALIDATE_NEW_API -)

So your script supports "-" for stdin? Nice

> +	fi
> +	[ $? -ne 0 -o $reta -ne 0 ] || return 0

Suggestion of more explicit variable naming:
$reta -> style_result
$? -> symbol_result

> +	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'

Wrong copy/paste: the sed is useless for the API report.

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

[Neil Horman]

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)
---
 MAINTAINERS                     |   1 +
 devtools/check-symbol-change.sh | 146 ++++++++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  46 +++++++++++--
 3 files changed, 188 insertions(+), 5 deletions(-)
 create mode 100755 devtools/check-symbol-change.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index a646ca3e1..f83b9ab33 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -87,6 +87,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..22b17e6f2
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,146 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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 7676a6b50..fa36b0d98 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -35,6 +35,10 @@
 # - DPDK_CHECKPATCH_LINE_LENGTH
 . $(dirname $(readlink -e $0))/load-devel-config
 
+trap "rm -f $TMPINPUT" SIGINT
+
+VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
+
 length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
@@ -61,6 +65,7 @@ print_usage () {
 	END_OF_HELP
 }
 
+
 number=0
 quiet=false
 verbose=false
@@ -86,19 +91,50 @@ total=0
 status=0
 
 check () { # <patch> <commit> <title>
+	local ret=0
+	TMPINPUT=`mktemp checkpatches.XXXXXX`
+
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
 		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
 	elif [ -n "$2" ] ; then
-		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
+		git format-patch --find-renames --no-stat --stdout -1 $commit > ./$TMPINPUT
+		report=$(cat ./$TMPINPUT |
 			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	else
-		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
+		cat > ./$TMPINPUT
+		report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
+	fi
+	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'
+
+	if [ -n "$1" ] ; then
+		report=$($VALIDATE_NEW_API "$1")
+	elif [ -n "$2" ] ; then
+		report=$( cat ./$TMPINPUT | 
+			$VALIDATE_NEW_API -)
+	else
+		report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
+	fi
+
+	if [ $? -ne 0 ]
+	then
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
+	rm -f ./$TMPINPUT
+	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))
 }
 
-- 
2.14.3

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

[Thomas Monjalon]

Hi Neil,

Sorry, this patch has been forgotten during the whole release cycle.

I see an issue about the dependency on filterdiff.
Is there a way to avoid it?

I have some minor comments below.

14/02/2018 20:19, Neil Horman:
> @@ -61,6 +65,7 @@ print_usage () {
>  	END_OF_HELP
>  }
>  
> +

This new blank line is probably a leftover.

>  number=0
>  quiet=false
>  verbose=false
> @@ -86,19 +91,50 @@ total=0
>  status=0
>  
>  check () { # <patch> <commit> <title>
> +	local ret=0
> +	TMPINPUT=`mktemp checkpatches.XXXXXX`

It is preferred to use $() construct to be consistent in the file.

> +
>  	total=$(($total + 1))
>  	! $verbose || printf '\n### %s\n\n' "$3"
>  	if [ -n "$1" ] ; then
>  		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
>  	elif [ -n "$2" ] ; then
> -		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> +		git format-patch --find-renames --no-stat --stdout -1 $commit > ./$TMPINPUT
> +		report=$(cat ./$TMPINPUT |
>  			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)

No need to use cat here.

>  	else
> -		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> +		cat > ./$TMPINPUT
> +		report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> +	fi

All calls to DPDK_CHECKPATCH_PATH can be moved to one call after the "fi".
Just need to set TPMINPUT=$1 in first case (and not remove TPMINPUT
in this case).

> +	if [ $? -ne 0 ]
> +	then

For consistency, the style in this file is:
	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'
> +
> +	if [ -n "$1" ] ; then
> +		report=$($VALIDATE_NEW_API "$1")
> +	elif [ -n "$2" ] ; then
> +		report=$( cat ./$TMPINPUT | 
> +			$VALIDATE_NEW_API -)
> +	else
> +		report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
> +	fi

Same as above, it can be simplified by only one line for all cases.

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

[Neil Horman]

On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> Sorry, this patch has been forgotten during the whole release cycle.
> 
Its ok, though that is frustrating, as we now need to re-familiarize ourselves
with how this patch works.

> I see an issue about the dependency on filterdiff.
> Is there a way to avoid it?
> 
I suppose, but to do so would require some awk magic.  Is there any advantage to
having an awk dependency rather than a filterdiff dependency?  filterdiff is a
pretty universal tool.

> I have some minor comments below.
> 
> 14/02/2018 20:19, Neil Horman:
> > @@ -61,6 +65,7 @@ print_usage () {
> >  	END_OF_HELP
> >  }
> >  
> > +
> 
> This new blank line is probably a leftover.
Yeah, if I wind up respinning this, I'll remove it.

> 
> >  number=0
> >  quiet=false
> >  verbose=false
> > @@ -86,19 +91,50 @@ total=0
> >  status=0
> >  
> >  check () { # <patch> <commit> <title>
> > +	local ret=0
> > +	TMPINPUT=`mktemp checkpatches.XXXXXX`
> 
> It is preferred to use $() construct to be consistent in the file.
> 
> > +
> >  	total=$(($total + 1))
> >  	! $verbose || printf '\n### %s\n\n' "$3"
> >  	if [ -n "$1" ] ; then
> >  		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
> >  	elif [ -n "$2" ] ; then
> > -		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> > +		git format-patch --find-renames --no-stat --stdout -1 $commit > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT |
> >  			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> 
> No need to use cat here.
> 
Actually, I vaguely recall with these changes I did encounter a problem with passing
output to checkpatch, but its been so long I can't recall now.

> >  	else
> > -		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +		cat > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +	fi
> 
> All calls to DPDK_CHECKPATCH_PATH can be moved to one call after the "fi".
> Just need to set TPMINPUT=$1 in first case (and not remove TPMINPUT
> in this case).
> 
Sure.

> > +	if [ $? -ne 0 ]
> > +	then
> 
> For consistency, the style in this file is:
> 	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'
> > +
> > +	if [ -n "$1" ] ; then
> > +		report=$($VALIDATE_NEW_API "$1")
> > +	elif [ -n "$2" ] ; then
> > +		report=$( cat ./$TMPINPUT | 
> > +			$VALIDATE_NEW_API -)
> > +	else
> > +		report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
> > +	fi
> 
> Same as above, it can be simplified by only one line for all cases.

Fine, at this point, I don't know when I'll get to this though, its pretty busy
here at the moment.

> 
> 
> 
> 

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

[Thomas Monjalon]

27/05/2018 23:00, Neil Horman:
> On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> > Hi Neil,
> > 
> > Sorry, this patch has been forgotten during the whole release cycle.
> > 
> Its ok, though that is frustrating, as we now need to re-familiarize ourselves
> with how this patch works.

I understand and apologize.

> > I see an issue about the dependency on filterdiff.
> > Is there a way to avoid it?
> > 
> I suppose, but to do so would require some awk magic.  Is there any advantage to
> having an awk dependency rather than a filterdiff dependency?  filterdiff is a
> pretty universal tool.

filterdiff is not installed on my machine.
I guess it is the same for a lot of people.

I will check how we can replace it.

[...]
> Fine, at this point, I don't know when I'll get to this though, its pretty busy
> here at the moment.

This delay is my fault, and I want to help.
If I find a good solution, do you accept I send a version 7 of this patch?

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

[Neil Horman]

On Mon, May 28, 2018 at 12:01:15AM +0200, Thomas Monjalon wrote:
> 27/05/2018 23:00, Neil Horman:
> > On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> > > Hi Neil,
> > > 
> > > Sorry, this patch has been forgotten during the whole release cycle.
> > > 
> > Its ok, though that is frustrating, as we now need to re-familiarize ourselves
> > with how this patch works.
> 
> I understand and apologize.
> 
No worries.

> > > I see an issue about the dependency on filterdiff.
> > > Is there a way to avoid it?
> > > 
> > I suppose, but to do so would require some awk magic.  Is there any advantage to
> > having an awk dependency rather than a filterdiff dependency?  filterdiff is a
> > pretty universal tool.
> 
> filterdiff is not installed on my machine.
> I guess it is the same for a lot of people.
> 
My guess is that awk is only installed on your machine because you needed it for
another script (possibly this one).  Neither is commonly installed by default,
but both are readily available.

> I will check how we can replace it.
> 
No need, I know how I can use awk to replace it, the only question is, do we
need to do it?  I suppose there is an implicit advantage in just using ask, as
we already require it.

All thats needed is an awk program like the following:
awk 'BEGIN {startprint=0;} /.*diff.*map.*/ {startprint=1;} \
	/.*diff.*[^map].*/ {startprint=0 /.*/ {if (startprint) {print $0}}'

its basically a state machine that prints every line in a file after hitting the
regex diff.*map (i.e. a map file), and stops when it hits the next diff block
that isn't for a map file.  The above isn't exactly right, but its close.

> [...]
> > Fine, at this point, I don't know when I'll get to this though, its pretty busy
> > here at the moment.
> 
> This delay is my fault, and I want to help.
> If I find a good solution, do you accept I send a version 7 of this patch?
Sure, if you have the time to take care of it, that would be great, thanks.  And
thank you for asking.  If I find time, I'll take a stab at it as well, but I
think given Red Hats schedule, it will be a few weeks before I'm able.

Best
Neil
> 
> 
> 

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

[Neil Horman]

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
---
 MAINTAINERS                     |   1 +
 devtools/check-symbol-change.sh | 145 ++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  50 +++++++++--
 3 files changed, 189 insertions(+), 7 deletions(-)
 create mode 100755 devtools/check-symbol-change.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 7398749d7..da4735c11 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..76d5562b6
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,145 @@
+#!/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 | filterdiff -i *.map | awk '
+		# Initialize our variables
+		BEGIN {map="";sym="";ar="";sec=""; in_sec=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}
+
+		# 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("+","");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).  Otherwise we flag it as an
+		# unknown section
+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
+			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_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 ithan 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 245d5ab10..2e9f30cc8 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))
 }
 
-- 
2.17.1

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

[Neil Horman]

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

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))
 }
 
-- 
2.17.1

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

[Thomas Monjalon]

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

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

[Neil Horman]

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
> 
> 
> 

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

[Neil Horman]

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)

v9)
 * various nits
---
 MAINTAINERS                     |   1 +
 devtools/check-symbol-change.sh | 159 ++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  47 ++++++++--
 3 files changed, 200 insertions(+), 7 deletions(-)
 create mode 100755 devtools/check-symbol-change.sh

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..17d123cf4
--- /dev/null
+++ b/devtools/check-symbol-change.sh
@@ -0,0 +1,159 @@
+#!/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..c6c7bfae1 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 '^checkpatches\.'
+	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,40 @@ 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
-	[ $? -ne 0 ] || return 0
-	$verbose || printf '\n### %s\n\n' "$3"
-	printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
+
+	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
+	[ $ret -eq 0 ] && return 0
+
 	status=$(($status + 1))
 }
 
-- 
2.17.1

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

[Thomas Monjalon]

27/06/2018 20:01, Neil Horman:
> 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>
> 
> ---
> +		tmpinput=$(mktemp checkpatches.XXXXXX)
> +		git format-patch --find-renames \
> +		--no-stat --stdout -1 $commit > ./$tmpinput

In case $tmpinput is an absolute path (like in /tmp),
we must not prepend it with ./
I fix it when applying.

Applied, thanks

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

[Rao, Nikhil]

On 6/27/2018 11:31 PM, Neil Horman wrote:
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> new file mode 100755
> index 000000000..17d123cf4
> --- /dev/null
> +++ b/devtools/check-symbol-change.sh
> @@ -0,0 +1,159 @@
> +#!/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;
> +			}
> +		}
> +

I am adding a symbol as shown below, however the rule above fails to 
detect that the new symbol is being added to a pre-existing EXPERIMENTAL 
block (picks up the section name as @@ instead).

Any suggestions ?

diff --git a/lib/librte_eventdev/rte_eventdev_version.map 
b/lib/librte_eventdev/rte_eventdev_version.map
index 12835e9..4b8c55d 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -96,6 +96,7 @@ EXPERIMENTAL {
   	rte_event_crypto_adapter_stats_reset;
   	rte_event_crypto_adapter_stop;
   	rte_event_eth_rx_adapter_cb_register;
+	rte_event_eth_tx_adapter_caps_get;
   	rte_event_timer_adapter_caps_get;
   	rte_event_timer_adapter_create;
   	rte_event_timer_adapter_create_ext;

Thanks,
Nikhil

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

[Neil Horman]

On Tue, Aug 14, 2018 at 09:23:59AM +0530, Rao, Nikhil wrote:
> On 6/27/2018 11:31 PM, Neil Horman wrote:
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > new file mode 100755
> > index 000000000..17d123cf4
> > --- /dev/null
> > +++ b/devtools/check-symbol-change.sh
> > @@ -0,0 +1,159 @@
> > +#!/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;
> > +			}
> > +		}
> > +
> 
> I am adding a symbol as shown below, however the rule above fails to detect
> that the new symbol is being added to a pre-existing EXPERIMENTAL block
> (picks up the section name as @@ instead).
> 
> Any suggestions ?
> 
I was about to say that its because you've not got enough context to let the awk
file figure out what your section name is, but that doesn't appear to be the
case. Can you provide the exact command line you are running to do your symbol
check, as well as the full patch that you are checking?  I'd like to try
recreate the issue here

Best
Neil

> diff --git a/lib/librte_eventdev/rte_eventdev_version.map
> b/lib/librte_eventdev/rte_eventdev_version.map
> index 12835e9..4b8c55d 100644
> --- a/lib/librte_eventdev/rte_eventdev_version.map
> +++ b/lib/librte_eventdev/rte_eventdev_version.map
> @@ -96,6 +96,7 @@ EXPERIMENTAL {
>   	rte_event_crypto_adapter_stats_reset;
>   	rte_event_crypto_adapter_stop;
>   	rte_event_eth_rx_adapter_cb_register;
> +	rte_event_eth_tx_adapter_caps_get;
>   	rte_event_timer_adapter_caps_get;
>   	rte_event_timer_adapter_create;
>   	rte_event_timer_adapter_create_ext;
> 
> Thanks,
> Nikhil
> 

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

[Nikhil Rao]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> I was about to say that its because you've not got enough context to let the
> awk file figure out what your section name is, but that doesn't appear to be
> the case. Can you provide the exact command line you are running to do your
> symbol check, as well as the full patch that you are checking?  I'd like to try
> recreate the issue here
> 
> Best
> Neil
> 

Complete patch is below

Thanks,
Nikhil

From: Nikhil Rao <nikhil.rao@intel.com>
Date: Thu, 5 Jul 2018 14:17:16 +0530
Subject: [PATCH v2 3/4] eventdev: add eth Tx adapter implementation

This patch implements the Tx adapter APIs by invoking the
corresponding eventdev PMD callbacks and also provides
the common rte_service function based implementation when
the eventdev PMD support is absent.

Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
---
 config/rte_config.h                            |    1 +
 lib/librte_eventdev/rte_event_eth_tx_adapter.c | 1120 ++++++++++++++++++++++++
 config/common_base                             |    2 +-
 lib/librte_eventdev/Makefile                   |    2 +
 lib/librte_eventdev/meson.build                |    6 +-
 lib/librte_eventdev/rte_eventdev_version.map   |   13 +
 6 files changed, 1141 insertions(+), 3 deletions(-)
 create mode 100644 lib/librte_eventdev/rte_event_eth_tx_adapter.c

diff --git a/config/rte_config.h b/config/rte_config.h
index b1fb8cd..63f51a9 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -66,6 +66,7 @@
 #define RTE_EVENT_TIMER_ADAPTER_NUM_MAX 32
 #define RTE_EVENT_ETH_INTR_RING_SIZE 1024
 #define RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE 32
+#define RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE 32
 
 /* rawdev defines */
 #define RTE_RAWDEV_MAX_DEVS 10
diff --git a/lib/librte_eventdev/rte_event_eth_tx_adapter.c b/lib/librte_eventdev/rte_event_eth_tx_adapter.c
new file mode 100644
index 0000000..05253d4
--- /dev/null
+++ b/lib/librte_eventdev/rte_event_eth_tx_adapter.c
@@ -0,0 +1,1120 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation.
+ */
+#include <rte_spinlock.h>
+#include <rte_service_component.h>
+#include <rte_ethdev.h>
+
+#include "rte_eventdev_pmd.h"
+#include "rte_event_eth_tx_adapter.h"
+
+#define TXA_BATCH_SIZE		32
+#define TXA_SERVICE_NAME_LEN	32
+#define TXA_MEM_NAME_LEN	32
+#define TXA_FLUSH_THRESHOLD	1024
+#define TXA_RETRY_CNT		100
+#define TXA_MAX_NB_TX		128
+#define TXA_INVALID_DEV_ID	INT32_C(-1)
+#define TXA_INVALID_SERVICE_ID	INT64_C(-1)
+
+#define txa_evdev(id) (&rte_eventdevs[txa_dev_id_array[(id)]])
+
+#define txa_dev_caps_get(id) txa_evdev((id))->dev_ops->eth_tx_adapter_caps_get
+
+#define txa_dev_adapter_create(t) txa_evdev(t)->dev_ops->eth_tx_adapter_create
+
+#define txa_dev_adapter_create_ext(t) \
+				txa_evdev(t)->dev_ops->eth_tx_adapter_create
+
+#define txa_dev_adapter_free(t) txa_evdev(t)->dev_ops->eth_tx_adapter_free
+
+#define txa_dev_queue_add(id) txa_evdev(id)->dev_ops->eth_tx_adapter_queue_add
+
+#define txa_dev_queue_del(t) txa_evdev(t)->dev_ops->eth_tx_adapter_queue_del
+
+#define txa_dev_start(t) txa_evdev(t)->dev_ops->eth_tx_adapter_start
+
+#define txa_dev_stop(t) txa_evdev(t)->dev_ops->eth_tx_adapter_stop
+
+#define txa_dev_stats_reset(t) txa_evdev(t)->dev_ops->eth_tx_adapter_stats_reset
+
+#define txa_dev_stats_get(t) txa_evdev(t)->dev_ops->eth_tx_adapter_stats_get
+
+#define RTE_EVENT_ETH_TX_ADAPTER_ID_VALID_OR_ERR_RET(id, retval) \
+do { \
+	if (!txa_valid_id(id)) { \
+		RTE_EDEV_LOG_ERR("Invalid eth Rx adapter id = %d", id); \
+		return retval; \
+	} \
+} while (0)
+
+#define TXA_CHECK_OR_ERR_RET(id) \
+do {\
+	int ret; \
+	RTE_EVENT_ETH_TX_ADAPTER_ID_VALID_OR_ERR_RET((id), -EINVAL); \
+	ret = txa_init(); \
+	if (ret != 0) \
+		return ret; \
+	if (!txa_adapter_exist((id))) \
+		return -EINVAL; \
+} while (0)
+
+/* Tx retry callback structure */
+struct txa_retry {
+	/* Ethernet port id */
+	uint16_t port_id;
+	/* Tx queue */
+	uint16_t tx_queue;
+	/* Adapter ID */
+	uint8_t id;
+};
+
+/* Per queue structure */
+struct txa_service_queue_info {
+	/* Queue has been added */
+	uint8_t added;
+	/* Retry callback argument */
+	struct txa_retry txa_retry;
+	/* Tx buffer */
+	struct rte_eth_dev_tx_buffer *tx_buf;
+};
+
+/* PMD private structure */
+struct txa_service_data {
+	/* Max mbufs processed in any service function invocation */
+	uint32_t max_nb_tx;
+	/* Number of Tx queues in adapter */
+	uint32_t nb_queues;
+	/*  Synchronization with data path */
+	rte_spinlock_t tx_lock;
+	/* Event port ID */
+	uint8_t port_id;
+	/* Event device identifier */
+	uint8_t eventdev_id;
+	/* Highest port id supported + 1 */
+	uint16_t dev_count;
+	/* Loop count to flush Tx buffers */
+	int loop_cnt;
+	/* Per ethernet device structure */
+	struct txa_service_ethdev *txa_ethdev;
+	/* Statistics */
+	struct rte_event_eth_tx_adapter_stats stats;
+	/* Adapter Identifier */
+	uint8_t id;
+	/* Conf arg must be freed */
+	uint8_t conf_free;
+	/* Configuration callback */
+	rte_event_eth_tx_adapter_conf_cb conf_cb;
+	/* Configuration callback argument */
+	void *conf_arg;
+	/* socket id */
+	int socket_id;
+	/* Per adapter EAL service */
+	int64_t service_id;
+	/* Memory allocation name */
+	char mem_name[TXA_MEM_NAME_LEN];
+} __rte_cache_aligned;
+
+/* Per eth device structure */
+struct txa_service_ethdev {
+	/* Pointer to ethernet device */
+	struct rte_eth_dev *dev;
+	/* Number of queues added */
+	uint16_t nb_queues;
+	/* PMD specific queue data */
+	void *queues;
+};
+
+/* Array of adapter instances, initialized with event device id
+ * when adapter is created
+ */
+static int *txa_dev_id_array;
+
+/* Array of pointers to service implementation data */
+static struct txa_service_data **txa_service_data_array;
+
+static int32_t txa_service_func(void *args);
+static int txa_service_adapter_create_ext(uint8_t id,
+			struct rte_eventdev *dev,
+			rte_event_eth_tx_adapter_conf_cb conf_cb,
+			void *conf_arg);
+static int txa_service_queue_del(uint8_t id,
+				const struct rte_eth_dev *dev,
+				int32_t tx_queue_id);
+
+static int
+txa_adapter_exist(uint8_t id)
+{
+	return txa_dev_id_array[id] != TXA_INVALID_DEV_ID;
+}
+
+static inline int
+txa_valid_id(uint8_t id)
+{
+	return id < RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE;
+}
+
+static void *
+txa_memzone_array_get(const char *name, unsigned int elt_size, int nb_elems)
+{
+	const struct rte_memzone *mz;
+	unsigned int sz;
+
+	sz = elt_size * nb_elems;
+	sz = RTE_ALIGN(sz, RTE_CACHE_LINE_SIZE);
+
+	mz = rte_memzone_lookup(name);
+	if (mz == NULL) {
+		mz = rte_memzone_reserve_aligned(name, sz, rte_socket_id(), 0,
+						 RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			RTE_EDEV_LOG_ERR("failed to reserve memzone"
+					" name = %s err = %"
+					PRId32, name, rte_errno);
+			return NULL;
+		}
+	}
+
+	return  mz->addr;
+}
+
+static int
+txa_dev_id_array_init(void)
+{
+	if (txa_dev_id_array == NULL) {
+		int i;
+
+		txa_dev_id_array = txa_memzone_array_get("txa_adapter_array",
+					sizeof(int),
+					RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE);
+		if (txa_dev_id_array == NULL)
+			return -ENOMEM;
+
+		for (i = 0; i < RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE; i++)
+			txa_dev_id_array[i] = TXA_INVALID_DEV_ID;
+	}
+
+	return 0;
+}
+
+static int
+txa_init(void)
+{
+	return txa_dev_id_array_init();
+}
+
+static int
+txa_service_data_init(void)
+{
+	if (txa_service_data_array == NULL) {
+		txa_service_data_array =
+				txa_memzone_array_get("txa_service_data_array",
+					sizeof(int),
+					RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE);
+		if (txa_service_data_array == NULL)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static inline struct txa_service_data *
+txa_service_id_to_data(uint8_t id)
+{
+	return txa_service_data_array[id];
+}
+
+static inline struct txa_service_queue_info *
+txa_service_queue(struct txa_service_data *txa, uint16_t port_id,
+		uint16_t tx_queue_id)
+{
+	struct txa_service_queue_info *tqi;
+
+	if (unlikely(txa->txa_ethdev == NULL || txa->dev_count < port_id + 1))
+		return NULL;
+
+	tqi = txa->txa_ethdev[port_id].queues;
+
+	return likely(tqi != NULL) ? tqi + tx_queue_id : NULL;
+}
+
+static int
+txa_service_conf_cb(uint8_t __rte_unused id, uint8_t dev_id,
+		struct rte_event_eth_tx_adapter_conf *conf, void *arg)
+{
+	int ret;
+	struct rte_eventdev *dev;
+	struct rte_event_port_conf *pc;
+	struct rte_event_dev_config dev_conf;
+	int started;
+	uint8_t port_id;
+
+	pc = arg;
+	dev = &rte_eventdevs[dev_id];
+	dev_conf = dev->data->dev_conf;
+
+	started = dev->data->dev_started;
+	if (started)
+		rte_event_dev_stop(dev_id);
+
+	port_id = dev_conf.nb_event_ports;
+	dev_conf.nb_event_ports += 1;
+
+	ret = rte_event_dev_configure(dev_id, &dev_conf);
+	if (ret) {
+		RTE_EDEV_LOG_ERR("failed to configure event dev %u",
+						dev_id);
+		if (started) {
+			if (rte_event_dev_start(dev_id))
+				return -EIO;
+		}
+		return ret;
+	}
+
+	pc->disable_implicit_release = 0;
+	ret = rte_event_port_setup(dev_id, port_id, pc);
+	if (ret) {
+		RTE_EDEV_LOG_ERR("failed to setup event port %u\n",
+					port_id);
+		if (started) {
+			if (rte_event_dev_start(dev_id))
+				return -EIO;
+		}
+		return ret;
+	}
+
+	conf->event_port_id = port_id;
+	conf->max_nb_tx = TXA_MAX_NB_TX;
+	if (started)
+		ret = rte_event_dev_start(dev_id);
+	return ret;
+}
+
+static int
+txa_service_ethdev_alloc(struct txa_service_data *txa)
+{
+	struct txa_service_ethdev *txa_ethdev;
+	uint16_t i, dev_count;
+
+	dev_count = rte_eth_dev_count_avail();
+	if (txa->txa_ethdev && dev_count == txa->dev_count)
+		return 0;
+
+	txa_ethdev = rte_zmalloc_socket(txa->mem_name,
+					dev_count * sizeof(*txa_ethdev),
+					0,
+					txa->socket_id);
+	if (txa_ethdev == NULL) {
+		RTE_EDEV_LOG_ERR("Failed to alloc txa::txa_ethdev ");
+		return -ENOMEM;
+	}
+
+	if (txa->dev_count)
+		memcpy(txa_ethdev, txa->txa_ethdev,
+			txa->dev_count * sizeof(*txa_ethdev));
+
+	RTE_ETH_FOREACH_DEV(i) {
+		if (i == dev_count)
+			break;
+		txa_ethdev[i].dev = &rte_eth_devices[i];
+	}
+
+	txa->txa_ethdev = txa_ethdev;
+	txa->dev_count = dev_count;
+	return 0;
+}
+
+static int
+txa_service_queue_array_alloc(struct txa_service_data *txa,
+			uint16_t port_id)
+{
+	struct txa_service_queue_info *tqi;
+	uint16_t nb_queue;
+	int ret;
+
+	ret = txa_service_ethdev_alloc(txa);
+	if (ret != 0)
+		return ret;
+
+	if (txa->txa_ethdev[port_id].queues)
+		return 0;
+
+	nb_queue = txa->txa_ethdev[port_id].dev->data->nb_tx_queues;
+	tqi = rte_zmalloc_socket(txa->mem_name,
+				nb_queue *
+				sizeof(struct txa_service_queue_info), 0,
+				txa->socket_id);
+	if (tqi == NULL)
+		return -ENOMEM;
+	txa->txa_ethdev[port_id].queues = tqi;
+	return 0;
+}
+
+static void
+txa_service_queue_array_free(struct txa_service_data *txa,
+			uint16_t port_id)
+{
+	struct txa_service_ethdev *txa_ethdev;
+	struct txa_service_queue_info *tqi;
+
+	txa_ethdev = &txa->txa_ethdev[port_id];
+	if (txa->txa_ethdev == NULL || txa_ethdev->nb_queues != 0)
+		return;
+
+	tqi = txa_ethdev->queues;
+	txa_ethdev->queues = NULL;
+	rte_free(tqi);
+
+	if (txa->nb_queues == 0) {
+		rte_free(txa->txa_ethdev);
+		txa->txa_ethdev = NULL;
+	}
+}
+
+static void
+txa_service_unregister(struct txa_service_data *txa)
+{
+	if (txa->service_id != TXA_INVALID_SERVICE_ID) {
+		rte_service_component_runstate_set(txa->service_id, 0);
+		while (rte_service_may_be_active(txa->service_id))
+			rte_pause();
+		rte_service_component_unregister(txa->service_id);
+	}
+	txa->service_id = TXA_INVALID_SERVICE_ID;
+}
+
+static int
+txa_service_register(struct txa_service_data *txa)
+{
+	int ret;
+	struct rte_service_spec service;
+	struct rte_event_eth_tx_adapter_conf conf;
+
+	if (txa->service_id != TXA_INVALID_SERVICE_ID)
+		return 0;
+
+	memset(&service, 0, sizeof(service));
+	snprintf(service.name, TXA_SERVICE_NAME_LEN, "txa_%d", txa->id);
+	service.socket_id = txa->socket_id;
+	service.callback = txa_service_func;
+	service.callback_userdata = txa;
+	service.capabilities = RTE_SERVICE_CAP_MT_SAFE;
+	ret = rte_service_component_register(&service,
+					(uint32_t *)&txa->service_id);
+	if (ret) {
+		RTE_EDEV_LOG_ERR("failed to register service %s err = %"
+				 PRId32, service.name, ret);
+		return ret;
+	}
+
+	ret = txa->conf_cb(txa->id, txa->eventdev_id, &conf, txa->conf_arg);
+	if (ret) {
+		txa_service_unregister(txa);
+		return ret;
+	}
+
+	rte_service_component_runstate_set(txa->service_id, 1);
+	txa->port_id = conf.event_port_id;
+	txa->max_nb_tx = conf.max_nb_tx;
+	return 0;
+}
+
+static struct rte_eth_dev_tx_buffer *
+txa_service_tx_buf_alloc(struct txa_service_data *txa,
+			const struct rte_eth_dev *dev)
+{
+	struct rte_eth_dev_tx_buffer *tb;
+	uint16_t port_id;
+
+	port_id = dev->data->port_id;
+	tb = rte_zmalloc_socket(txa->mem_name,
+				RTE_ETH_TX_BUFFER_SIZE(TXA_BATCH_SIZE),
+				0,
+				rte_eth_dev_socket_id(port_id));
+	if (tb == NULL)
+		RTE_EDEV_LOG_ERR("Failed to allocate memory for tx buffer");
+	return tb;
+}
+
+static int
+txa_service_is_queue_added(struct txa_service_data *txa,
+			const struct rte_eth_dev *dev,
+			uint16_t tx_queue_id)
+{
+	struct txa_service_queue_info *tqi;
+
+	tqi = txa_service_queue(txa, dev->data->port_id, tx_queue_id);
+	return tqi && tqi->added;
+}
+
+static int
+txa_service_ctrl(uint8_t id, int start)
+{
+	int ret;
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	if (txa->service_id == TXA_INVALID_SERVICE_ID)
+		return 0;
+
+	ret = rte_service_runstate_set(txa->service_id, start);
+	if (ret == 0 && !start) {
+		while (rte_service_may_be_active(txa->service_id))
+			rte_pause();
+	}
+	return ret;
+}
+
+static void
+txa_service_buffer_retry(struct rte_mbuf **pkts, uint16_t unsent,
+			void *userdata)
+{
+	struct txa_retry *tr;
+	struct txa_service_data *data;
+	struct rte_event_eth_tx_adapter_stats *stats;
+	uint16_t sent = 0;
+	unsigned int retry = 0;
+	uint16_t i, n;
+
+	tr = (struct txa_retry *)(uintptr_t)userdata;
+	data = txa_service_id_to_data(tr->id);
+	stats = &data->stats;
+
+	do {
+		n = rte_eth_tx_burst(tr->port_id, tr->tx_queue,
+			       &pkts[sent], unsent - sent);
+
+		sent += n;
+	} while (sent != unsent && retry++ < TXA_RETRY_CNT);
+
+	for (i = sent; i < unsent; i++)
+		rte_pktmbuf_free(pkts[i]);
+
+	stats->tx_retry += retry;
+	stats->tx_packets += sent;
+	stats->tx_dropped += unsent - sent;
+}
+
+static void
+txa_service_tx(struct txa_service_data *txa, struct rte_event *ev,
+	uint32_t n)
+{
+	uint32_t i;
+	uint16_t nb_tx;
+	struct rte_event_eth_tx_adapter_stats *stats;
+
+	stats = &txa->stats;
+
+	nb_tx = 0;
+	for (i = 0; i < n; i++) {
+		struct rte_mbuf *m;
+		uint16_t port;
+		uint16_t queue;
+		struct txa_service_queue_info *tqi;
+
+		m = ev[i].mbuf;
+		port = m->port;
+		queue = rte_event_eth_tx_adapter_txq_get(m);
+
+		tqi = txa_service_queue(txa, port, queue);
+		if (unlikely(tqi == NULL || !tqi->added)) {
+			rte_pktmbuf_free(m);
+			continue;
+		}
+
+		nb_tx += rte_eth_tx_buffer(port, queue, tqi->tx_buf, m);
+	}
+
+	stats->tx_packets += nb_tx;
+}
+
+static int32_t
+txa_service_func(void *args)
+{
+	struct txa_service_data *txa = args;
+	uint8_t dev_id;
+	uint8_t port;
+	uint16_t n;
+	uint32_t nb_tx, max_nb_tx;
+	struct rte_event ev[TXA_BATCH_SIZE];
+
+	dev_id = txa->eventdev_id;
+	max_nb_tx = txa->max_nb_tx;
+	port = txa->port_id;
+
+	if (txa->nb_queues == 0)
+		return 0;
+
+	if (!rte_spinlock_trylock(&txa->tx_lock))
+		return 0;
+
+	for (nb_tx = 0; nb_tx < max_nb_tx; nb_tx += n) {
+
+		n = rte_event_dequeue_burst(dev_id, port, ev, RTE_DIM(ev), 0);
+		if (!n)
+			break;
+		txa_service_tx(txa, ev, n);
+	}
+
+	if ((txa->loop_cnt++ & (TXA_FLUSH_THRESHOLD - 1)) == 0) {
+
+		struct txa_service_ethdev *tdi;
+		struct txa_service_queue_info *tqi;
+		struct rte_eth_dev *dev;
+		uint16_t i;
+
+		tdi = txa->txa_ethdev;
+		nb_tx = 0;
+
+		RTE_ETH_FOREACH_DEV(i) {
+			uint16_t q;
+
+			if (i == txa->dev_count)
+				break;
+
+			dev = tdi[i].dev;
+			if (tdi[i].nb_queues == 0)
+				continue;
+			for (q = 0; q < dev->data->nb_tx_queues; q++) {
+
+				tqi = txa_service_queue(txa, i, q);
+				if (unlikely(tqi == NULL || !tqi->added))
+					continue;
+
+				nb_tx += rte_eth_tx_buffer_flush(i, q,
+							tqi->tx_buf);
+			}
+		}
+
+		txa->stats.tx_packets += nb_tx;
+	}
+	rte_spinlock_unlock(&txa->tx_lock);
+	return 0;
+}
+
+static int
+txa_service_adapter_create(uint8_t id, struct rte_eventdev *dev,
+			struct rte_event_port_conf *port_conf)
+{
+	struct txa_service_data *txa;
+	struct rte_event_port_conf *cb_conf;
+	int ret;
+
+	cb_conf = rte_malloc(NULL, sizeof(*cb_conf), 0);
+	if (cb_conf == NULL)
+		return -ENOMEM;
+
+	*cb_conf = *port_conf;
+	ret = txa_service_adapter_create_ext(id, dev, txa_service_conf_cb,
+					cb_conf);
+	if (ret) {
+		rte_free(cb_conf);
+		return ret;
+	}
+
+	txa = txa_service_id_to_data(id);
+	txa->conf_free = 1;
+	return ret;
+}
+
+static int
+txa_service_adapter_create_ext(uint8_t id, struct rte_eventdev *dev,
+			rte_event_eth_tx_adapter_conf_cb conf_cb,
+			void *conf_arg)
+{
+	struct txa_service_data *txa;
+	int socket_id;
+	char mem_name[TXA_SERVICE_NAME_LEN];
+	int ret;
+
+	if (conf_cb == NULL)
+		return -EINVAL;
+
+	socket_id = dev->data->socket_id;
+	snprintf(mem_name, TXA_MEM_NAME_LEN,
+		"rte_event_eth_txa_%d",
+		id);
+
+	ret = txa_service_data_init();
+	if (ret != 0)
+		return ret;
+
+	txa = rte_zmalloc_socket(mem_name,
+				sizeof(*txa),
+				RTE_CACHE_LINE_SIZE, socket_id);
+	if (txa == NULL) {
+		RTE_EDEV_LOG_ERR("failed to get mem for tx adapter");
+		return -ENOMEM;
+	}
+
+	txa->id = id;
+	txa->eventdev_id = dev->data->dev_id;
+	txa->socket_id = socket_id;
+	strncpy(txa->mem_name, mem_name, TXA_SERVICE_NAME_LEN);
+	txa->conf_cb = conf_cb;
+	txa->conf_arg = conf_arg;
+	txa->service_id = TXA_INVALID_SERVICE_ID;
+	rte_spinlock_init(&txa->tx_lock);
+	txa_service_data_array[id] = txa;
+
+	return 0;
+}
+
+static int
+txa_service_event_port_get(uint8_t id, uint8_t *port)
+{
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	if (txa->service_id == TXA_INVALID_SERVICE_ID)
+		return -ENODEV;
+
+	*port = txa->port_id;
+	return 0;
+}
+
+static int
+txa_service_adapter_free(uint8_t id)
+{
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	if (txa->nb_queues) {
+		RTE_EDEV_LOG_ERR("%" PRIu16 " Tx queues not deleted",
+				txa->nb_queues);
+		return -EBUSY;
+	}
+
+	if (txa->conf_free)
+		rte_free(txa->conf_arg);
+	rte_free(txa);
+	return 0;
+}
+
+static int
+txa_service_queue_add(uint8_t id,
+		__rte_unused struct rte_eventdev *dev,
+		const struct rte_eth_dev *eth_dev,
+		int32_t tx_queue_id)
+{
+	struct txa_service_data *txa;
+	struct txa_service_ethdev *tdi;
+	struct txa_service_queue_info *tqi;
+	struct rte_eth_dev_tx_buffer *tb;
+	struct txa_retry *txa_retry;
+	int ret;
+
+	txa = txa_service_id_to_data(id);
+
+	if (tx_queue_id == -1) {
+		int nb_queues;
+		uint16_t i, j;
+		uint16_t *qdone;
+
+		nb_queues = eth_dev->data->nb_tx_queues;
+		if (txa->dev_count > eth_dev->data->port_id) {
+			tdi = &txa->txa_ethdev[eth_dev->data->port_id];
+			nb_queues -= tdi->nb_queues;
+		}
+
+		qdone = rte_zmalloc(txa->mem_name,
+				nb_queues * sizeof(*qdone), 0);
+		j = 0;
+		for (i = 0; i < nb_queues; i++) {
+			if (txa_service_is_queue_added(txa, eth_dev, i))
+				continue;
+			ret = txa_service_queue_add(id, dev, eth_dev, i);
+			if (ret == 0)
+				qdone[j++] = i;
+			else
+				break;
+		}
+
+		if (i != nb_queues) {
+			for (i = 0; i < j; i++)
+				txa_service_queue_del(id, eth_dev, qdone[i]);
+		}
+		rte_free(qdone);
+		return ret;
+	}
+
+	ret = txa_service_register(txa);
+	if (ret)
+		return ret;
+
+	rte_spinlock_lock(&txa->tx_lock);
+
+	if (txa_service_is_queue_added(txa, eth_dev, tx_queue_id)) {
+		rte_spinlock_unlock(&txa->tx_lock);
+		return 0;
+	}
+
+	ret = txa_service_queue_array_alloc(txa, eth_dev->data->port_id);
+	if (ret)
+		goto err_unlock;
+
+	tb = txa_service_tx_buf_alloc(txa, eth_dev);
+	if (tb == NULL)
+		goto err_unlock;
+
+	tdi = &txa->txa_ethdev[eth_dev->data->port_id];
+	tqi = txa_service_queue(txa, eth_dev->data->port_id, tx_queue_id);
+
+	txa_retry = &tqi->txa_retry;
+	txa_retry->id = txa->id;
+	txa_retry->port_id = eth_dev->data->port_id;
+	txa_retry->tx_queue = tx_queue_id;
+
+	rte_eth_tx_buffer_init(tb, TXA_BATCH_SIZE);
+	rte_eth_tx_buffer_set_err_callback(tb,
+		txa_service_buffer_retry, txa_retry);
+
+	tqi->tx_buf = tb;
+	tqi->added = 1;
+	tdi->nb_queues++;
+	txa->nb_queues++;
+
+err_unlock:
+	if (txa->nb_queues == 0) {
+		txa_service_queue_array_free(txa,
+					eth_dev->data->port_id);
+		txa_service_unregister(txa);
+	}
+
+	rte_spinlock_unlock(&txa->tx_lock);
+	return 0;
+}
+
+static int
+txa_service_queue_del(uint8_t id,
+		const struct rte_eth_dev *dev,
+		int32_t tx_queue_id)
+{
+	struct txa_service_data *txa;
+	struct txa_service_queue_info *tqi;
+	struct rte_eth_dev_tx_buffer *tb;
+	uint16_t port_id;
+
+	if (tx_queue_id == -1) {
+		uint16_t i;
+		int ret;
+
+		for (i = 0; i < dev->data->nb_tx_queues; i++) {
+			ret = txa_service_queue_del(id, dev, i);
+			if (ret != 0)
+				break;
+		}
+		return ret;
+	}
+
+	txa = txa_service_id_to_data(id);
+	port_id = dev->data->port_id;
+
+	tqi = txa_service_queue(txa, port_id, tx_queue_id);
+	if (tqi == NULL || !tqi->added)
+		return 0;
+
+	tb = tqi->tx_buf;
+	tqi->added = 0;
+	tqi->tx_buf = NULL;
+	rte_free(tb);
+	txa->nb_queues--;
+	txa->txa_ethdev[port_id].nb_queues--;
+
+	txa_service_queue_array_free(txa, port_id);
+	return 0;
+}
+
+static int
+txa_service_id_get(uint8_t id, uint32_t *service_id)
+{
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	if (txa->service_id == TXA_INVALID_SERVICE_ID)
+		return -EINVAL;
+
+	*service_id = txa->service_id;
+	return 0;
+}
+
+static int
+txa_service_start(uint8_t id)
+{
+	return txa_service_ctrl(id, 1);
+}
+
+static int
+txa_service_stats_get(uint8_t id,
+		struct rte_event_eth_tx_adapter_stats *stats)
+{
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	*stats = txa->stats;
+	return 0;
+}
+
+static int
+txa_service_stats_reset(uint8_t id)
+{
+	struct txa_service_data *txa;
+
+	txa = txa_service_id_to_data(id);
+	memset(&txa->stats, 0, sizeof(txa->stats));
+	return 0;
+}
+
+static int
+txa_service_stop(uint8_t id)
+{
+	return txa_service_ctrl(id, 0);
+}
+
+
+int __rte_experimental
+rte_event_eth_tx_adapter_create(uint8_t id, uint8_t dev_id,
+				struct rte_event_port_conf *port_conf)
+{
+	struct rte_eventdev *dev;
+	int ret;
+
+	if (port_conf == NULL)
+		return -EINVAL;
+
+	RTE_EVENT_ETH_TX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	dev = &rte_eventdevs[dev_id];
+
+	ret = txa_init();
+	if (ret != 0)
+		return ret;
+
+	if (txa_adapter_exist(id))
+		return -EEXIST;
+
+	txa_dev_id_array[id] = dev_id;
+	if (txa_dev_adapter_create(id))
+		ret = txa_dev_adapter_create(id)(id, dev);
+
+	if (ret != 0) {
+		txa_dev_id_array[id] = TXA_INVALID_DEV_ID;
+		return ret;
+	}
+
+	ret = txa_service_adapter_create(id, dev, port_conf);
+	if (ret != 0) {
+		if (txa_dev_adapter_free(id))
+			txa_dev_adapter_free(id)(id, dev);
+		txa_dev_id_array[id] = TXA_INVALID_DEV_ID;
+		return ret;
+	}
+
+	txa_dev_id_array[id] = dev_id;
+	return 0;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_create_ext(uint8_t id, uint8_t dev_id,
+				rte_event_eth_tx_adapter_conf_cb conf_cb,
+				void *conf_arg)
+{
+	struct rte_eventdev *dev;
+	int ret;
+
+	RTE_EVENT_ETH_TX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	ret = txa_init();
+	if (ret != 0)
+		return ret;
+
+	if (txa_adapter_exist(id))
+		return -EINVAL;
+
+	dev = &rte_eventdevs[dev_id];
+
+	txa_dev_id_array[id] = dev_id;
+	if (txa_dev_adapter_create_ext(id))
+		ret = txa_dev_adapter_create_ext(id)(id, dev);
+
+	if (ret != 0) {
+		txa_dev_id_array[id] = TXA_INVALID_DEV_ID;
+		return ret;
+	}
+
+	ret = txa_service_adapter_create_ext(id, dev, conf_cb, conf_arg);
+	if (ret != 0) {
+		if (txa_dev_adapter_free(id))
+			txa_dev_adapter_free(id)(id, dev);
+		txa_dev_id_array[id] = TXA_INVALID_DEV_ID;
+		return ret;
+	}
+
+	txa_dev_id_array[id] = dev_id;
+	return 0;
+}
+
+
+int __rte_experimental
+rte_event_eth_tx_adapter_event_port_get(uint8_t id, uint8_t *event_port_id)
+{
+	TXA_CHECK_OR_ERR_RET(id);
+
+	return txa_service_event_port_get(id, event_port_id);
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_free(uint8_t id)
+{
+	int ret;
+
+	TXA_CHECK_OR_ERR_RET(id);
+
+	ret = txa_dev_adapter_free(id) ?
+		txa_dev_adapter_free(id)(id, txa_evdev(id)) :
+		0;
+
+	if (ret == 0)
+		ret = txa_service_adapter_free(id);
+	txa_dev_id_array[id] = TXA_INVALID_DEV_ID;
+
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_queue_add(uint8_t id,
+				uint16_t eth_dev_id,
+				int32_t queue)
+{
+	struct rte_eth_dev *eth_dev;
+	int ret;
+	uint32_t caps;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL);
+	TXA_CHECK_OR_ERR_RET(id);
+
+	eth_dev = &rte_eth_devices[eth_dev_id];
+	if (queue != -1 && (uint16_t)queue >= eth_dev->data->nb_tx_queues) {
+		RTE_EDEV_LOG_ERR("Invalid tx queue_id %" PRIu16,
+				(uint16_t)queue);
+		return -EINVAL;
+	}
+
+	caps = 0;
+	if (txa_dev_caps_get(id))
+		txa_dev_caps_get(id)(txa_evdev(id), eth_dev, &caps);
+
+	if (caps & RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT)
+		ret =  txa_dev_queue_add(id) ?
+					txa_dev_queue_add(id)(id,
+							txa_evdev(id),
+							eth_dev,
+							queue) : 0;
+	else
+		ret = txa_service_queue_add(id, txa_evdev(id), eth_dev, queue);
+
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_queue_del(uint8_t id,
+				uint16_t eth_dev_id,
+				int32_t queue)
+{
+	struct rte_eth_dev *eth_dev;
+	int ret;
+	uint32_t caps;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL);
+	TXA_CHECK_OR_ERR_RET(id);
+
+	eth_dev = &rte_eth_devices[eth_dev_id];
+	if (queue != -1 && (uint16_t)queue >= eth_dev->data->nb_tx_queues) {
+		RTE_EDEV_LOG_ERR("Invalid tx queue_id %" PRIu16,
+				(uint16_t)queue);
+		return -EINVAL;
+	}
+
+	caps = 0;
+
+	if (txa_dev_caps_get(id))
+		txa_dev_caps_get(id)(txa_evdev(id), eth_dev, &caps);
+
+	if (caps & RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT)
+		ret =  txa_dev_queue_del(id) ?
+					txa_dev_queue_del(id)(id, txa_evdev(id),
+							eth_dev,
+							queue) : 0;
+	else
+		ret = txa_service_queue_del(id, eth_dev, queue);
+
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_service_id_get(uint8_t id, uint32_t *service_id)
+{
+	TXA_CHECK_OR_ERR_RET(id);
+
+	return txa_service_id_get(id, service_id);
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_start(uint8_t id)
+{
+	int ret;
+
+	TXA_CHECK_OR_ERR_RET(id);
+
+	ret = txa_dev_start(id) ? txa_dev_start(id)(id, txa_evdev(id)) : 0;
+	if (ret == 0)
+		ret = txa_service_start(id);
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_stats_get(uint8_t id,
+				struct rte_event_eth_tx_adapter_stats *stats)
+{
+	int ret;
+
+	TXA_CHECK_OR_ERR_RET(id);
+
+	if (stats == NULL)
+		return -EINVAL;
+
+	ret = txa_dev_stats_get(id) ?
+			txa_dev_stats_get(id)(id, txa_evdev(id), stats) : 0;
+	if (ret == 0)
+		ret = txa_service_stats_get(id, stats);
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_stats_reset(uint8_t id)
+{
+	int ret;
+
+	TXA_CHECK_OR_ERR_RET(id);
+
+	ret = txa_dev_stats_reset(id) ?
+		txa_dev_stats_reset(id)(id, txa_evdev(id)) : 0;
+	if (ret == 0)
+		ret = txa_service_stats_reset(id);
+	return ret;
+}
+
+int __rte_experimental
+rte_event_eth_tx_adapter_stop(uint8_t id)
+{
+	int ret;
+
+	TXA_CHECK_OR_ERR_RET(id);
+
+	ret = txa_dev_stop(id) ? txa_dev_stop(id)(id,  txa_evdev(id)) : 0;
+	if (ret == 0)
+		ret = txa_service_stop(id);
+	return ret;
+}
diff --git a/config/common_base b/config/common_base
index 4a51aa9..ffef6f4 100644
--- a/config/common_base
+++ b/config/common_base
@@ -594,7 +594,7 @@ CONFIG_RTE_EVENT_MAX_QUEUES_PER_DEV=64
 CONFIG_RTE_EVENT_TIMER_ADAPTER_NUM_MAX=32
 CONFIG_RTE_EVENT_ETH_INTR_RING_SIZE=1024
 CONFIG_RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE=32
-
+CONFIG_RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE=32
 #
 # Compile PMD for skeleton event device
 #
diff --git a/lib/librte_eventdev/Makefile b/lib/librte_eventdev/Makefile
index 47f599a..424ff35 100644
--- a/lib/librte_eventdev/Makefile
+++ b/lib/librte_eventdev/Makefile
@@ -28,6 +28,7 @@ SRCS-y += rte_event_ring.c
 SRCS-y += rte_event_eth_rx_adapter.c
 SRCS-y += rte_event_timer_adapter.c
 SRCS-y += rte_event_crypto_adapter.c
+SRCS-y += rte_event_eth_tx_adapter.c
 
 # export include files
 SYMLINK-y-include += rte_eventdev.h
@@ -39,6 +40,7 @@ SYMLINK-y-include += rte_event_eth_rx_adapter.h
 SYMLINK-y-include += rte_event_timer_adapter.h
 SYMLINK-y-include += rte_event_timer_adapter_pmd.h
 SYMLINK-y-include += rte_event_crypto_adapter.h
+SYMLINK-y-include += rte_event_eth_tx_adapter.h
 
 # versioning export map
 EXPORT_MAP := rte_eventdev_version.map
diff --git a/lib/librte_eventdev/meson.build b/lib/librte_eventdev/meson.build
index 3cbaf29..47989e7 100644
--- a/lib/librte_eventdev/meson.build
+++ b/lib/librte_eventdev/meson.build
@@ -14,7 +14,8 @@ sources = files('rte_eventdev.c',
 		'rte_event_ring.c',
 		'rte_event_eth_rx_adapter.c',
 		'rte_event_timer_adapter.c',
-		'rte_event_crypto_adapter.c')
+		'rte_event_crypto_adapter.c',
+		'rte_event_eth_tx_adapter.c')
 headers = files('rte_eventdev.h',
 		'rte_eventdev_pmd.h',
 		'rte_eventdev_pmd_pci.h',
@@ -23,5 +24,6 @@ headers = files('rte_eventdev.h',
 		'rte_event_eth_rx_adapter.h',
 		'rte_event_timer_adapter.h',
 		'rte_event_timer_adapter_pmd.h',
-		'rte_event_crypto_adapter.h')
+		'rte_event_crypto_adapter.h',
+		'rte_event_eth_tx_adapter.h')
 deps += ['ring', 'ethdev', 'hash', 'mempool', 'mbuf', 'timer', 'cryptodev']
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 12835e9..47e2898 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -96,6 +96,18 @@ EXPERIMENTAL {
 	rte_event_crypto_adapter_stats_reset;
 	rte_event_crypto_adapter_stop;
 	rte_event_eth_rx_adapter_cb_register;
+	rte_event_eth_tx_adapter_caps_get;
+	rte_event_eth_tx_adapter_create;
+	rte_event_eth_tx_adapter_create_ext;
+	rte_event_eth_tx_adapter_event_port_get;
+	rte_event_eth_tx_adapter_free;
+	rte_event_eth_tx_adapter_queue_add;
+	rte_event_eth_tx_adapter_queue_del;
+	rte_event_eth_tx_adapter_service_id_get;
+	rte_event_eth_tx_adapter_start;
+	rte_event_eth_tx_adapter_stats_get;
+	rte_event_eth_tx_adapter_stats_reset;
+	rte_event_eth_tx_adapter_stop;
 	rte_event_timer_adapter_caps_get;
 	rte_event_timer_adapter_create;
 	rte_event_timer_adapter_create_ext;
-- 
1.8.3.1

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

[Neil Horman]

On Wed, Aug 15, 2018 at 11:40:42AM +0530, Nikhil Rao wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > I was about to say that its because you've not got enough context to let the
> > awk file figure out what your section name is, but that doesn't appear to be
> > the case. Can you provide the exact command line you are running to do your
> > symbol check, as well as the full patch that you are checking?  I'd like to try
> > recreate the issue here
> > 
> > Best
> > Neil
> > 
> 
> Complete patch is below
> 
> 

Thanks, I think I made a mistake in how I detect section names in the awk
script.  The rule assumes that the entire section is getting added (i.e. we are
adding the EXPERIMENTAL section as a whole unit, hence the starting a line with
+ to id the section name, and thats not the case here.  I think the rule needs
to be any line in a map file that ends with a { (based on our coding practice),
is a section start, and the section name is the next to the last field in the
line (i.e. $(NF-1) ).  Please apply the patch below and confirm that this works
for you.

Best
Neil



diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index daaf45e14..cf9cfc745 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -25,14 +25,14 @@ build_map_changes()
 		# supresses the subordonate rules below
 		/[-+] a\/.*\.^(map)/ {in_map=0}
 
-		# Triggering this rule, which starts a line with a + and ends it
+		# Triggering this rule, which starts a line 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;
+				sec=$(NF-1); in_sec=1;
 			}
 		}
 

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

[Rao, Nikhil]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, August 15, 2018 4:19 PM
> To: Rao, Nikhil <nikhil.rao@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Mcnamara, John
> <john.mcnamara@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stephen@networkplumber.org; toggle-mailboxes@hmswarspite.think-
> freely.org
> Subject: Re: [PATCH v9] checkpatches.sh: Add checks for ABI symbol addition
> 
> 
> 
> Thanks, I think I made a mistake in how I detect section names in the awk
> script.  The rule assumes that the entire section is getting added (i.e. we are
> adding the EXPERIMENTAL section as a whole unit, hence the starting a line
> with
> + to id the section name, and thats not the case here.  I think the rule
> + needs
> to be any line in a map file that ends with a { (based on our coding practice), is
> a section start, and the section name is the next to the last field in the line (i.e.
> $(NF-1) ).  Please apply the patch below and confirm that this works for you.
> 
> Best
> Neil
> 
> 
Thanks for the fix, it works.

Nikhil

> 
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-
> change.sh index daaf45e14..cf9cfc745 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -25,14 +25,14 @@ build_map_changes()
>  		# supresses the subordonate rules below
>  		/[-+] a\/.*\.^(map)/ {in_map=0}
> 
> -		# Triggering this rule, which starts a line with a + and ends it
> +		# Triggering this rule, which starts a line 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;
> +				sec=$(NF-1); in_sec=1;
>  			}
>  		}
> 

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

[Neil Horman]

On Thu, Aug 16, 2018 at 06:19:40AM +0000, Rao, Nikhil wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Wednesday, August 15, 2018 4:19 PM
> > To: Rao, Nikhil <nikhil.rao@intel.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; Mcnamara, John
> > <john.mcnamara@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > stephen@networkplumber.org; toggle-mailboxes@hmswarspite.think-
> > freely.org
> > Subject: Re: [PATCH v9] checkpatches.sh: Add checks for ABI symbol addition
> > 
> > 
> > 
> > Thanks, I think I made a mistake in how I detect section names in the awk
> > script.  The rule assumes that the entire section is getting added (i.e. we are
> > adding the EXPERIMENTAL section as a whole unit, hence the starting a line
> > with
> > + to id the section name, and thats not the case here.  I think the rule
> > + needs
> > to be any line in a map file that ends with a { (based on our coding practice), is
> > a section start, and the section name is the next to the last field in the line (i.e.
> > $(NF-1) ).  Please apply the patch below and confirm that this works for you.
> > 
> > Best
> > Neil
> > 
> > 
> Thanks for the fix, it works.
> 
> Nikhil
Thanks, I'll propose it as a fix here shortly.
Neil

> 
> > 
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-
> > change.sh index daaf45e14..cf9cfc745 100755
> > --- a/devtools/check-symbol-change.sh
> > +++ b/devtools/check-symbol-change.sh
> > @@ -25,14 +25,14 @@ build_map_changes()
> >  		# supresses the subordonate rules below
> >  		/[-+] a\/.*\.^(map)/ {in_map=0}
> > 
> > -		# Triggering this rule, which starts a line with a + and ends it
> > +		# Triggering this rule, which starts a line 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;
> > +				sec=$(NF-1); in_sec=1;
> >  			}
> >  		}
> > 
>