[dpdk-dev,v6] 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)
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
Comments
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.
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.
>
>
>
>
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?
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
>
>
>
@@ -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
new file mode 100755
@@ -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
+
+
@@ -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))
}