| Message ID | 20171213151728.16747-2-nhorman@tuxdriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
Hi Neil, Sorry for the very late review. I thought review had been done by others but it seems not. Please find my comments below. 13/12/2017 16:17, Neil Horman: > create mode 100755 buildtools/experimentalsyms.sh When adding a new file, you must reference it in MAINTAINERS. Please add it in the section "ABI versioning". > --- /dev/null > +++ b/buildtools/experimentalsyms.sh I think the file name should include the word "check". What about check-experimental-syms.sh ? > @@ -0,0 +1,35 @@ > +#!/bin/sh You must insert a SPDX license and copyright here. > +if [ -d $MAPFILE ] > +then > + exit 0 > +fi > + > +if [ -d $OBJFILE ] > +then > + exit 0 > +fi Why checking for not being a directory? I guess you could check being a readable file (-r)? Should it return an error? > +for i in `awk 'BEGIN {found=0} > + /.*EXPERIMENTAL.*/ {found=1} > + /.*}.*;/ {found=0} > + /.*;/ {if (found == 1) print $1}' $MAPFILE` > +do > + SYM=`echo $i | sed -e"s/;//"` > + objdump -t $OBJFILE | grep -q "\.text.*$SYM" > + IN_TEXT=$? > + objdump -t $OBJFILE | grep -q "\.text\.experimental.*$SYM" > + IN_EXP=$? > + if [ $IN_TEXT -eq 0 -a $IN_EXP -ne 0 ] > + then > + echo "$SYM is not flagged as experimental" > + echo "but is listed in version map" > + echo "Please add __experimental to the definition of $SYM" > + exit 1 > + fi > +done > +exit 0 exit 0 is useless at the end of a script.
On Sun, Jan 21, 2018 at 07:31:31PM +0100, Thomas Monjalon wrote: > Hi Neil, > > Sorry for the very late review. > I thought review had been done by others but it seems not. > Please find my comments below. > > 13/12/2017 16:17, Neil Horman: > > create mode 100755 buildtools/experimentalsyms.sh > > When adding a new file, you must reference it in MAINTAINERS. > Please add it in the section "ABI versioning". > yup > > --- /dev/null > > +++ b/buildtools/experimentalsyms.sh > > I think the file name should include the word "check". > What about check-experimental-syms.sh ? > > > @@ -0,0 +1,35 @@ > > +#!/bin/sh > > You must insert a SPDX license and copyright here. > Will do. > > +if [ -d $MAPFILE ] > > +then > > + exit 0 > > +fi > > + > > +if [ -d $OBJFILE ] > > +then > > + exit 0 > > +fi > > Why checking for not being a directory? > I guess you could check being a readable file (-r)? > Should it return an error? > The objfile check is out of date (had it in place initially, and is no longer needed). the mapfile check is there because dpdk apps use the same C_TO_O rule and have no mapfile variable set. > > +for i in `awk 'BEGIN {found=0} > > + /.*EXPERIMENTAL.*/ {found=1} > > + /.*}.*;/ {found=0} > > + /.*;/ {if (found == 1) print $1}' $MAPFILE` > > +do > > + SYM=`echo $i | sed -e"s/;//"` > > + objdump -t $OBJFILE | grep -q "\.text.*$SYM" > > + IN_TEXT=$? > > + objdump -t $OBJFILE | grep -q "\.text\.experimental.*$SYM" > > + IN_EXP=$? > > + if [ $IN_TEXT -eq 0 -a $IN_EXP -ne 0 ] > > + then > > + echo "$SYM is not flagged as experimental" > > + echo "but is listed in version map" > > + echo "Please add __experimental to the definition of $SYM" > > + exit 1 > > + fi > > +done > > +exit 0 > > exit 0 is useless at the end of a script. Its there for clarity of exit value. I prefer to be expicit in that. Neil
diff --git a/buildtools/experimentalsyms.sh b/buildtools/experimentalsyms.sh new file mode 100755 index 000000000..7342ec9b3 --- /dev/null +++ b/buildtools/experimentalsyms.sh @@ -0,0 +1,35 @@ +#!/bin/sh + +MAPFILE=$1 +OBJFILE=$2 + +if [ -d $MAPFILE ] +then + exit 0 +fi + +if [ -d $OBJFILE ] +then + exit 0 +fi + +for i in `awk 'BEGIN {found=0} + /.*EXPERIMENTAL.*/ {found=1} + /.*}.*;/ {found=0} + /.*;/ {if (found == 1) print $1}' $MAPFILE` +do + SYM=`echo $i | sed -e"s/;//"` + objdump -t $OBJFILE | grep -q "\.text.*$SYM" + IN_TEXT=$? + objdump -t $OBJFILE | grep -q "\.text\.experimental.*$SYM" + IN_EXP=$? + if [ $IN_TEXT -eq 0 -a $IN_EXP -ne 0 ] + then + echo "$SYM is not flagged as experimental" + echo "but is listed in version map" + echo "Please add __experimental to the definition of $SYM" + exit 1 + fi +done +exit 0 +
This tools reads the given version map for a directory, and checks to ensure that, for each symbol listed in the export list, the corresponding definition is tagged as __experimental, erroring out if its not. In this way, we can ensure that the EXPERIMENTAL api is kept in sync with the tags Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas Monjalon <thomas@monjalon.net> CC: "Mcnamara, John" <john.mcnamara@intel.com> CC: Bruce Richardson <bruce.richardson@intel.com> --- buildtools/experimentalsyms.sh | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100755 buildtools/experimentalsyms.sh