[PATCH] devtools: add tracepoint check in checkpatch

[PATCH] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a check in checkpatch tool, to check if a
tracepoint is present in any new function added in cryptodev
library.

It uses the existing build_map_changes() function and version.map
file to create a list of newly added functions. The definition of
these functions are checked if they contain tracepoint.
The checkpatch return error if the tracepoint is not present.

For functions for which trace is not needed, they can be
appended to devtools/trace-skiplist.txt file. The above tracepoint
check will be skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-symbol-change.sh | 76 +-------------------------------
 devtools/check-tracepoint.py    | 52 ++++++++++++++++++++++
 devtools/check-tracepoint.sh    | 65 ++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  9 ++++
 devtools/common-func.sh         | 77 +++++++++++++++++++++++++++++++++
 devtools/trace-skiplist.txt     |  0
 6 files changed, 205 insertions(+), 74 deletions(-)
 create mode 100755 devtools/check-tracepoint.py
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/common-func.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index 8992214ac8..4bdd0d727a 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -2,80 +2,8 @@
 # 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
-		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
-
-		# The previous rule catches all .map files, anything else
-		# indicates we left the map chunk.
-		/[-+] [ab]\// {in_map=0}
-
-		# 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 removed.
-		# Triggering this rule sets in_sec to 1, which actives the
-		# symbol rule below
-		/^.*{/ {
-			gsub("+", "");
-			if (in_map == 1) {
-				sec=$(NF-1); in_sec=1;
-			}
-		}
-
-		# This rule identifies 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"
-
-}
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/common-func.sh
 
 is_stable_section() {
 	[ "$1" != 'EXPERIMENTAL' ] && [ "$1" != 'INTERNAL' ]
diff --git a/devtools/check-tracepoint.py b/devtools/check-tracepoint.py
new file mode 100755
index 0000000000..5ce0934493
--- /dev/null
+++ b/devtools/check-tracepoint.py
@@ -0,0 +1,52 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2022 Marvell.
+
+import sys
+
+patch = sys.argv[1]
+fn = sys.argv[2]
+
+with open(patch, 'r') as fr:
+	fstr = fr.read()
+
+def find_fn_def():
+	found = 0
+	tmp = 0
+	idx = 0
+	while found == 0:
+		idx = fstr.find("+"+fn+"(", idx)
+		if (idx != -1):
+			tmp = fstr.find(')', idx)
+			if (fstr[tmp + 1] == ';'):
+				idx = tmp
+				continue
+			else:
+				found = 1
+		else:
+			break
+	return idx
+
+def find_trace(index):
+	fp = fstr.find("{", index)
+	sp = fstr.find("}", fp)
+	fd = fstr[fp:sp]
+
+	i = fd.find("_trace_")
+	if (i != -1):
+		return 0
+	else:
+		return 1
+
+
+def __main():
+	ret=0
+	index = find_fn_def()
+	if (index != -1):
+		# If function definition is present,
+		# check if tracepoint is present
+		ret = find_trace(index)
+	return ret
+
+if __name__ == "__main__":
+	sys.exit(__main())
diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..278b40ac83
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2022 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/common-func.sh
+
+# Library for trace check
+libdir="cryptodev"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+check_for_tracepoint()
+{
+	mapdb="$2"
+	ret=0
+
+	while read -r mname symname secname ar; do
+		libp=0
+		skip_sym=0
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+
+		for lib in $libdir; do
+			if [ $lib = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 1 ] && [ $skip_sym -eq 0 ] && [ "$ar" = "add" ]; then
+			if [ "$secname" = "EXPERIMENTAL" ]; then
+				# Check if new API is added with tracepoint
+				if ! devtools/check-tracepoint.py $1 $symname; then
+					ret=1
+					echo -n "ERROR: New function $symname is added "
+					echo -n "without a tracepoint. Please add a tracepoint "
+					echo -n "or add the function $symname in "
+					echo "devtools/trace-skiplist.txt to skip this error."
+				fi
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+clean_and_exit_on_sig()
+{
+	rm -rf "$mapfile"
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=$(mktemp -t dpdk.mapdb.XXXXXX)
+
+build_map_changes "$1" "$mapfile"
+check_for_tracepoint "$1" "$mapfile"
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 1f1175c4f1..7392560460 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -352,6 +353,14 @@ check () { # <patch> <commit> <title>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$3"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/common-func.sh b/devtools/common-func.sh
new file mode 100644
index 0000000000..c88e949890
--- /dev/null
+++ b/devtools/common-func.sh
@@ -0,0 +1,77 @@
+#!/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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+}
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.28.0

[PATCH v2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a check in checkpatch tool, to check if a
tracepoint is present in any new function added in cryptodev
library.

It uses the existing build_map_changes() function and version.map
file to create a list of newly added functions. The definition of
these functions are checked if they contain tracepoint.
The checkpatch return error if the tracepoint is not present.

For functions for which trace is not needed, they can be
appended to devtools/trace-skiplist.txt file. The above tracepoint
check will be skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
v2:
 - Add check for parent directory.

 devtools/check-symbol-change.sh | 76 +-------------------------------
 devtools/check-tracepoint.py    | 52 ++++++++++++++++++++++
 devtools/check-tracepoint.sh    | 66 ++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |  9 ++++
 devtools/common-func.sh         | 77 +++++++++++++++++++++++++++++++++
 devtools/trace-skiplist.txt     |  0
 6 files changed, 206 insertions(+), 74 deletions(-)
 create mode 100755 devtools/check-tracepoint.py
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/common-func.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index 8992214ac8..4bdd0d727a 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -2,80 +2,8 @@
 # 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
-		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
-
-		# The previous rule catches all .map files, anything else
-		# indicates we left the map chunk.
-		/[-+] [ab]\// {in_map=0}
-
-		# 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 removed.
-		# Triggering this rule sets in_sec to 1, which actives the
-		# symbol rule below
-		/^.*{/ {
-			gsub("+", "");
-			if (in_map == 1) {
-				sec=$(NF-1); in_sec=1;
-			}
-		}
-
-		# This rule identifies 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"
-
-}
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/common-func.sh
 
 is_stable_section() {
 	[ "$1" != 'EXPERIMENTAL' ] && [ "$1" != 'INTERNAL' ]
diff --git a/devtools/check-tracepoint.py b/devtools/check-tracepoint.py
new file mode 100755
index 0000000000..5ce0934493
--- /dev/null
+++ b/devtools/check-tracepoint.py
@@ -0,0 +1,52 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2022 Marvell.
+
+import sys
+
+patch = sys.argv[1]
+fn = sys.argv[2]
+
+with open(patch, 'r') as fr:
+	fstr = fr.read()
+
+def find_fn_def():
+	found = 0
+	tmp = 0
+	idx = 0
+	while found == 0:
+		idx = fstr.find("+"+fn+"(", idx)
+		if (idx != -1):
+			tmp = fstr.find(')', idx)
+			if (fstr[tmp + 1] == ';'):
+				idx = tmp
+				continue
+			else:
+				found = 1
+		else:
+			break
+	return idx
+
+def find_trace(index):
+	fp = fstr.find("{", index)
+	sp = fstr.find("}", fp)
+	fd = fstr[fp:sp]
+
+	i = fd.find("_trace_")
+	if (i != -1):
+		return 0
+	else:
+		return 1
+
+
+def __main():
+	ret=0
+	index = find_fn_def()
+	if (index != -1):
+		# If function definition is present,
+		# check if tracepoint is present
+		ret = find_trace(index)
+	return ret
+
+if __name__ == "__main__":
+	sys.exit(__main())
diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..dc8f14ae87
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2022 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/common-func.sh
+
+# Library for trace check
+libdir="cryptodev"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+check_for_tracepoint()
+{
+	mapdb="$2"
+	ret=0
+
+	while read -r mname symname secname ar; do
+		libp=0
+		skip_sym=0
+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+
+		for lib in $libdir; do
+			if [ "$pdir" = "lib" ] && [ $lib = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 1 ] && [ $skip_sym -eq 0 ] && [ "$ar" = "add" ]; then
+			if [ "$secname" = "EXPERIMENTAL" ]; then
+				# Check if new API is added with tracepoint
+				if ! devtools/check-tracepoint.py $1 $symname; then
+					ret=1
+					echo -n "ERROR: New function $symname is added "
+					echo -n "without a tracepoint. Please add a tracepoint "
+					echo -n "or add the function $symname in "
+					echo "devtools/trace-skiplist.txt to skip this error."
+				fi
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+clean_and_exit_on_sig()
+{
+	rm -rf "$mapfile"
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=$(mktemp -t dpdk.mapdb.XXXXXX)
+
+build_map_changes "$1" "$mapfile"
+check_for_tracepoint "$1" "$mapfile"
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 1f1175c4f1..7392560460 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -352,6 +353,14 @@ check () { # <patch> <commit> <title>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$3"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/common-func.sh b/devtools/common-func.sh
new file mode 100644
index 0000000000..c88e949890
--- /dev/null
+++ b/devtools/common-func.sh
@@ -0,0 +1,77 @@
+#!/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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+}
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.28.0

Re: [PATCH v2] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

12/10/2022 13:45, Ankur Dwivedi:
>  devtools/check-symbol-change.sh | 76 +-------------------------------
>  devtools/check-tracepoint.py    | 52 ++++++++++++++++++++++
>  devtools/check-tracepoint.sh    | 66 ++++++++++++++++++++++++++++
>  devtools/checkpatches.sh        |  9 ++++
>  devtools/common-func.sh         | 77 +++++++++++++++++++++++++++++++++
>  devtools/trace-skiplist.txt     |  0

Before diving into this proposal,
I would like a split of the patch for the rework (and move)
of check-symbol-change.sh alone.

In general I see too many files added:
	check-tracepoint.py and check-tracepoint.sh
common-func.sh is probably a bad name.

RE: [EXT] Re: [PATCH v2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Wednesday, October 12, 2022 6:39 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; Akhil Goyal <gakhil@marvell.com>;
>royzhang1980@gmail.com; Amit Prakash Shukla <amitprakashs@marvell.com>;
>Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ankur Dwivedi
><adwivedi@marvell.com>; david.marchand@redhat.com
>Subject: [EXT] Re: [PATCH v2] devtools: add tracepoint check in checkpatch
>
>External Email
>
>----------------------------------------------------------------------
>12/10/2022 13:45, Ankur Dwivedi:
>>  devtools/check-symbol-change.sh | 76 +-------------------------------
>>  devtools/check-tracepoint.py    | 52 ++++++++++++++++++++++
>>  devtools/check-tracepoint.sh    | 66 ++++++++++++++++++++++++++++
>>  devtools/checkpatches.sh        |  9 ++++
>>  devtools/common-func.sh         | 77 +++++++++++++++++++++++++++++++++
>>  devtools/trace-skiplist.txt     |  0
>
>Before diving into this proposal,
>I would like a split of the patch for the rework (and move) of check-symbol-
>change.sh alone.
Will split the patch in next version.
>
>In general I see too many files added:
>	check-tracepoint.py and check-tracepoint.sh common-func.sh is
>probably a bad name.
Regarding common-func.sh name, I am thinking of renaming it to common.sh or helper.sh, considering there may be more common shell routines in future. Otherwise it can be renamed to build-map.sh considering it will contain only build_map_changes() function. 
Please suggest a suitable name if my suggested names are bad.

Will try to combine check-tracepoint.py and check-tracepoint.sh.
>

Re: [EXT] Re: [PATCH v2] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

12/10/2022 17:16, Ankur Dwivedi:
> 
> >-----Original Message-----
> >From: Thomas Monjalon <thomas@monjalon.net>
> >Sent: Wednesday, October 12, 2022 6:39 PM
> >To: Ankur Dwivedi <adwivedi@marvell.com>
> >Cc: dev@dpdk.org; Akhil Goyal <gakhil@marvell.com>;
> >royzhang1980@gmail.com; Amit Prakash Shukla <amitprakashs@marvell.com>;
> >Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ankur Dwivedi
> ><adwivedi@marvell.com>; david.marchand@redhat.com
> >Subject: [EXT] Re: [PATCH v2] devtools: add tracepoint check in checkpatch
> >
> >External Email
> >
> >----------------------------------------------------------------------
> >12/10/2022 13:45, Ankur Dwivedi:
> >>  devtools/check-symbol-change.sh | 76 +-------------------------------
> >>  devtools/check-tracepoint.py    | 52 ++++++++++++++++++++++
> >>  devtools/check-tracepoint.sh    | 66 ++++++++++++++++++++++++++++
> >>  devtools/checkpatches.sh        |  9 ++++
> >>  devtools/common-func.sh         | 77 +++++++++++++++++++++++++++++++++
> >>  devtools/trace-skiplist.txt     |  0
> >
> >Before diving into this proposal,
> >I would like a split of the patch for the rework (and move) of check-symbol-
> >change.sh alone.
> Will split the patch in next version.
> >
> >In general I see too many files added:
> >	check-tracepoint.py and check-tracepoint.sh common-func.sh is
> >probably a bad name.
> Regarding common-func.sh name, I am thinking of renaming it to common.sh or helper.sh, considering there may be more common shell routines in future. Otherwise it can be renamed to build-map.sh considering it will contain only build_map_changes() function. 
> Please suggest a suitable name if my suggested names are bad.
> 
> Will try to combine check-tracepoint.py and check-tracepoint.sh.

There are many very different scripts in devtools.
Having a very common script won't work well.
Please keep scripts specialized.
If you need a script to check symbol, it should include "symbol" in its name.

[PATCH v3 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch series adds a validation in checkpatch tool to check if
tracepoint is present in any new function added in cryptodev library.

The first patch in the series moves build_map_changes function from
check-symbol-change.sh to a new file build-symbol-map.sh.

The second patch in the series adds a new script file
check-tracepoint.sh which is called from checkpatch tool. The
check-tracepoint.sh contains the code to detect the presence
of tracepoint in a new function added to cryptodev library.

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 symbol map function
  devtools: add tracepoint check in checkpatch

 devtools/build-symbol-map.sh    |  77 +++++++++++++++++++
 devtools/check-symbol-change.sh |  76 +------------------
 devtools/check-tracepoint.sh    | 129 ++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |   9 +++
 devtools/trace-skiplist.txt     |   0
 5 files changed, 217 insertions(+), 74 deletions(-)
 create mode 100644 devtools/build-symbol-map.sh
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

-- 
2.28.0

[PATCH v3 1/2] devtools: move build symbol map function

[Ankur Dwivedi]

This patch moves the build_map_changes function from
check-symbol-change.sh to a new build-symbol-map.sh file.
This function can be used in other scripts by including
build-symbol-map.sh file.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/build-symbol-map.sh    | 77 +++++++++++++++++++++++++++++++++
 devtools/check-symbol-change.sh | 76 +-------------------------------
 2 files changed, 79 insertions(+), 74 deletions(-)
 create mode 100644 devtools/build-symbol-map.sh

diff --git a/devtools/build-symbol-map.sh b/devtools/build-symbol-map.sh
new file mode 100644
index 0000000000..c88e949890
--- /dev/null
+++ b/devtools/build-symbol-map.sh
@@ -0,0 +1,77 @@
+#!/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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+}
diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index 8992214ac8..874afa2632 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -2,80 +2,8 @@
 # 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
-		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
-
-		# The previous rule catches all .map files, anything else
-		# indicates we left the map chunk.
-		/[-+] [ab]\// {in_map=0}
-
-		# 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 removed.
-		# Triggering this rule sets in_sec to 1, which actives the
-		# symbol rule below
-		/^.*{/ {
-			gsub("+", "");
-			if (in_map == 1) {
-				sec=$(NF-1); in_sec=1;
-			}
-		}
-
-		# This rule identifies 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"
-
-}
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/build-symbol-map.sh
 
 is_stable_section() {
 	[ "$1" != 'EXPERIMENTAL' ] && [ "$1" != 'INTERNAL' ]
-- 
2.28.0

[PATCH v3 2/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a validation in checkpatch tool, to check if a
tracepoint is present in any new function added in cryptodev
library.

It uses the existing build_map_changes function to create a map
of functions. In the map, the newly added functions are identified
and their definition are checked for the presence of tracepoint.
The checkpatch return error if the tracepoint is not present.

For functions for which trace is not needed, they can be added to
devtools/trace-skiplist.txt file. The above tracepoint check will
be skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-tracepoint.sh | 129 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 +++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 138 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..2e8a33758c
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,129 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2022 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/build-symbol-map.sh
+
+# Library for trace check
+libdir="cryptodev"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+find_trace_fn()
+{
+	local fname="$1"
+
+	cat "$fname" | awk -v symname="$2 *\\\(" '
+		# Initialize the variables.
+		# The variable symname provides a pattern to match
+		# "function_name(", zero or more spaces can be present
+		# between function_name and (.
+		BEGIN {found=0; fndef=""}
+
+		# Matches the function name, set found=1.
+		$0 ~ symname {found=1}
+
+		# If closing parentheses with semicolon ");" is found, then it
+		# is not the function definition.
+		/) *;/ {
+			if (found == 1) {
+				found=0
+			}
+		}
+
+		# If closing parentheses is found, this is the start of function
+		# definition. Check for tracepoint in the function definition.
+		# The tracepoint has _trace_ in its name.
+		/)/ {
+			if (found == 1) {
+				while (index($0, "}") == 0) {
+					if (index($0, "_trace_") != 0) {
+						exit 0
+					}
+					if (getline <= 0) {
+						break
+					}
+				}
+				exit 1
+			}
+		}
+	'
+}
+
+check_for_tracepoint()
+{
+	local patch="$1"
+	local mapdb="$2"
+	local skip_sym
+	local libname
+	local secname
+	local mname
+	local ret=0
+	local pdir
+	local libp
+	local libn
+	local sym
+	local ar
+
+	while read -r mname symname secname ar; do
+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+		skip_sym=0
+		libp=0
+
+		if [ "$pdir" != "lib" ]; then
+			continue
+		fi
+
+		for libn in $libdir; do
+			if [ $libn = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 0 ]; then
+			continue
+		fi
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $skip_sym -eq 1 ]; then
+			continue
+		fi
+
+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
+			# Check if new API is added with tracepoint
+			find_trace_fn $patch $symname
+			if [ $? -eq 1 ]; then
+				ret=1
+				echo -n "ERROR: New function $symname is added "
+				echo -n "without a tracepoint. Please add a tracepoint "
+				echo -n "or add the function $symname in "
+				echo "devtools/trace-skiplist.txt to skip this error."
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+clean_and_exit_on_sig()
+{
+	rm -rf "$mapfile"
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=$(mktemp -t dpdk.mapdb.XXXXXX)
+patch=$1
+
+build_map_changes "$patch" "$mapfile"
+check_for_tracepoint "$patch" "$mapfile"
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 1f1175c4f1..7392560460 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -352,6 +353,14 @@ check () { # <patch> <commit> <title>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$3"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.28.0

RE: [PATCH v3 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

Hi Thomas,

Please let me know if this series can be included in 22.11 rc3.

Regards,
Ankur

>-----Original Message-----
>From: Ankur Dwivedi <adwivedi@marvell.com>
>Sent: Saturday, October 15, 2022 6:28 PM
>To: dev@dpdk.org
>Cc: thomas@monjalon.net; david.marchand@redhat.com; Akhil Goyal
><gakhil@marvell.com>; royzhang1980@gmail.com; Amit Prakash Shukla
><amitprakashs@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>
>Subject: [PATCH v3 0/2] devtools: add tracepoint check in checkpatch
>
>This patch series adds a validation in checkpatch tool to check if tracepoint is
>present in any new function added in cryptodev library.
>
>The first patch in the series moves build_map_changes function from check-
>symbol-change.sh to a new file build-symbol-map.sh.
>
>The second patch in the series adds a new script file check-tracepoint.sh which
>is called from checkpatch tool. The check-tracepoint.sh contains the code to
>detect the presence of tracepoint in a new function added to cryptodev library.
>
>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 symbol map function
>  devtools: add tracepoint check in checkpatch
>
> devtools/build-symbol-map.sh    |  77 +++++++++++++++++++
> devtools/check-symbol-change.sh |  76 +------------------
> devtools/check-tracepoint.sh    | 129 ++++++++++++++++++++++++++++++++
> devtools/checkpatches.sh        |   9 +++
> devtools/trace-skiplist.txt     |   0
> 5 files changed, 217 insertions(+), 74 deletions(-)  create mode 100644
>devtools/build-symbol-map.sh  create mode 100755 devtools/check-
>tracepoint.sh  create mode 100644 devtools/trace-skiplist.txt
>
>--
>2.28.0

[PATCH v4 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch series adds a validation in checkpatch tool to check if
tracepoint is present in any new function added in ethdev library.

The first patch in the series moves build_map_changes function from
check-symbol-change.sh to a new file build-symbol-map.sh.

The second patch in the series adds a new script file
check-tracepoint.sh which is called from checkpatch tool. The
check-tracepoint.sh contains the code to detect the presence
of tracepoint in a new function added to ethdev library.

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 symbol map function
  devtools: add tracepoint check in checkpatch

 devtools/build-symbol-map.sh    |  78 +++++++++++++++++
 devtools/check-symbol-change.sh |  76 +----------------
 devtools/check-tracepoint.sh    | 146 ++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh        |   9 ++
 devtools/trace-skiplist.txt     |   0
 5 files changed, 235 insertions(+), 74 deletions(-)
 create mode 100755 devtools/build-symbol-map.sh
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

-- 
2.25.1

[PATCH v4 1/2] devtools: move build symbol map function

[Ankur Dwivedi]

This patch moves the build_map_changes function from
check-symbol-change.sh to a new build-symbol-map.sh file.
This function can be used in other scripts by including
build-symbol-map.sh file.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/build-symbol-map.sh    | 78 +++++++++++++++++++++++++++++++++
 devtools/check-symbol-change.sh | 76 +-------------------------------
 2 files changed, 80 insertions(+), 74 deletions(-)
 create mode 100755 devtools/build-symbol-map.sh

diff --git a/devtools/build-symbol-map.sh b/devtools/build-symbol-map.sh
new file mode 100755
index 0000000000..86e4279cdc
--- /dev/null
+++ b/devtools/build-symbol-map.sh
@@ -0,0 +1,78 @@
+#!/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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+
+}
diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index 8992214ac8..874afa2632 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -2,80 +2,8 @@
 # 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
-		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
-
-		# The previous rule catches all .map files, anything else
-		# indicates we left the map chunk.
-		/[-+] [ab]\// {in_map=0}
-
-		# 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 removed.
-		# Triggering this rule sets in_sec to 1, which actives the
-		# symbol rule below
-		/^.*{/ {
-			gsub("+", "");
-			if (in_map == 1) {
-				sec=$(NF-1); in_sec=1;
-			}
-		}
-
-		# This rule identifies 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"
-
-}
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/build-symbol-map.sh
 
 is_stable_section() {
 	[ "$1" != 'EXPERIMENTAL' ] && [ "$1" != 'INTERNAL' ]
-- 
2.25.1

[PATCH v4 2/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a validation in checkpatch tool, to check if a
tracepoint is present in any new function added in ethdev library.

It uses the existing build_map_changes function to create a map
of functions. In the map, the newly added functions, added in the
experimental section are identified and their definition are checked for
the presence of tracepoint. The checkpatch return error if the tracepoint
is not present.

For functions for which trace is not needed, they can be added to
devtools/trace-skiplist.txt file. The above tracepoint check will be
skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-tracepoint.sh | 146 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 +++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 155 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..591d5cd397
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,146 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (C) 2023 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/build-symbol-map.sh
+
+# Library for trace check
+libdir="ethdev"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+find_trace_fn()
+{
+	local fname="$1"
+
+	cat "$fname" | awk -v symname="$2 *\\\(" '
+		# Initialize the variables.
+		# The variable symname provides a pattern to match
+		# "function_name(", zero or more spaces can be present
+		# between function_name and (.
+		BEGIN {found=0}
+
+		# Matches the function name, set found=1.
+		$0 ~ symname {found=1}
+
+		# If closing parentheses with semicolon ");" is found, then it
+		# is not the function definition.
+		/) *;/ {
+			if (found == 1) {
+				found=0
+			}
+		}
+
+		/)/ {
+			if (found == 1) {
+				ln_pth = NR
+			}
+		}
+
+		# If closing parentheses and then opening braces is found in
+		# adjacent line, then this is the start of function
+		# definition. Check for tracepoint in the function definition.
+		# The tracepoint has _trace_ in its name.
+		/{/ {
+			if (found == 1) {
+				if (ln_pth != NR - 1) {
+					found = 0
+					next
+				}
+				while (index($0, "}") != 2) {
+					if (index($0, "_trace_") != 0) {
+						exit 0
+					}
+					if (getline <= 0) {
+						break
+					}
+				}
+				exit 1
+			}
+		}
+	'
+}
+
+check_for_tracepoint()
+{
+	local patch="$1"
+	local mapdb="$2"
+	local skip_sym
+	local libname
+	local secname
+	local mname
+	local ret=0
+	local pdir
+	local libp
+	local libn
+	local sym
+	local ar
+
+	while read -r mname symname secname ar; do
+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+		skip_sym=0
+		libp=0
+
+		if [ "$pdir" != "lib" ]; then
+			continue
+		fi
+
+		for libn in $libdir; do
+			if [ $libn = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 0 ]; then
+			continue
+		fi
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $skip_sym -eq 1 ]; then
+			continue
+		fi
+
+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
+			# Check if new API is added with tracepoint
+			find_trace_fn $patch $symname
+			if [ $? -eq 1 ]; then
+				ret=1
+				echo -n "ERROR: New function $symname is added "
+				echo -n "without a tracepoint. Please add a tracepoint "
+				echo -n "or add the function $symname in "
+				echo "devtools/trace-skiplist.txt to skip this error."
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=`mktemp -t dpdk.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_tracepoint "$patch" "$mapfile"
+exit_code=$?
+rm -f "$mapfile"
+
+exit $exit_code
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 1dee094c7a..471db1d21b 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -386,6 +387,14 @@ check () { # <patch-file> <commit>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$subject"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.25.1

[PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

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.

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 (1):
  devtools: add tracepoint check in checkpatch

 devtools/check-tracepoint.sh | 223 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 ++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 232 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

-- 
2.25.1

[PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a validation in checkpatch tool, to check if a
tracepoint is present in any new function added in cryptodev, ethdev,
eventdev and mempool library.

In this patch, the build_map_changes function is copied from
check-symbol-change.sh to check-tracepoint.sh. The check-tracepoint.sh
script uses build_map_changes function to create a map of functions.
In the map, the newly added functions, added in the experimental section
are identified and their definition are checked for the presence of
tracepoint. The checkpatch return error if the tracepoint is not present.

For functions for which trace is not needed, they can be added to
devtools/trace-skiplist.txt file. The above tracepoint check will be
skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-tracepoint.sh | 223 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 ++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 232 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..88d6b1fd53
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,223 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2018 Neil Horman <nhorman@tuxdriver.com>
+# Copyright(C) 2023 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+
+# Library for trace check
+libdir="cryptodev ethdev eventdev mempool"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+
+}
+
+find_trace_fn()
+{
+	local fname="$1"
+
+	cat "$fname" | awk -v symname="$2 *\\\(" '
+		# Initialize the variables.
+		# The variable symname provides a pattern to match
+		# "function_name(", zero or more spaces can be present
+		# between function_name and (.
+		BEGIN {state=0; ln_pth=0}
+
+		# Matches the function name, set state=1.
+		$0 ~ symname {state=1}
+
+		# If closing parentheses with semicolon ");" is found, then it
+		# is not the function definition.
+		/) *;/ {
+			if (state == 1) {
+				state=0
+			}
+		}
+
+		/)/ {
+			if (state == 1) {
+				state=2
+				ln_pth=NR
+				next
+			}
+		}
+
+		# If closing parentheses and then opening braces is found in
+		# adjacent line, then this is the start of function
+		# definition. Check for tracepoint in the function definition.
+		# The tracepoint has _trace_ in its name.
+		/{/ {
+			if (state == 2) {
+				if (ln_pth != NR - 1) {
+					state=0
+					next
+				}
+				while (index($0, "}") != 2) {
+					if (index($0, "_trace_") != 0) {
+						exit 0
+					}
+					if (getline <= 0) {
+						break
+					}
+				}
+				exit 1
+			}
+		}
+	'
+}
+
+check_for_tracepoint()
+{
+	local patch="$1"
+	local mapdb="$2"
+	local skip_sym
+	local libname
+	local secname
+	local mname
+	local ret=0
+	local pdir
+	local libp
+	local libn
+	local sym
+	local ar
+
+	while read -r mname symname secname ar; do
+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+		skip_sym=0
+		libp=0
+
+		if [ "$pdir" != "lib" ]; then
+			continue
+		fi
+
+		for libn in $libdir; do
+			if [ $libn = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 0 ]; then
+			continue
+		fi
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $skip_sym -eq 1 ]; then
+			continue
+		fi
+
+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
+			# Check if new API is added with tracepoint
+			find_trace_fn $patch $symname
+			if [ $? -eq 1 ]; then
+				ret=1
+				echo -n "ERROR: New function $symname is added "
+				echo -n "without a tracepoint. Please add a tracepoint "
+				echo -n "or add the function $symname in "
+				echo "devtools/trace-skiplist.txt to skip this error."
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=`mktemp -t dpdk.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_tracepoint "$patch" "$mapfile"
+exit_code=$?
+rm -f "$mapfile"
+
+exit $exit_code
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 1dee094c7a..471db1d21b 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -386,6 +387,14 @@ check () { # <patch-file> <commit>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$subject"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.25.1

RE: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

Hi Thomas,

Please let me know if there is any feedback on this patch.

Regards,
Ankur

>-----Original Message-----
>From: Ankur Dwivedi <adwivedi@marvell.com>
>Sent: Tuesday, March 7, 2023 5:35 PM
>To: dev@dpdk.org
>Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>Ankur Dwivedi <adwivedi@marvell.com>
>Subject: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch
>
>This patch adds a validation in checkpatch tool, to check if a tracepoint is
>present in any new function added in cryptodev, ethdev, eventdev and
>mempool library.
>
>In this patch, the build_map_changes function is copied from check-symbol-
>change.sh to check-tracepoint.sh. The check-tracepoint.sh script uses
>build_map_changes function to create a map of functions.
>In the map, the newly added functions, added in the experimental section are
>identified and their definition are checked for the presence of tracepoint. The
>checkpatch return error if the tracepoint is not present.
>
>For functions for which trace is not needed, they can be added to
>devtools/trace-skiplist.txt file. The above tracepoint check will be skipped for
>them.
>
>Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>---
> devtools/check-tracepoint.sh | 223 +++++++++++++++++++++++++++++++++++
> devtools/checkpatches.sh     |   9 ++
> devtools/trace-skiplist.txt  |   0
> 3 files changed, 232 insertions(+)
> create mode 100755 devtools/check-tracepoint.sh  create mode 100644
>devtools/trace-skiplist.txt
>
>diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh new
>file mode 100755 index 0000000000..88d6b1fd53
>--- /dev/null
>+++ b/devtools/check-tracepoint.sh
>@@ -0,0 +1,223 @@
>+#!/bin/sh
>+# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018 Neil Horman
>+<nhorman@tuxdriver.com> # Copyright(C) 2023 Marvell.
>+
>+selfdir=$(dirname $(readlink -f $0))
>+
>+# Library for trace check
>+libdir="cryptodev ethdev eventdev mempool"
>+
>+# Functions for which the trace check can be skipped
>+skiplist="$selfdir/trace-skiplist.txt"
>+
>+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
>+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
>+
>+		# The previous rule catches all .map files, anything else
>+		# indicates we left the map chunk.
>+		/[-+] [ab]\// {in_map=0}
>+
>+		# 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 removed.
>+		# Triggering this rule sets in_sec to 1, which actives the
>+		# symbol rule below
>+		/^.*{/ {
>+			gsub("+", "");
>+			if (in_map == 1) {
>+				sec=$(NF-1); in_sec=1;
>+			}
>+		}
>+
>+		# This rule identifies 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"
>+
>+}
>+
>+find_trace_fn()
>+{
>+	local fname="$1"
>+
>+	cat "$fname" | awk -v symname="$2 *\\\(" '
>+		# Initialize the variables.
>+		# The variable symname provides a pattern to match
>+		# "function_name(", zero or more spaces can be present
>+		# between function_name and (.
>+		BEGIN {state=0; ln_pth=0}
>+
>+		# Matches the function name, set state=1.
>+		$0 ~ symname {state=1}
>+
>+		# If closing parentheses with semicolon ");" is found, then it
>+		# is not the function definition.
>+		/) *;/ {
>+			if (state == 1) {
>+				state=0
>+			}
>+		}
>+
>+		/)/ {
>+			if (state == 1) {
>+				state=2
>+				ln_pth=NR
>+				next
>+			}
>+		}
>+
>+		# If closing parentheses and then opening braces is found in
>+		# adjacent line, then this is the start of function
>+		# definition. Check for tracepoint in the function definition.
>+		# The tracepoint has _trace_ in its name.
>+		/{/ {
>+			if (state == 2) {
>+				if (ln_pth != NR - 1) {
>+					state=0
>+					next
>+				}
>+				while (index($0, "}") != 2) {
>+					if (index($0, "_trace_") != 0) {
>+						exit 0
>+					}
>+					if (getline <= 0) {
>+						break
>+					}
>+				}
>+				exit 1
>+			}
>+		}
>+	'
>+}
>+
>+check_for_tracepoint()
>+{
>+	local patch="$1"
>+	local mapdb="$2"
>+	local skip_sym
>+	local libname
>+	local secname
>+	local mname
>+	local ret=0
>+	local pdir
>+	local libp
>+	local libn
>+	local sym
>+	local ar
>+
>+	while read -r mname symname secname ar; do
>+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
>+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
>+		skip_sym=0
>+		libp=0
>+
>+		if [ "$pdir" != "lib" ]; then
>+			continue
>+		fi
>+
>+		for libn in $libdir; do
>+			if [ $libn = $libname ]; then
>+				libp=1
>+				break
>+			fi
>+		done
>+
>+		if [ $libp -eq 0 ]; then
>+			continue
>+		fi
>+
>+		for sym in $(cat $skiplist); do
>+			if [ $sym = $symname ]; then
>+				skip_sym=1
>+				break
>+			fi
>+		done
>+
>+		if [ $skip_sym -eq 1 ]; then
>+			continue
>+		fi
>+
>+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
>+			# Check if new API is added with tracepoint
>+			find_trace_fn $patch $symname
>+			if [ $? -eq 1 ]; then
>+				ret=1
>+				echo -n "ERROR: New function $symname is
>added "
>+				echo -n "without a tracepoint. Please add a
>tracepoint "
>+				echo -n "or add the function $symname in "
>+				echo "devtools/trace-skiplist.txt to skip this
>error."
>+			fi
>+		fi
>+	done < "$mapdb"
>+
>+	return $ret
>+}
>+
>+trap clean_and_exit_on_sig EXIT
>+
>+mapfile=`mktemp -t dpdk.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_tracepoint "$patch" "$mapfile"
>+exit_code=$?
>+rm -f "$mapfile"
>+
>+exit $exit_code
>diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh index
>1dee094c7a..471db1d21b 100755
>--- a/devtools/checkpatches.sh
>+++ b/devtools/checkpatches.sh
>@@ -10,6 +10,7 @@
> . $(dirname $(readlink -f $0))/load-devel-config
>
> VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
>+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
>
> # Enable codespell by default. This can be overwritten from a config file.
> # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to
>a valid path @@ -386,6 +387,14 @@ check () { # <patch-file> <commit>
> 		ret=1
> 	fi
>
>+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
>+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
>+	if [ $? -ne 0 ] ; then
>+		$headline_printed || print_headline "$subject"
>+		printf '%s\n' "$report"
>+		ret=1
>+	fi
>+
> 	if [ "$tmpinput" != "$1" ]; then
> 		rm -f "$tmpinput"
> 		trap - INT
>diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt new file
>mode 100644 index 0000000000..e69de29bb2
>--
>2.25.1

Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Stephen Hemminger]

On Thu, 18 May 2023 13:45:29 +0000
Ankur Dwivedi <adwivedi@marvell.com> wrote:

> >-----Original Message-----
> >From: Ankur Dwivedi <adwivedi@marvell.com>
> >Sent: Tuesday, March 7, 2023 5:35 PM
> >To: dev@dpdk.org
> >Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> >Ankur Dwivedi <adwivedi@marvell.com>
> >Subject: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch
> >
> >This patch adds a validation in checkpatch tool, to check if a tracepoint is
> >present in any new function added in cryptodev, ethdev, eventdev and
> >mempool library.
> >
> >In this patch, the build_map_changes function is copied from check-symbol-
> >change.sh to check-tracepoint.sh. The check-tracepoint.sh script uses
> >build_map_changes function to create a map of functions.
> >In the map, the newly added functions, added in the experimental section are
> >identified and their definition are checked for the presence of tracepoint. The
> >checkpatch return error if the tracepoint is not present.
> >
> >For functions for which trace is not needed, they can be added to
> >devtools/trace-skiplist.txt file. The above tracepoint check will be skipped for
> >them.
> >
> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>

Given the amount of string processing, it would be more readable in python.
That is not a show stopper, just a suggestion.

RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Thursday, May 18, 2023 9:04 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; dev@dpdk.org
>Subject: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch
>
>External Email
>
>----------------------------------------------------------------------
>On Thu, 18 May 2023 13:45:29 +0000
>Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
>> >-----Original Message-----
>> >From: Ankur Dwivedi <adwivedi@marvell.com>
>> >Sent: Tuesday, March 7, 2023 5:35 PM
>> >To: dev@dpdk.org
>> >Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran
>> ><jerinj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>
>> >Subject: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch
>> >
>> >This patch adds a validation in checkpatch tool, to check if a
>> >tracepoint is present in any new function added in cryptodev, ethdev,
>> >eventdev and mempool library.
>> >
>> >In this patch, the build_map_changes function is copied from
>> >check-symbol- change.sh to check-tracepoint.sh. The
>> >check-tracepoint.sh script uses build_map_changes function to create a
>map of functions.
>> >In the map, the newly added functions, added in the experimental
>> >section are identified and their definition are checked for the
>> >presence of tracepoint. The checkpatch return error if the tracepoint is not
>present.
>> >
>> >For functions for which trace is not needed, they can be added to
>> >devtools/trace-skiplist.txt file. The above tracepoint check will be
>> >skipped for them.
>> >
>> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>
>Given the amount of string processing, it would be more readable in python.
>That is not a show stopper, just a suggestion.

Hi Thomas,

Please let me know if the shell script in this patch is fine or would a python implementation would be more preferable.

Regards,
Ankur

RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Morten Brørup]

> From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> Sent: Monday, 21 August 2023 15.54
> 
> >From: Stephen Hemminger <stephen@networkplumber.org>
> >Sent: Thursday, May 18, 2023 9:04 PM
> >
> >----------------------------------------------------------------------
> >On Thu, 18 May 2023 13:45:29 +0000
> >Ankur Dwivedi <adwivedi@marvell.com> wrote:
> >
> >> >From: Ankur Dwivedi <adwivedi@marvell.com>
> >> >Sent: Tuesday, March 7, 2023 5:35 PM
> >> >
> >> >This patch adds a validation in checkpatch tool, to check if a
> >> >tracepoint is present in any new function added in cryptodev,
> ethdev,
> >> >eventdev and mempool library.
> >> >
> >> >In this patch, the build_map_changes function is copied from
> >> >check-symbol- change.sh to check-tracepoint.sh. The
> >> >check-tracepoint.sh script uses build_map_changes function to create
> a
> >map of functions.
> >> >In the map, the newly added functions, added in the experimental
> >> >section are identified and their definition are checked for the
> >> >presence of tracepoint. The checkpatch return error if the
> tracepoint is not
> >present.
> >> >
> >> >For functions for which trace is not needed, they can be added to
> >> >devtools/trace-skiplist.txt file. The above tracepoint check will be
> >> >skipped for them.
> >> >
> >> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> >
> >Given the amount of string processing, it would be more readable in
> python.
> >That is not a show stopper, just a suggestion.
> 
> Hi Thomas,
> 
> Please let me know if the shell script in this patch is fine or would a
> python implementation would be more preferable.
> 
> Regards,
> Ankur

The bigger question is: Do we really want to change tracepoints in functions from opt-in to opt-out?

In my opinion, opt-in for trace is more appropriate.

Nonetheless, having a tool to check for tracepoint presence might still be useful for library reviewers and maintainers. And such a tool might be useful for any library, not just the few libraries suggested by this patch.

Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

21/08/2023 16:46, Morten Brørup:
> > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > Sent: Monday, 21 August 2023 15.54
> > 
> > >From: Stephen Hemminger <stephen@networkplumber.org>
> > >Sent: Thursday, May 18, 2023 9:04 PM
> > >
> > >----------------------------------------------------------------------
> > >On Thu, 18 May 2023 13:45:29 +0000
> > >Ankur Dwivedi <adwivedi@marvell.com> wrote:
> > >
> > >> >From: Ankur Dwivedi <adwivedi@marvell.com>
> > >> >Sent: Tuesday, March 7, 2023 5:35 PM
> > >> >
> > >> >This patch adds a validation in checkpatch tool, to check if a
> > >> >tracepoint is present in any new function added in cryptodev,
> > ethdev,
> > >> >eventdev and mempool library.
> > >> >
> > >> >In this patch, the build_map_changes function is copied from
> > >> >check-symbol- change.sh to check-tracepoint.sh. The
> > >> >check-tracepoint.sh script uses build_map_changes function to create
> > a
> > >map of functions.
> > >> >In the map, the newly added functions, added in the experimental
> > >> >section are identified and their definition are checked for the
> > >> >presence of tracepoint. The checkpatch return error if the
> > tracepoint is not
> > >present.
> > >> >
> > >> >For functions for which trace is not needed, they can be added to
> > >> >devtools/trace-skiplist.txt file. The above tracepoint check will be
> > >> >skipped for them.
> > >> >
> > >> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > >
> > >Given the amount of string processing, it would be more readable in
> > python.
> > >That is not a show stopper, just a suggestion.
> > 
> > Hi Thomas,
> > 
> > Please let me know if the shell script in this patch is fine or would a
> > python implementation would be more preferable.

In general, I wonder how much the check is useful compared to the complexity.


> The bigger question is: Do we really want to change tracepoints in functions from opt-in to opt-out?

Yes that's the question: should traces be mandatory in some libs?

> In my opinion, opt-in for trace is more appropriate.

There was some work to add traces everywhere in few libs, so why not maintaining this state?
I don't really like adding a skip list as one more burden for future authors.


> Nonetheless, having a tool to check for tracepoint presence might still be useful for library reviewers and maintainers. And such a tool might be useful for any library, not just the few libraries suggested by this patch.

RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 30 August 2023 18.24
> 
> 21/08/2023 16:46, Morten Brørup:
> > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > > Sent: Monday, 21 August 2023 15.54
> > >
> > > >From: Stephen Hemminger <stephen@networkplumber.org>
> > > >Sent: Thursday, May 18, 2023 9:04 PM
> > > >
> > > >-------------------------------------------------------------------
> ---
> > > >On Thu, 18 May 2023 13:45:29 +0000
> > > >Ankur Dwivedi <adwivedi@marvell.com> wrote:
> > > >
> > > >> >From: Ankur Dwivedi <adwivedi@marvell.com>
> > > >> >Sent: Tuesday, March 7, 2023 5:35 PM
> > > >> >
> > > >> >This patch adds a validation in checkpatch tool, to check if a
> > > >> >tracepoint is present in any new function added in cryptodev,
> > > ethdev,
> > > >> >eventdev and mempool library.
> > > >> >
> > > >> >In this patch, the build_map_changes function is copied from
> > > >> >check-symbol- change.sh to check-tracepoint.sh. The
> > > >> >check-tracepoint.sh script uses build_map_changes function to
> create
> > > a
> > > >map of functions.
> > > >> >In the map, the newly added functions, added in the experimental
> > > >> >section are identified and their definition are checked for the
> > > >> >presence of tracepoint. The checkpatch return error if the
> > > tracepoint is not
> > > >present.
> > > >> >
> > > >> >For functions for which trace is not needed, they can be added
> to
> > > >> >devtools/trace-skiplist.txt file. The above tracepoint check
> will be
> > > >> >skipped for them.
> > > >> >
> > > >> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > > >
> > > >Given the amount of string processing, it would be more readable in
> > > python.
> > > >That is not a show stopper, just a suggestion.
> > >
> > > Hi Thomas,
> > >
> > > Please let me know if the shell script in this patch is fine or
> would a
> > > python implementation would be more preferable.
> 
> In general, I wonder how much the check is useful compared to the
> complexity.
> 
> 
> > The bigger question is: Do we really want to change tracepoints in
> functions from opt-in to opt-out?
> 
> Yes that's the question: should traces be mandatory in some libs?
> 
> > In my opinion, opt-in for trace is more appropriate.
> 
> There was some work to add traces everywhere in few libs, so why not
> maintaining this state?

I still think it's a silly and burdening requirement, but I'm not against it for the libs where it is already a de facto standard.

<irony>
I can imagine similar requirements regarding logging, telemetry and dump functions being imposed on select libs.

Perhaps we should also require that new libs implement all four types of instrumentation, to ensure that only high quality (i.e. fully instrumented) libs are accepted into DPDK.
</irony>

> I don't really like adding a skip list as one more burden for future
> authors.

If the warning omitted from checkpatches refers to the skip list and its location, it is relatively easy for developers to manage.

And reviewers will notice if new functions have been added to the skip list, indicating that trace has been omitted. So there are also advantages to the skip list.

> 
> 
> > Nonetheless, having a tool to check for tracepoint presence might
> still be useful for library reviewers and maintainers. And such a tool
> might be useful for any library, not just the few libraries suggested by
> this patch.
> 
> 
> 

Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Jerin Jacob]

On Thu, Aug 31, 2023 at 12:08 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, 30 August 2023 18.24
> >
> > 21/08/2023 16:46, Morten Brørup:
> > > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > > > Sent: Monday, 21 August 2023 15.54

> >
> > In general, I wonder how much the check is useful compared to the
> > complexity.
> >
> >
> > > The bigger question is: Do we really want to change tracepoints in
> > functions from opt-in to opt-out?
> >
> > Yes that's the question: should traces be mandatory in some libs?
> >
> > > In my opinion, opt-in for trace is more appropriate.
> >
> > There was some work to add traces everywhere in few libs, so why not
> > maintaining this state?
>
> I still think it's a silly and burdening requirement, but I'm not against it for the libs where it is already a de facto standard.
>
> <irony>
> I can imagine similar requirements regarding logging, telemetry and dump functions being imposed on select libs.
>
> Perhaps we should also require that new libs implement all four types of instrumentation, to ensure that only high quality (i.e. fully instrumented) libs are accepted into DPDK.

IMO, There is a lot of effort to put trace points to existing
libraries, without these check, soon the disparity shows up when
someone adds new APIs to library and forget
to add trace points.

It is pretty easy to add trace point and there are a lot of
references, so I don't think there is burden  for a developer
comparing to the effort of adding a new API.

Most of the time, contributors forgets to add trace point because
there is no automatic way to find it.
Also, additional git commits are needs if some decide to add it later.

No strong opinion, If not find not useful, we could mark this patch is
not appliable. Keeping the patch is limbo state is the issue. It is
already reached to v5.
So please decide one way or another.




> </irony>
>
> > I don't really like adding a skip list as one more burden for future
> > authors.
>
> If the warning omitted from checkpatches refers to the skip list and its location, it is relatively easy for developers to manage.
>
> And reviewers will notice if new functions have been added to the skip list, indicating that trace has been omitted. So there are also advantages to the skip list.
>

Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

Let's decide in a techboard meeting whether traces are mandatory or not.


01/09/2023 04:32, Jerin Jacob:
> On Thu, Aug 31, 2023 at 12:08 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Wednesday, 30 August 2023 18.24
> > >
> > > 21/08/2023 16:46, Morten Brørup:
> > > > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > > > > Sent: Monday, 21 August 2023 15.54
> 
> > >
> > > In general, I wonder how much the check is useful compared to the
> > > complexity.
> > >
> > >
> > > > The bigger question is: Do we really want to change tracepoints in
> > > functions from opt-in to opt-out?
> > >
> > > Yes that's the question: should traces be mandatory in some libs?
> > >
> > > > In my opinion, opt-in for trace is more appropriate.
> > >
> > > There was some work to add traces everywhere in few libs, so why not
> > > maintaining this state?
> >
> > I still think it's a silly and burdening requirement, but I'm not against it for the libs where it is already a de facto standard.
> >
> > <irony>
> > I can imagine similar requirements regarding logging, telemetry and dump functions being imposed on select libs.
> >
> > Perhaps we should also require that new libs implement all four types of instrumentation, to ensure that only high quality (i.e. fully instrumented) libs are accepted into DPDK.
> 
> IMO, There is a lot of effort to put trace points to existing
> libraries, without these check, soon the disparity shows up when
> someone adds new APIs to library and forget
> to add trace points.
> 
> It is pretty easy to add trace point and there are a lot of
> references, so I don't think there is burden  for a developer
> comparing to the effort of adding a new API.
> 
> Most of the time, contributors forgets to add trace point because
> there is no automatic way to find it.
> Also, additional git commits are needs if some decide to add it later.
> 
> No strong opinion, If not find not useful, we could mark this patch is
> not appliable. Keeping the patch is limbo state is the issue. It is
> already reached to v5.
> So please decide one way or another.
> 
> 
> 
> 
> > </irony>
> >
> > > I don't really like adding a skip list as one more burden for future
> > > authors.
> >
> > If the warning omitted from checkpatches refers to the skip list and its location, it is relatively easy for developers to manage.
> >
> > And reviewers will notice if new functions have been added to the skip list, indicating that trace has been omitted. So there are also advantages to the skip list.

RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Friday, September 1, 2023 12:59 PM
>To: Morten Brørup <mb@smartsharesystems.com>; Jerin Jacob
><jerinjacobk@gmail.com>
>Cc: Ankur Dwivedi <adwivedi@marvell.com>; Stephen Hemminger
><stephen@networkplumber.org>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; dev@dpdk.org; techboard@dpdk.org
>Subject: Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in
>checkpatch
>
>Let's decide in a techboard meeting whether traces are mandatory or not.

It was agreed in techboard meeting in September, that tracepoints needs to be present for new public API functions in cryptodev, ethdev, eventdev, mempool. Can this patch which automates the check for presence of tracepoint be reviewed and merged if its fine?
>
>
>01/09/2023 04:32, Jerin Jacob:
>> On Thu, Aug 31, 2023 at 12:08 AM Morten Brørup
><mb@smartsharesystems.com> wrote:
>> >
>> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> > > Sent: Wednesday, 30 August 2023 18.24
>> > >
>> > > 21/08/2023 16:46, Morten Brørup:
>> > > > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
>> > > > > Sent: Monday, 21 August 2023 15.54
>>
>> > >
>> > > In general, I wonder how much the check is useful compared to the
>> > > complexity.
>> > >
>> > >
>> > > > The bigger question is: Do we really want to change tracepoints
>> > > > in
>> > > functions from opt-in to opt-out?
>> > >
>> > > Yes that's the question: should traces be mandatory in some libs?
>> > >
>> > > > In my opinion, opt-in for trace is more appropriate.
>> > >
>> > > There was some work to add traces everywhere in few libs, so why
>> > > not maintaining this state?
>> >
>> > I still think it's a silly and burdening requirement, but I'm not against it for
>the libs where it is already a de facto standard.
>> >
>> > <irony>
>> > I can imagine similar requirements regarding logging, telemetry and dump
>functions being imposed on select libs.
>> >
>> > Perhaps we should also require that new libs implement all four types of
>instrumentation, to ensure that only high quality (i.e. fully instrumented) libs
>are accepted into DPDK.
>>
>> IMO, There is a lot of effort to put trace points to existing
>> libraries, without these check, soon the disparity shows up when
>> someone adds new APIs to library and forget to add trace points.
>>
>> It is pretty easy to add trace point and there are a lot of
>> references, so I don't think there is burden  for a developer
>> comparing to the effort of adding a new API.
>>
>> Most of the time, contributors forgets to add trace point because
>> there is no automatic way to find it.
>> Also, additional git commits are needs if some decide to add it later.
>>
>> No strong opinion, If not find not useful, we could mark this patch is
>> not appliable. Keeping the patch is limbo state is the issue. It is
>> already reached to v5.
>> So please decide one way or another.
>>
>>
>>
>>
>> > </irony>
>> >
>> > > I don't really like adding a skip list as one more burden for
>> > > future authors.
>> >
>> > If the warning omitted from checkpatches refers to the skip list and its
>location, it is relatively easy for developers to manage.
>> >
>> > And reviewers will notice if new functions have been added to the skip list,
>indicating that trace has been omitted. So there are also advantages to the
>skip list.
>
>
>

Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

07/03/2023 13:05, Ankur Dwivedi:
> 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.
> 
> 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.

Why did you decide to copy the function in v5,
instead of having a common file usable by different scripts?

RE: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>07/03/2023 13:05, Ankur Dwivedi:
>> 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.
>>
>> 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.
>
>Why did you decide to copy the function in v5, instead of having a common
>file usable by different scripts?
>
There was comments in v2 of the patch that common scripts may not work well and to keep the scripts specialized.

Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

28/11/2023 15:07, Ankur Dwivedi:
> >07/03/2023 13:05, Ankur Dwivedi:
> >> 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.
> >>
> >> 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.
> >
> >Why did you decide to copy the function in v5, instead of having a common
> >file usable by different scripts?
> >
> There was comments in v2 of the patch that common scripts may not work well and to keep the scripts specialized.

I meant you can have a common file specialized in symbols.
In general, you should reply, establish a discussion, so we share the same understanding.

RE: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Tuesday, November 28, 2023 9:25 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in
>checkpatch
>
>28/11/2023 15:07, Ankur Dwivedi:
>> >07/03/2023 13:05, Ankur Dwivedi:
>> >> 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.
>> >>
>> >> 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.
>> >
>> >Why did you decide to copy the function in v5, instead of having a
>> >common file usable by different scripts?
>> >
>> There was comments in v2 of the patch that common scripts may not work
>well and to keep the scripts specialized.
>
>I meant you can have a common file specialized in symbols.
The build_map_changes() (in devtools/check-symbol-change.sh) which is a common function can be moved to a new file named devtools/build-symbol-map.sh.
The build-symbol-map.sh can be included in check-symbol-change.sh and check-tracepoint.sh. 
Please let me know if this is fine.
>In general, you should reply, establish a discussion, so we share the same
>understanding.
>

Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon]

30/11/2023 06:56, Ankur Dwivedi:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 28/11/2023 15:07, Ankur Dwivedi:
> >> > 07/03/2023 13:05, Ankur Dwivedi:
> >> >> 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.
> >> >>
> >> >> 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.
> >> >
> >> >Why did you decide to copy the function in v5, instead of having a
> >> >common file usable by different scripts?
> >> >
> >> There was comments in v2 of the patch that common scripts may not work
> >well and to keep the scripts specialized.
> >
> >I meant you can have a common file specialized in symbols.
> The build_map_changes() (in devtools/check-symbol-change.sh) which is a common function can be moved to a new file named devtools/build-symbol-map.sh.
> The build-symbol-map.sh can be included in check-symbol-change.sh and check-tracepoint.sh. 
> Please let me know if this is fine.

Yes
We can imagine moving more symbol map related funtions in this new file.
What about symbol-map-util.sh as filename?

RE: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Thursday, November 30, 2023 2:11 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in
>checkpatch
>
>30/11/2023 06:56, Ankur Dwivedi:
>> From: Thomas Monjalon <thomas@monjalon.net>
>> > 28/11/2023 15:07, Ankur Dwivedi:
>> >> > 07/03/2023 13:05, Ankur Dwivedi:
>> >> >> 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.
>> >> >>
>> >> >> 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.
>> >> >
>> >> >Why did you decide to copy the function in v5, instead of having a
>> >> >common file usable by different scripts?
>> >> >
>> >> There was comments in v2 of the patch that common scripts may not
>> >> work
>> >well and to keep the scripts specialized.
>> >
>> >I meant you can have a common file specialized in symbols.
>> The build_map_changes() (in devtools/check-symbol-change.sh) which is a
>common function can be moved to a new file named devtools/build-symbol-
>map.sh.
>> The build-symbol-map.sh can be included in check-symbol-change.sh and
>check-tracepoint.sh.
>> Please let me know if this is fine.
>
>Yes
>We can imagine moving more symbol map related funtions in this new file.
>What about symbol-map-util.sh as filename?
I am ok with this filename.
>

[PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

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

-- 
2.25.1

[PATCH v6 1/2] devtools: move build map changes function

[Ankur Dwivedi]

This patch moves the build_map_changes function 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. The
license in check-symbol-change.sh is copied to symbol-map-util.sh.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-symbol-change.sh | 76 +-------------------------------
 devtools/symbol-map-util.sh     | 78 +++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 74 deletions(-)
 create mode 100644 devtools/symbol-map-util.sh

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index 8992214ac8..68613f63cc 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -2,80 +2,8 @@
 # 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
-		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
-
-		# The previous rule catches all .map files, anything else
-		# indicates we left the map chunk.
-		/[-+] [ab]\// {in_map=0}
-
-		# 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 removed.
-		# Triggering this rule sets in_sec to 1, which actives the
-		# symbol rule below
-		/^.*{/ {
-			gsub("+", "");
-			if (in_map == 1) {
-				sec=$(NF-1); in_sec=1;
-			}
-		}
-
-		# This rule identifies 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"
-
-}
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/symbol-map-util.sh
 
 is_stable_section() {
 	[ "$1" != 'EXPERIMENTAL' ] && [ "$1" != 'INTERNAL' ]
diff --git a/devtools/symbol-map-util.sh b/devtools/symbol-map-util.sh
new file mode 100644
index 0000000000..86e4279cdc
--- /dev/null
+++ b/devtools/symbol-map-util.sh
@@ -0,0 +1,78 @@
+#!/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
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
+
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
+
+		# 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 removed.
+		# Triggering this rule sets in_sec to 1, which actives the
+		# symbol rule below
+		/^.*{/ {
+			gsub("+", "");
+			if (in_map == 1) {
+				sec=$(NF-1); in_sec=1;
+			}
+		}
+
+		# This rule identifies 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"
+
+}
-- 
2.25.1

[PATCH v6 2/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

This patch adds a validation in checkpatch tool, to check if a
tracepoint is present in any new function added in cryptodev, ethdev,
eventdev and mempool library.

It uses the existing build_map_changes function to create a map of
functions. In the map, the newly added functions, added in the
experimental section are identified and their definition are checked
for the presence of tracepoint. The checkpatch return error if the
tracepoint is not present.

For functions for which trace is not needed, they can be added to
devtools/trace-skiplist.txt file. The above tracepoint check will be
skipped for them.

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 devtools/check-tracepoint.sh | 148 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 +++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 157 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt

diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh
new file mode 100755
index 0000000000..f957ff780c
--- /dev/null
+++ b/devtools/check-tracepoint.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(C) 2023 Marvell.
+
+selfdir=$(dirname $(readlink -f $0))
+. $selfdir/symbol-map-util.sh
+
+# Library for trace check
+libdir="cryptodev ethdev eventdev mempool"
+
+# Functions for which the trace check can be skipped
+skiplist="$selfdir/trace-skiplist.txt"
+
+find_trace_fn()
+{
+	local fname="$1"
+
+	cat "$fname" | awk -v symname="$2 *\\\(" '
+		# Initialize the variables.
+		# The variable symname provides a pattern to match
+		# "function_name(", zero or more spaces can be present
+		# between function_name and (.
+		BEGIN {state=0; ln_pth=0}
+
+		# Matches the function name, set state=1.
+		$0 ~ symname {state=1}
+
+		# If closing parentheses with semicolon ");" is found, then it
+		# is not the function definition.
+		/) *;/ {
+			if (state == 1) {
+				state=0
+			}
+		}
+
+		/)/ {
+			if (state == 1) {
+				state=2
+				ln_pth=NR
+				next
+			}
+		}
+
+		# If closing parentheses and then opening braces is found in
+		# adjacent line, then this is the start of function
+		# definition. Check for tracepoint in the function definition.
+		# The tracepoint has _trace_ in its name.
+		/{/ {
+			if (state == 2) {
+				if (ln_pth != NR - 1) {
+					state=0
+					next
+				}
+				while (index($0, "}") != 2) {
+					if (index($0, "_trace_") != 0) {
+						exit 0
+					}
+					if (getline <= 0) {
+						break
+					}
+				}
+				exit 1
+			}
+		}
+	'
+}
+
+check_for_tracepoint()
+{
+	local patch="$1"
+	local mapdb="$2"
+	local skip_sym
+	local libname
+	local secname
+	local mname
+	local ret=0
+	local pdir
+	local libp
+	local libn
+	local sym
+	local ar
+
+	while read -r mname symname secname ar; do
+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
+		skip_sym=0
+		libp=0
+
+		if [ "$pdir" != "lib" ]; then
+			continue
+		fi
+
+		for libn in $libdir; do
+			if [ $libn = $libname ]; then
+				libp=1
+				break
+			fi
+		done
+
+		if [ $libp -eq 0 ]; then
+			continue
+		fi
+
+		for sym in $(cat $skiplist); do
+			if [ $sym = $symname ]; then
+				skip_sym=1
+				break
+			fi
+		done
+
+		if [ $skip_sym -eq 1 ]; then
+			continue
+		fi
+
+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
+			# Check if new API is added with tracepoint
+			find_trace_fn $patch $symname
+			if [ $? -eq 1 ]; then
+				ret=1
+				echo -n "ERROR: New function $symname is added "
+				echo -n "without a tracepoint. Please add a tracepoint "
+				echo -n "or add the function $symname in "
+				echo "devtools/trace-skiplist.txt to skip this error."
+			fi
+		fi
+	done < "$mapdb"
+
+	return $ret
+}
+
+trap clean_and_exit_on_sig EXIT
+
+mapfile=`mktemp -t dpdk.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_tracepoint "$patch" "$mapfile"
+exit_code=$?
+rm -f "$mapfile"
+
+exit $exit_code
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 10b79ca2bc..d74e541c9f 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -10,6 +10,7 @@
 . $(dirname $(readlink -f $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
 
 # Enable codespell by default. This can be overwritten from a config file.
 # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
@@ -413,6 +414,14 @@ check () { # <patch-file> <commit>
 		ret=1
 	fi
 
+	! $verbose || printf '\nChecking API additions with tracepoint:\n'
+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
+	if [ $? -ne 0 ] ; then
+		$headline_printed || print_headline "$subject"
+		printf '%s\n' "$report"
+		ret=1
+	fi
+
 	if [ "$tmpinput" != "$1" ]; then
 		rm -f "$tmpinput"
 		trap - INT
diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.25.1

RE: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Ankur Dwivedi <adwivedi@marvell.com>
>Sent: Friday, December 15, 2023 12:14 PM
>To: dev@dpdk.org
>Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>Ankur Dwivedi <adwivedi@marvell.com>
>Subject: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch
>
>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.

Please let me know if this patch series can be merged in DPDK or if there are any comments.
>
>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
>
>--
>2.25.1

Re: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Stephen Hemminger]

On Wed, 17 Jul 2024 12:09:53 +0000
Ankur Dwivedi <adwivedi@marvell.com> wrote:

> >-----Original Message-----
> >From: Ankur Dwivedi <adwivedi@marvell.com>
> >Sent: Friday, December 15, 2023 12:14 PM
> >To: dev@dpdk.org
> >Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> >Ankur Dwivedi <adwivedi@marvell.com>
> >Subject: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch
> >
> >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.  
> 
> Please let me know if this patch series can be merged in DPDK or if there are any comments.

Not sure why the patch got ignored.
Perhaps if check-tracepoint was run first against existing code; add to check-patch later.

And the skip list is empty, is that right? is all of existing cryptodev ethdev ... ok now?

RE: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Tuesday, October 8, 2024 6:11 AM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob <jerinj@marvell.com>
>Subject: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in
>checkpatch
>
>On Wed, 17 Jul 2024 12: 09: 53 +0000 Ankur Dwivedi
><adwivedi@ marvell. com> wrote: > >-----Original Message----- > >From: Ankur
>Dwivedi <adwivedi@ marvell. com> > >Sent: Friday, December 15, 2023 12: 14
>PM > >To: 
>On Wed, 17 Jul 2024 12:09:53 +0000
>Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
>> >-----Original Message-----
>> >From: Ankur Dwivedi <adwivedi@marvell.com>
>> >Sent: Friday, December 15, 2023 12:14 PM
>> >To: dev@dpdk.org
>> >Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran
>> ><jerinj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>
>> >Subject: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch
>> >
>> >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.
>>
>> Please let me know if this patch series can be merged in DPDK or if there are
>any comments.
>
>Not sure why the patch got ignored.
>Perhaps if check-tracepoint was run first against existing code; add to check-
>patch later.

check-tracepoint reads a patch and checks if a newly added function in a library has the trace in it or not. 
For existing code trace can be added manually. Trace was added for existing functions in 23.03 release.
>
>And the skip list is empty, is that right?
Yes. 
If trace is not required for a new library function, the function name can be added in skiplist.
The checkpatch will ignore trace check for that function.
> is all of existing cryptodev ethdev ... ok
>now?

No, it's not completely ok. Few functions does not have trace added.  Majority have trace added.

Re: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Stephen Hemminger]

On Wed, 9 Oct 2024 06:03:58 +0000
Ankur Dwivedi <adwivedi@marvell.com> wrote:

> >> Please let me know if this patch series can be merged in DPDK or if there are  
> >any comments.
> >
> >Not sure why the patch got ignored.
> >Perhaps if check-tracepoint was run first against existing code; add to check-
> >patch later.  
> 
> check-tracepoint reads a patch and checks if a newly added function in a library has the trace in it or not. 
> For existing code trace can be added manually. Trace was added for existing functions in 23.03 release.
> >
> >And the skip list is empty, is that right?  
> Yes. 
> If trace is not required for a new library function, the function name can be added in skiplist.
> The checkpatch will ignore trace check for that function.
> > is all of existing cryptodev ethdev ... ok
> >now?  
> 
> No, it's not completely ok. Few functions does not have trace added.  Majority have trace added.


I wonder if a coccinelle script might be better for this.
Rather than adding more checks to already annoying checkpatch.

RE: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>On Wed, 9 Oct 2024 06:03:58 +0000
>Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
>> >> Please let me know if this patch series can be merged in DPDK or if
>> >> there are
>> >any comments.
>> >
>> >Not sure why the patch got ignored.
>> >Perhaps if check-tracepoint was run first against existing code; add
>> >to check- patch later.
>>
>> check-tracepoint reads a patch and checks if a newly added function in a
>library has the trace in it or not.
>> For existing code trace can be added manually. Trace was added for existing
>functions in 23.03 release.
>> >
>> >And the skip list is empty, is that right?
>> Yes.
>> If trace is not required for a new library function, the function name can be
>added in skiplist.
>> The checkpatch will ignore trace check for that function.
>> > is all of existing cryptodev ethdev ... ok now?
>>
>> No, it's not completely ok. Few functions does not have trace added.
>Majority have trace added.
>
>
>I wonder if a coccinelle script might be better for this.

Not sure if newly added functions can be detected in coccinelle script. In this patch the already existing build_map_changes shell function was detecting this.

>Rather than adding more checks to already annoying checkpatch.

RE: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi]

>-----Original Message-----
>From: Ankur Dwivedi
>Sent: Monday, October 21, 2024 7:37 PM
>To: Stephen Hemminger <stephen@networkplumber.org>
>Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob <jerinj@marvell.com>
>Subject: RE: [EXTERNAL] Re: [PATCH v6 0/2] devtools: add tracepoint check in
>checkpatch
>
>>On Wed, 9 Oct 2024 06:03:58 +0000
>>Ankur Dwivedi <adwivedi@marvell.com> wrote:
>>
>>> >> Please let me know if this patch series can be merged in DPDK or
>>> >> if there are
>>> >any comments.
>>> >
>>> >Not sure why the patch got ignored.
>>> >Perhaps if check-tracepoint was run first against existing code; add
>>> >to check- patch later.
>>>
>>> check-tracepoint reads a patch and checks if a newly added function
>>> in a
>>library has the trace in it or not.
>>> For existing code trace can be added manually. Trace was added for
>>> existing
>>functions in 23.03 release.
>>> >
>>> >And the skip list is empty, is that right?
>>> Yes.
>>> If trace is not required for a new library function, the function
>>> name can be
>>added in skiplist.
>>> The checkpatch will ignore trace check for that function.
>>> > is all of existing cryptodev ethdev ... ok now?
>>>
>>> No, it's not completely ok. Few functions does not have trace added.
>>Majority have trace added.
>>
>>
>>I wonder if a coccinelle script might be better for this.
>
>Not sure if newly added functions can be detected in coccinelle script. In this
>patch the already existing build_map_changes shell function was detecting
>this.
Let me know if any more comments or changes are required in this patch.
>
>>Rather than adding more checks to already annoying checkpatch.