On Fri, 15 Dec 2023 12:13:53 +0530
Ankur Dwivedi <[email protected]> wrote:
> This patch series adds a validation in checkpatch tool to check if
> tracepoint is present in any new function added in ethdev, eventdev
> cryptodev and mempool library.
>
> v6:
> - The build_map_changes function is moved from check-symbol-change.sh to
> a new symbol-map-util.sh file. This function can be used in other
> scripts by including symbol-map-util.sh file.
>
> v5:
> - Copied the build_map_changes function from check-symbol-change.sh to
> check-tracepoint.sh.
> - Added eventdev, cryptodev and mempool in libdir in check-tracepoint.sh.
>
> v4:
> - Rebased on the recent next-net branch.
> - Refined logic to find function definition.
> - Updated year in the license in devtools/check-tracepoint.sh.
> - Removed cryptodev, added ethdev in libdir in
> devtools/check-tracepoint.sh.
>
> v3:
> - Split the v2 patch into 2 patches.
> - The file common-func.sh is renamed to build-symbol-map.sh.
> - Removed check-tracepoint.py file.
> - Code improvements in check-tracepoint.sh.
>
> v2:
> - Add check for parent directory.
>
> Ankur Dwivedi (2):
> devtools: move build map changes function
> devtools: add tracepoint check in checkpatch
>
> devtools/check-symbol-change.sh | 76 +---------------
> devtools/check-tracepoint.sh | 148 ++++++++++++++++++++++++++++++++
> devtools/checkpatches.sh | 9 ++
> devtools/symbol-map-util.sh | 78 +++++++++++++++++
> devtools/trace-skiplist.txt | 0
> 5 files changed, 237 insertions(+), 74 deletions(-)
> create mode 100755 devtools/check-tracepoint.sh
> create mode 100644 devtools/symbol-map-util.sh
> create mode 100644 devtools/trace-skiplist.txt
>
This patch seems to have not been updated since build system changes around
symbols.
AI spotted this an other issues. If interested change and resubmit.
Review of [PATCH v6 1/2] devtools: move build map changes function
[PATCH v6 2/2] devtools: add tracepoint check in checkpatch
NOTE: This patch series (v6, Dec 2023) is based on the old
version.map / check-symbol-change.sh infrastructure. Upstream has
since replaced check-symbol-change.sh with check-symbol-change.py
and moved to auto-generated symbol maps via RTE_EXPORT_* macros.
The build_map_changes() shell function parses version.map diffs,
but version.map files are no longer manually edited. This means
the entire detection mechanism will never trigger on current DPDK
patches. The approach needs to be redesigned around RTE_EXPORT_*
annotations rather than version.map changes.
Patch 1/2 - devtools: move build map changes function
------------------------------------------------------
No issues. The function is moved verbatim and the sourcing
mechanism is correct.
Patch 2/2 - devtools: add tracepoint check in checkpatch
---------------------------------------------------------
Error:
1. find_trace_fn silently passes when the function body is absent
from the patch. If a symbol is added to version.map but the
function definition is in a different patch or already in the
tree, the awk script never matches the function name, never
reaches "exit 1", and falls through with exit status 0. The
check reports "tracepoint present" when it was never examined.
The awk should exit with a distinct status (e.g., 2) when the
function body is not found, and the caller should handle that
case (skip or warn).
Warning:
2. find_trace_fn does not filter on diff line prefixes. The awk
matches the function name on any line in the patch -- context
lines (space prefix), added lines (+), and removed lines (-).
If a function is being deleted, its body will still match on
the "-" lines, and the script could incorrectly conclude that
the tracepoint is present (from the old removed code). The
match on the function name and the _trace_ search should both
be restricted to "+" lines to avoid false results.
3. Trap is registered before the function it references is
defined. The line "trap clean_and_exit_on_sig EXIT" appears
before clean_and_exit_on_sig() is defined. While POSIX sh
allows this (the name is resolved when the trap fires, not
when registered), it is fragile and confusing. Move the trap
registration after the function definition.
4. Double cleanup on normal exit. The normal code path does
"rm -f $mapfile" then "exit $exit_code". The exit triggers the
EXIT trap, which runs clean_and_exit_on_sig() calling rm again
(harmless) and exit again. This works but is unnecessarily
convoluted. Either remove the explicit rm/exit at the bottom
and let the trap handle it, or remove the trap and rely on the
explicit cleanup.
5. Shell variables not quoted. Multiple instances of unquoted
variables that could break on paths with spaces:
selfdir=$(dirname $(readlink -f $0))
. $selfdir/symbol-map-util.sh
find_trace_fn $patch $symname
for sym in $(cat $skiplist); do
These should use double quotes:
selfdir=$(dirname "$(readlink -f "$0")")
. "$selfdir/symbol-map-util.sh"
find_trace_fn "$patch" "$symname"
The same quoting issue exists in patch 1/2 as well. Admittedly
the existing DPDK scripts have similar quoting habits, but new
code should set a better example.
Reviewed-by: Stephen Hemminger <[email protected]>
