[dpdk-dev,v2] checkpatches.sh: Add checks for ABI symbol addition
Checks
Commit Message
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
Comments
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.
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
>
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
@@ -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/
@@ -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))
}
new file mode 100755
@@ -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
+
+