[PATCH net 0/2] selftests: pmtu.sh: Fix cleanup of processes launched in subshell.

[PATCH net 0/2] selftests: pmtu.sh: Fix cleanup of processes launched in subshell.

[Guillaume Nault]

Depending on the options used, pmtu.sh may launch tcpdump and nettest
processes in the background. However it fails to clean them up after
the tests complete.

Patch 1 allows the cleanup() function to read the list of PIDs launched
by the tests.
Patch 2 fixes the way the nettest PIDs are retrieved.

v2:
  * Use tcpdump's immediate mode to capture packets even in short lived
    tests.
  * Add patch 2 to fix the nettest_pids list.

Guillaume Nault (2):
  selftests: pmtu.sh: Kill tcpdump processes launched by subshell.
  selftests: pmtu.sh: Kill nettest processes launched in subshell.

 tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

-- 
2.21.3

[PATCH net 1/2] selftests: pmtu.sh: Kill tcpdump processes launched by subshell.

[Guillaume Nault]

The cleanup() function takes care of killing processes launched by the
test functions. It relies on variables like ${tcpdump_pids} to get the
relevant PIDs. But tests are run in their own subshell, so updated
*_pids values are invisible to other shells. Therefore cleanup() never
sees any process to kill:

$ ./tools/testing/selftests/net/pmtu.sh -t pmtu_ipv4_exception
TEST: ipv4: PMTU exceptions                                         [ OK ]
TEST: ipv4: PMTU exceptions - nexthop objects                       [ OK ]

$ pgrep -af tcpdump
6084 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap
6085 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap
6086 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap
6087 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap
6088 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap
6089 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap
6090 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap
6091 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap
6228 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap
6229 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap
6230 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap
6231 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap
6232 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap
6233 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap
6234 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap
6235 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap

Fix this by running cleanup() in the context of the test subshell.
Now that each test cleans the environment after completion, there's no
need for calling cleanup() again when the next test starts. So let's
drop it from the setup() function. This is okay because cleanup() is
also called when pmtu.sh starts, so even the first test starts in a
clean environment.

Also, use tcpdump's immediate mode. Otherwise it might not have time to
process buffered packets, resulting in missing packets or even empty
pcap files for short tests.

Note: PAUSE_ON_FAIL is still evaluated before cleanup(), so one can
still inspect the test environment upon failure when using -p.

Fixes: a92a0a7b8e7c ("selftests: pmtu: Simplify cleanup and namespace names")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 tools/testing/selftests/net/pmtu.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
index 543ad7513a8e..2e8972573d91 100755
--- a/tools/testing/selftests/net/pmtu.sh
+++ b/tools/testing/selftests/net/pmtu.sh
@@ -865,7 +865,6 @@ setup_ovs_bridge() {
 setup() {
 	[ "$(id -u)" -ne 0 ] && echo "  need to run as root" && return $ksft_skip
 
-	cleanup
 	for arg do
 		eval setup_${arg} || { echo "  ${arg} not supported"; return 1; }
 	done
@@ -876,7 +875,7 @@ trace() {
 
 	for arg do
 		[ "${ns_cmd}" = "" ] && ns_cmd="${arg}" && continue
-		${ns_cmd} tcpdump -s 0 -i "${arg}" -w "${name}_${arg}.pcap" 2> /dev/null &
+		${ns_cmd} tcpdump --immediate-mode -s 0 -i "${arg}" -w "${name}_${arg}.pcap" 2> /dev/null &
 		tcpdump_pids="${tcpdump_pids} $!"
 		ns_cmd=
 	done
@@ -1836,6 +1835,10 @@ run_test() {
 
 	unset IFS
 
+	# Since cleanup() relies on variables modified by this subshell, it
+	# has to run in this context.
+	trap cleanup EXIT
+
 	if [ "$VERBOSE" = "1" ]; then
 		printf "\n##########################################################################\n\n"
 	fi
-- 
2.21.3

[PATCH net 2/2] selftests: pmtu.sh: Kill nettest processes launched in subshell.

[Guillaume Nault]

When using "run_cmd <command> &", then "$!" refers to the PID of the
subshell used to run <command>, not the command itself. Therefore
nettest_pids actually doesn't contain the list of the nettest commands
running in the background. So cleanup() can't kill them and the nettest
processes run until completion (fortunately they have a 5s timeout).

Fix this by defining a new command for running processes in the
background, for which "$!" really refers to the PID of the command run.

Also, double quote variables on the modified lines, to avoid shellcheck
warnings.

Fixes: ece1278a9b81 ("selftests: net: add ESP-in-UDP PMTU test")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 tools/testing/selftests/net/pmtu.sh | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
index 2e8972573d91..694732e4b344 100755
--- a/tools/testing/selftests/net/pmtu.sh
+++ b/tools/testing/selftests/net/pmtu.sh
@@ -374,6 +374,16 @@ run_cmd() {
 	return $rc
 }
 
+run_cmd_bg() {
+	cmd="$*"
+
+	if [ "$VERBOSE" = "1" ]; then
+		printf "    COMMAND: %s &\n" "${cmd}"
+	fi
+
+	$cmd 2>&1 &
+}
+
 # Find the auto-generated name for this namespace
 nsname() {
 	eval echo \$NS_$1
@@ -670,10 +680,10 @@ setup_nettest_xfrm() {
 	[ ${1} -eq 6 ] && proto="-6" || proto=""
 	port=${2}
 
-	run_cmd ${ns_a} nettest ${proto} -q -D -s -x -p ${port} -t 5 &
+	run_cmd_bg "${ns_a}" nettest "${proto}" -q -D -s -x -p "${port}" -t 5
 	nettest_pids="${nettest_pids} $!"
 
-	run_cmd ${ns_b} nettest ${proto} -q -D -s -x -p ${port} -t 5 &
+	run_cmd_bg "${ns_b}" nettest "${proto}" -q -D -s -x -p "${port}" -t 5
 	nettest_pids="${nettest_pids} $!"
 }
 
-- 
2.21.3

Re: [PATCH net 0/2] selftests: pmtu.sh: Fix cleanup of processes launched in subshell.

[Shuah Khan]

On 3/8/22 3:14 PM, Guillaume Nault wrote:
> Depending on the options used, pmtu.sh may launch tcpdump and nettest
> processes in the background. However it fails to clean them up after
> the tests complete.
> 
> Patch 1 allows the cleanup() function to read the list of PIDs launched
> by the tests.
> Patch 2 fixes the way the nettest PIDs are retrieved.
> 
> v2:
>    * Use tcpdump's immediate mode to capture packets even in short lived
>      tests.
>    * Add patch 2 to fix the nettest_pids list.
> 
> Guillaume Nault (2):
>    selftests: pmtu.sh: Kill tcpdump processes launched by subshell.
>    selftests: pmtu.sh: Kill nettest processes launched in subshell.
> 
>   tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 

Both of these look good to me. One nit on commit header. Please
include net in the patch subject line in the future.

e.g: selftests:net pmtu.sh

Tested them on my system. Seeing these messages even after building
nettest:

'nettest' command not found; skipping tests
   xfrm6udp not supported
TEST: vti6: PMTU exceptions (ESP-in-UDP)                            [SKIP]
'nettest' command not found; skipping tests
   xfrm4udp not supported
TEST: vti4: PMTU exceptions (ESP-in-UDP)                            [SKIP]
'nettest' command not found; skipping tests
   xfrm6udprouted not supported
TEST: vti6: PMTU exceptions, routed (ESP-in-UDP)                    [SKIP]
'nettest' command not found; skipping tests
   xfrm4udprouted not supported

Might not be related to this patch though. I jusr ran pmtu.sh from
net directory.


Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH net 0/2] selftests: pmtu.sh: Fix cleanup of processes launched in subshell.

[Guillaume Nault]

On Tue, Mar 08, 2022 at 04:51:46PM -0700, Shuah Khan wrote:
> On 3/8/22 3:14 PM, Guillaume Nault wrote:
> > Depending on the options used, pmtu.sh may launch tcpdump and nettest
> > processes in the background. However it fails to clean them up after
> > the tests complete.
> > 
> > Patch 1 allows the cleanup() function to read the list of PIDs launched
> > by the tests.
> > Patch 2 fixes the way the nettest PIDs are retrieved.
> > 
> > v2:
> >    * Use tcpdump's immediate mode to capture packets even in short lived
> >      tests.
> >    * Add patch 2 to fix the nettest_pids list.
> > 
> > Guillaume Nault (2):
> >    selftests: pmtu.sh: Kill tcpdump processes launched by subshell.
> >    selftests: pmtu.sh: Kill nettest processes launched in subshell.
> > 
> >   tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++----
> >   1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> 
> Both of these look good to me. One nit on commit header. Please
> include net in the patch subject line in the future.
> 
> e.g: selftests:net pmtu.sh

Thanks, I'll do that next time (I just reused keywords used by other
commits for this file).

> Tested them on my system. Seeing these messages even after building
> nettest:
> 
> 'nettest' command not found; skipping tests
>   xfrm6udp not supported
> TEST: vti6: PMTU exceptions (ESP-in-UDP)                            [SKIP]
> 'nettest' command not found; skipping tests
>   xfrm4udp not supported
> TEST: vti4: PMTU exceptions (ESP-in-UDP)                            [SKIP]
> 'nettest' command not found; skipping tests
>   xfrm6udprouted not supported
> TEST: vti6: PMTU exceptions, routed (ESP-in-UDP)                    [SKIP]
> 'nettest' command not found; skipping tests
>   xfrm4udprouted not supported
> 
> Might not be related to this patch though. I jusr ran pmtu.sh from
> net directory.

Personally I just modified my PATH before running pmtu.sh. Not sure if
there's a better way. But in any case, that's not related to this
patch.

> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> thanks,
> -- Shuah
> 

Re: [PATCH net 0/2] selftests: pmtu.sh: Fix cleanup of processes launched in subshell.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 8 Mar 2022 23:14:57 +0100 you wrote:
> Depending on the options used, pmtu.sh may launch tcpdump and nettest
> processes in the background. However it fails to clean them up after
> the tests complete.
> 
> Patch 1 allows the cleanup() function to read the list of PIDs launched
> by the tests.
> Patch 2 fixes the way the nettest PIDs are retrieved.
> 
> [...]

Here is the summary with links:
  - [net,1/2] selftests: pmtu.sh: Kill tcpdump processes launched by subshell.
    https://git.kernel.org/netdev/net/c/18dfc667550f
  - [net,2/2] selftests: pmtu.sh: Kill nettest processes launched in subshell.
    https://git.kernel.org/netdev/net/c/94a4a4fe4c69

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html