[PATCH net-next v2 0/7] selftests: rds: Log collection, TAP compliance and cleanups

[PATCH net-next v2 0/7] selftests: rds: Log collection, TAP compliance and cleanups

[Allison Henderson]

This series is a set of bug fixes and improvements for the rds
selftests.

Patch 1 bumps the kselftest timeout from 400s to 800s. The original
limit was developed against a lean config, but the kselftest harness
counts boot time and gcov log collection against the limit, so a
default config with gcov enabled needs more headroom. 

Patch 2 corrects some typos in the run.sh USAGE string and removes an
unused "-g" flag.

Patch 3 silences a handful of pylint warnings in test.py: it adds a
module docstring, suppresses the warnings tied to the sys.path.append
import trick, marks the long lived tcpdump Popen with disable-next
consider-using-with, and drops unused exception variables from two
BlockingIOError except clauses.

Patch 4 adds a -t flag to run.sh so the timeout can be overridden
if needed.

Patch 5 fixes log collection under vng. The vng guest inherits the
host's systemd config, so tmp and debugfs may not be mounted by
default; without them tcpdump can't write pcaps and gcov data is
silently dropped. The patch mounts each filesystem when it isn't
already mounted, and also specifies the --root folder so that gcov
can still find the kernel source when it is run from the ksft
test directory.

Patch 6 hoists pcap collection into a helper and calls it from the
timeout signal handler so dumps are preserved when a test times out.

Patch 7 makes the test output TAP compliant so the kselftest runner
parses results correctly.

Questions, comments and feedback appreciated!

Thanks everyone!
Allison

Change log:
v2:
   [PATCH net-next v2 3/7] selftests: rds: Fix more pylint errors
      NEW

   [PATCH net-next v2 6/7] selftests: rds: Collect pcaps on timeout
      Fixed pylint errors in collect_pcaps()

   [PATCH net-next v2 7/7] selftests: rds: Make rds selftests TAP compliant
      Fixed pylint errors from ksft import

Allison Henderson (7):
  selftests: rds: Increase selftest timeout
  selftests: rds: Update USAGE string for run.sh
  selftests: rds: Fix more pylint errors
  selftests: rds: Add timeout flag to run.sh
  selftests: rds: Fix gcov and pcap collection
  selftests: rds: Collect pcaps on timeout
  selftests: rds: Make rds selftests TAP compliant

 tools/testing/selftests/net/rds/run.sh   | 42 ++++++++----
 tools/testing/selftests/net/rds/settings |  2 +-
 tools/testing/selftests/net/rds/test.py  | 83 +++++++++++++++---------
 3 files changed, 86 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH net-next v2 1/7] selftests: rds: Increase selftest timeout

[Allison Henderson]

The 400s time out was originally developed under a leaner
kernel config that booted much faster than a default config.
Boot up is included as part of the over all test runtime, as
well as any log collection done when the test is complete.
A slower config combined with the gcov enabled test means
we'll need more time to accommodate the boot up and log
collection.  So, bump time out to 800s.

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/settings | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/rds/settings b/tools/testing/selftests/net/rds/settings
index d2009a64589c..8cb41e6a83cc 100644
--- a/tools/testing/selftests/net/rds/settings
+++ b/tools/testing/selftests/net/rds/settings
@@ -1 +1 @@
-timeout=400
+timeout=800
-- 
2.25.1

[PATCH net-next v2 2/7] selftests: rds: Update USAGE string for run.sh

[Allison Henderson]

The run.sh script does not have a -g flag.  Update USAGE string with
correct flags.  Aslo fix typo packet_duplcate -> packet_duplicate

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/run.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/rds/run.sh b/tools/testing/selftests/net/rds/run.sh
index 897d17d1b8db..73a9b986b0ef 100755
--- a/tools/testing/selftests/net/rds/run.sh
+++ b/tools/testing/selftests/net/rds/run.sh
@@ -171,7 +171,7 @@ while getopts "d:l:c:u:" opt; do
       ;;
     :)
       echo "USAGE: run.sh [-d logdir] [-l packet_loss] [-c packet_corruption]" \
-           "[-u packet_duplcate] [-g]"
+           "[-u packet_duplicate]"
       exit 1
       ;;
     ?)
-- 
2.25.1

[PATCH net-next v2 3/7] selftests: rds: Fix more pylint errors

[Allison Henderson]

This patch fixes a few pylint errors in test.py. Remove unused exception
variables from except blocks, and disable warnings for imports that cannot
appear at the start of the module.  Also disable warnings for the
tcpdump processes.  The suggestion to use a with block does not apply
here since the process needs to outlive the parent to collect the dumps.
Lastly add the module docstring at the top of the module.

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/test.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
index 93e23e8b256c..4b6ffbb3a81c 100755
--- a/tools/testing/selftests/net/rds/test.py
+++ b/tools/testing/selftests/net/rds/test.py
@@ -1,5 +1,8 @@
 #! /usr/bin/env python3
 # SPDX-License-Identifier: GPL-2.0
+"""
+This module provides functional testing for the net/rds component.
+"""
 
 import argparse
 import ctypes
@@ -17,7 +20,8 @@ import shutil
 # Allow utils module to be imported from different directory
 this_dir = os.path.dirname(os.path.realpath(__file__))
 sys.path.append(os.path.join(this_dir, "../"))
-from lib.py.utils import ip
+# pylint: disable-next=wrong-import-position,import-error,no-name-in-module
+from lib.py.utils import ip # noqa: E402
 
 libc = ctypes.cdll.LoadLibrary('libc.so.6')
 setns = libc.setns
@@ -129,6 +133,7 @@ tcpdump_procs = []
 for net in [NET0, NET1]:
     pcap = logdir+'/'+net+'.pcap'
     fd, pcap_tmp = tempfile.mkstemp(suffix=".pcap", prefix=f"{net}-", dir="/tmp")
+    # pylint: disable-next=consider-using-with
     p = subprocess.Popen(
         ['ip', 'netns', 'exec', net,
          '/usr/sbin/tcpdump', '-i', 'any', '-w', pcap_tmp])
@@ -192,7 +197,7 @@ while nr_send < NUM_PACKETS:
             send_hashes.setdefault((sender.fileno(), receiver.fileno()),
                     hashlib.sha256()).update(f'<{send_data}>'.encode('utf-8'))
             nr_send = nr_send + 1
-        except BlockingIOError as e:
+        except BlockingIOError:
             break
         except OSError as e:
             if e.errno in [errno.ENOBUFS, errno.ECONNRESET, errno.EPIPE]:
@@ -214,7 +219,7 @@ while nr_send < NUM_PACKETS:
                             receiver.fileno()), hashlib.sha256()).update(
                                     f'<{recv_data}>'.encode('utf-8'))
                         nr_recv = nr_recv + 1
-                    except BlockingIOError as e:
+                    except BlockingIOError:
                         break
 
     # exercise net/rds/tcp.c:rds_tcp_sysctl_reset()
-- 
2.25.1

[PATCH net-next v2 4/7] selftests: rds: Add timeout flag to run.sh

[Allison Henderson]

Add a -t flag to run.sh to optionally override the default
timeout.  The --timeout flag is already supported in test.py,
so just add the shorthand -t flag

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/run.sh  | 11 ++++++++---
 tools/testing/selftests/net/rds/test.py |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/rds/run.sh b/tools/testing/selftests/net/rds/run.sh
index 73a9b986b0ef..bc2e53126aab 100755
--- a/tools/testing/selftests/net/rds/run.sh
+++ b/tools/testing/selftests/net/rds/run.sh
@@ -154,8 +154,9 @@ LOG_DIR="$current_dir"/rds_logs
 PLOSS=0
 PCORRUPT=0
 PDUP=0
+TIMEOUT=$timeout
 GENERATE_GCOV_REPORT=1
-while getopts "d:l:c:u:" opt; do
+while getopts "d:l:c:u:t:" opt; do
   case ${opt} in
     d)
       LOG_DIR=${OPTARG}
@@ -166,12 +167,15 @@ while getopts "d:l:c:u:" opt; do
     c)
       PCORRUPT=${OPTARG}
       ;;
+    t)
+      TIMEOUT=${OPTARG}
+      ;;
     u)
       PDUP=${OPTARG}
       ;;
     :)
       echo "USAGE: run.sh [-d logdir] [-l packet_loss] [-c packet_corruption]" \
-           "[-u packet_duplicate]"
+           "[-u packet_duplicate] [-t timeout]"
       exit 1
       ;;
     ?)
@@ -198,7 +202,8 @@ echo running RDS tests...
 echo Traces will be logged to "$TRACE_FILE"
 rm -f "$TRACE_FILE"
 strace -T -tt -o "$TRACE_FILE" python3 "$(dirname "$0")/test.py" \
-	--timeout "$timeout" -d "$LOG_DIR" -l "$PLOSS" -c "$PCORRUPT" -u "$PDUP"
+	-t "$TIMEOUT" -d "$LOG_DIR" -l "$PLOSS" -c "$PCORRUPT" \
+	-u "$PDUP"
 
 test_rc=$?
 dmesg > "${LOG_DIR}/dmesg.out"
diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
index 4b6ffbb3a81c..d48533505f0f 100755
--- a/tools/testing/selftests/net/rds/test.py
+++ b/tools/testing/selftests/net/rds/test.py
@@ -83,7 +83,7 @@ parser = argparse.ArgumentParser(description="init script args",
                   formatter_class=argparse.ArgumentDefaultsHelpFormatter)
 parser.add_argument("-d", "--logdir", action="store",
                     help="directory to store logs", default="/tmp")
-parser.add_argument('--timeout', help="timeout to terminate hung test",
+parser.add_argument('-t', '--timeout', help="timeout to terminate hung test",
                     type=int, default=0)
 parser.add_argument('-l', '--loss', help="Simulate tcp packet loss",
                     type=int, default=0)
-- 
2.25.1

[PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Allison Henderson]

The vng guest shares the host filesystem via 9p and runs a minimal
systemd inherited from the host's /lib/systemd/system/. As a result,
which filesystems get auto-mounted in the guest depends on the host OS
and its systemd version.

The tcpdump pcaps are initially saved to /tmp because 9p does not support
chown which tcpdump requires. But whether /tmp is already a tmpfs depends
on the host's systemd configuration and may still sometimes fail if /tmp
is not mounted by default. Fix this by mounting tmpfs on /tmp in run.sh
when it is not already a separately mounted filesystem.

A similar dependency exists for gcov. debugfs is not mounted automatically
in vng guest, so the gcov data copy from /sys/kernel/debug/gcov/
silently finds nothing depending on whether debugfs is mounted by default
on the host OS. Fix this by mounting debugfs in run.sh before copying the
gcda files.

Finally, when invoked through the kselftest runner, the working directory
is the test directory rather than the kernel source root. gcovr defaults
--root to the current working directory, causing it to filter out all
coverage data for files under net/rds/ since they are not under the test
directory. Fix this by passing --root to gcovr explicitly.

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/run.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/rds/run.sh b/tools/testing/selftests/net/rds/run.sh
index bc2e53126aab..3fc116d23410 100755
--- a/tools/testing/selftests/net/rds/run.sh
+++ b/tools/testing/selftests/net/rds/run.sh
@@ -197,6 +197,13 @@ COVR_DIR="${LOG_DIR}/coverage/"
 mkdir -p  "$LOG_DIR"
 mkdir -p "$COVR_DIR"
 
+# tcpdump saves pcaps to /tmp because it requires chown to save the
+# pcap but chown is not supported by 9p.  Mount tmpfs on /tmp if it is
+# not already a separate filesystem
+if ! mountpoint -q /tmp 2>/dev/null; then
+	mount -t tmpfs tmpfs /tmp
+fi
+
 set +e
 echo running RDS tests...
 echo Traces will be logged to "$TRACE_FILE"
@@ -210,6 +217,12 @@ dmesg > "${LOG_DIR}/dmesg.out"
 
 if [ "$GENERATE_GCOV_REPORT" -eq 1 ]; then
        echo saving coverage data...
+
+       # Ensure debugfs is mounted
+       if ! test -d /sys/kernel/debug/gcov; then
+               mount -t debugfs debugfs /sys/kernel/debug 2>/dev/null || true
+       fi
+
        (set +x; cd /sys/kernel/debug/gcov; find ./* -name '*.gcda' | \
        while read -r f
        do
@@ -218,7 +231,7 @@ if [ "$GENERATE_GCOV_REPORT" -eq 1 ]; then
 
        echo running gcovr...
        gcovr -s --html-details --gcov-executable "$GCOV_CMD" --gcov-ignore-parse-errors \
-             -o "${COVR_DIR}/gcovr" "${ksrc_dir}/net/rds/"
+             --root "${ksrc_dir}" -o "${COVR_DIR}/gcovr" "${ksrc_dir}/net/rds/"
 else
        echo "Coverage report will be skipped"
 fi
-- 
2.25.1

[PATCH net-next v2 6/7] selftests: rds: Collect pcaps on timeout

[Allison Henderson]

The timeout signal handler for the rds selftests currently just
exits when the time limit is exceeded, and forgets to collect the
network dumps.  Which can be valueable for discerning why the test
timed out in the first place.  Fix this by hoisting the network
dump collection into a helper function, and call it from the
signal handler before exiting

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/test.py | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
index d48533505f0f..1c7aebddeb61 100755
--- a/tools/testing/selftests/net/rds/test.py
+++ b/tools/testing/selftests/net/rds/test.py
@@ -70,11 +70,21 @@ def netns_socket(netns, *sock_args):
     u1.close()
     return socket.fromfd(fds[0], *sock_args)
 
+def collect_pcaps():
+    """Stop tcpdump processes and move their pcaps into the log dir."""
+    print("Stopping network packet captures")
+    for proc, tmp_path, dest_path, fno in tcpdump_procs:
+        proc.terminate()
+        proc.wait()
+        os.close(fno)
+        shutil.move(tmp_path, dest_path)
+
 def signal_handler(_sig, _frame):
     """
     Test timed out signal handler
     """
     print('Test timed out')
+    collect_pcaps()
     sys.exit(1)
 
 #Parse out command line arguments.  We take an optional
@@ -251,13 +261,7 @@ for s in sockets:
                 pass
 
 print(f"getsockopt(): {nr_success}/{nr_error}")
-
-print("Stopping network packet captures")
-for p, pcap_tmp, pcap, fd in tcpdump_procs:
-    p.terminate()
-    p.wait()
-    os.close(fd)
-    shutil.move(pcap_tmp, pcap)
+collect_pcaps()
 
 # We're done sending and receiving stuff, now let's check if what
 # we received is what we sent.
-- 
2.25.1

[PATCH net-next v2 7/7] selftests: rds: Make rds selftests TAP compliant

[Allison Henderson]

This patch updates the rds selftests output to be TAP compliant.

Use ksft_pr() to mark debug output with a leading '# ' so that TAP
parsers treat it as commentary, and convert all informational print()
calls to use ksft_pr(). sys.exit(0) is changed to os._exit(0) to
avoid duplicate prints from the buffered TAP output. The console
output from the tcpdump subprocess is silenced, and the gcov console
output is redirected to a gcovr.log.

Finally adjust the exit path so that the hash check loop sets a
return code instead exiting directly. Then print the TAP results
and totals lines before exiting.

Signed-off-by: Allison Henderson <achender@kernel.org>
---
 tools/testing/selftests/net/rds/run.sh  | 18 +++++----
 tools/testing/selftests/net/rds/test.py | 54 ++++++++++++++++---------
 2 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/net/rds/run.sh b/tools/testing/selftests/net/rds/run.sh
index 3fc116d23410..805cd0915585 100755
--- a/tools/testing/selftests/net/rds/run.sh
+++ b/tools/testing/selftests/net/rds/run.sh
@@ -205,8 +205,8 @@ if ! mountpoint -q /tmp 2>/dev/null; then
 fi
 
 set +e
-echo running RDS tests...
-echo Traces will be logged to "$TRACE_FILE"
+echo "# running RDS tests..."
+echo "# Traces will be logged to $TRACE_FILE"
 rm -f "$TRACE_FILE"
 strace -T -tt -o "$TRACE_FILE" python3 "$(dirname "$0")/test.py" \
 	-t "$TIMEOUT" -d "$LOG_DIR" -l "$PLOSS" -c "$PCORRUPT" \
@@ -216,7 +216,7 @@ test_rc=$?
 dmesg > "${LOG_DIR}/dmesg.out"
 
 if [ "$GENERATE_GCOV_REPORT" -eq 1 ]; then
-       echo saving coverage data...
+       echo "# saving coverage data..."
 
        # Ensure debugfs is mounted
        if ! test -d /sys/kernel/debug/gcov; then
@@ -229,17 +229,19 @@ if [ "$GENERATE_GCOV_REPORT" -eq 1 ]; then
                cat < "/sys/kernel/debug/gcov/$f" > "/$f"
        done)
 
-       echo running gcovr...
+       echo "# running gcovr..."
        gcovr -s --html-details --gcov-executable "$GCOV_CMD" --gcov-ignore-parse-errors \
-             --root "${ksrc_dir}" -o "${COVR_DIR}/gcovr" "${ksrc_dir}/net/rds/"
+             --root "${ksrc_dir}" -o "${COVR_DIR}/gcovr" "${ksrc_dir}/net/rds/" \
+             > "${LOG_DIR}/gcovr.log" 2>&1
+       echo "# gcovr log: ${LOG_DIR}/gcovr.log"
 else
-       echo "Coverage report will be skipped"
+       echo "# Coverage report will be skipped"
 fi
 
 if [ "$test_rc" -eq 0 ]; then
-	echo "PASS: Test completed successfully"
+	echo "# PASS: Test completed successfully"
 else
-	echo "FAIL: Test failed"
+	echo "# FAIL: Test failed"
 fi
 
 exit "$test_rc"
diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
index 1c7aebddeb61..530ad9f9acba 100755
--- a/tools/testing/selftests/net/rds/test.py
+++ b/tools/testing/selftests/net/rds/test.py
@@ -22,6 +22,8 @@ this_dir = os.path.dirname(os.path.realpath(__file__))
 sys.path.append(os.path.join(this_dir, "../"))
 # pylint: disable-next=wrong-import-position,import-error,no-name-in-module
 from lib.py.utils import ip # noqa: E402
+# pylint: disable-next=wrong-import-position,import-error,no-name-in-module
+from lib.py.ksft import ksft_pr # noqa: E402
 
 libc = ctypes.cdll.LoadLibrary('libc.so.6')
 setns = libc.setns
@@ -61,7 +63,7 @@ def netns_socket(netns, *sock_args):
         # send resulting socket to parent
         socket.send_fds(u0, [], [sock.fileno()])
 
-        sys.exit(0)
+        os._exit(0)
 
     # receive socket from child
     _, fds, _, _ = socket.recv_fds(u1, 0, 1)
@@ -72,7 +74,7 @@ def netns_socket(netns, *sock_args):
 
 def collect_pcaps():
     """Stop tcpdump processes and move their pcaps into the log dir."""
-    print("Stopping network packet captures")
+    ksft_pr("Stopping network packet captures")
     for proc, tmp_path, dest_path, fno in tcpdump_procs:
         proc.terminate()
         proc.wait()
@@ -83,8 +85,9 @@ def signal_handler(_sig, _frame):
     """
     Test timed out signal handler
     """
-    print('Test timed out')
+    ksft_pr("Test timed out")
     collect_pcaps()
+    print("not ok 1 rds selftest")
     sys.exit(1)
 
 #Parse out command line arguments.  We take an optional
@@ -146,7 +149,8 @@ for net in [NET0, NET1]:
     # pylint: disable-next=consider-using-with
     p = subprocess.Popen(
         ['ip', 'netns', 'exec', net,
-         '/usr/sbin/tcpdump', '-i', 'any', '-w', pcap_tmp])
+         '/usr/sbin/tcpdump', '-i', 'any', '-w', pcap_tmp],
+         stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
     tcpdump_procs.append((p, pcap_tmp, pcap, fd))
 
 # simulate packet loss, duplication and corruption
@@ -155,6 +159,9 @@ for net, iface in [(NET0, VETH0), (NET1, VETH1)]:
          corrupt {PACKET_CORRUPTION} loss {PACKET_LOSS} duplicate  \
          {PACKET_DUPLICATE}")
 
+print("TAP version 13")
+print("1..1")
+
 # add a timeout
 if args.timeout > 0:
     signal.alarm(args.timeout)
@@ -193,7 +200,7 @@ nr_recv = 0
 
 while nr_send < NUM_PACKETS:
     # Send as much as we can without blocking
-    print("sending...", nr_send, nr_recv)
+    ksft_pr("sending...", nr_send, nr_recv)
     while nr_send < NUM_PACKETS:
         send_data = hashlib.sha256(
             f'packet {nr_send}'.encode('utf-8')).hexdigest().encode('utf-8')
@@ -215,7 +222,7 @@ while nr_send < NUM_PACKETS:
             raise
 
     # Receive as much as we can without blocking
-    print("receiving...", nr_send, nr_recv)
+    ksft_pr("receiving...", nr_send, nr_recv)
     while nr_recv < nr_send:
         for fileno, eventmask in ep.poll():
             receiver = fileno_to_socket[fileno]
@@ -237,7 +244,7 @@ while nr_send < NUM_PACKETS:
         ip(f"netns exec {net} /usr/sbin/sysctl net.rds.tcp.rds_tcp_rcvbuf=10000")
         ip(f"netns exec {net} /usr/sbin/sysctl net.rds.tcp.rds_tcp_sndbuf=10000")
 
-print("done", nr_send, nr_recv)
+ksft_pr("done", nr_send, nr_recv)
 
 # the Python socket module doesn't know these
 RDS_INFO_FIRST = 10000
@@ -260,25 +267,34 @@ for s in sockets:
                 # ignore
                 pass
 
-print(f"getsockopt(): {nr_success}/{nr_error}")
+ksft_pr(f"getsockopt(): {nr_success}/{nr_error}")
 collect_pcaps()
 
 # We're done sending and receiving stuff, now let's check if what
 # we received is what we sent.
+ret = 0
 for (sender, receiver), send_hash in send_hashes.items():
     recv_hash = recv_hashes.get((sender, receiver))
 
     if recv_hash is None:
-        print("FAIL: No data received")
-        sys.exit(1)
+        ksft_pr("FAIL: No data received")
+        ret = 1
+        break
 
     if send_hash.hexdigest() != recv_hash.hexdigest():
-        print("FAIL: Send/recv mismatch")
-        print("hash expected:", send_hash.hexdigest())
-        print("hash received:", recv_hash.hexdigest())
-        sys.exit(1)
-
-    print(f"{sender}/{receiver}: ok")
-
-print("Success")
-sys.exit(0)
+        ksft_pr("FAIL: Send/recv mismatch")
+        ksft_pr("hash expected:", send_hash.hexdigest())
+        ksft_pr("hash received:", recv_hash.hexdigest())
+        ret = 1
+        break
+
+    ksft_pr(f"{sender}/{receiver}: ok")
+
+if ret == 0:
+    ksft_pr("Success")
+    print("ok 1 rds selftest")
+else:
+    print("not ok 1 rds selftest")
+
+ksft_pr(f"Totals: pass:{1-ret} fail:{ret} skip:0")
+sys.exit(ret)
-- 
2.25.1

Re: [PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: rds: Fix gcov and pcap collection

This commit addresses dependencies on host systemd configurations in the vng
guest. It adds mounts for tmpfs on /tmp and debugfs to ensure pcap and gcov
data collection work, and updates the gcovr root path.

> diff --git a/tools/testing/selftests/net/rds/run.sh b/tools/testing/selftests/net/rds/run.sh
> index bc2e53126aab..3fc116d23410 100755
> --- a/tools/testing/selftests/net/rds/run.sh
> +++ b/tools/testing/selftests/net/rds/run.sh
> @@ -197,6 +197,13 @@ COVR_DIR="${LOG_DIR}/coverage/"
>  mkdir -p  "$LOG_DIR"
>  mkdir -p "$COVR_DIR"
>  
> +# tcpdump saves pcaps to /tmp because it requires chown to save the
> +# pcap but chown is not supported by 9p.  Mount tmpfs on /tmp if it is
> +# not already a separate filesystem
> +if ! mountpoint -q /tmp 2>/dev/null; then
> +	mount -t tmpfs tmpfs /tmp
> +fi
> +

Could this introduce a regression when the test is run directly on a host
workstation rather than inside an isolated VM?

If /tmp is a regular directory on the root filesystem, mounting tmpfs over it
will instantly hide all existing files and UNIX domain sockets in /tmp.

Since there is no cleanup trap to unmount it on exit or failure, does this
leave the host system in a degraded state?

[ ... ]

>  if [ "$GENERATE_GCOV_REPORT" -eq 1 ]; then
>         echo saving coverage data...
> +
> +       # Ensure debugfs is mounted
> +       if ! test -d /sys/kernel/debug/gcov; then
> +               mount -t debugfs debugfs /sys/kernel/debug 2>/dev/null || true
> +       fi
> +

If the test is run against a kernel that does not have CONFIG_GCOV_KERNEL
enabled, the gcov directory will not exist even when debugfs is correctly
mounted.

Will this condition cause a regression where the script repeatedly mounts
debugfs on every run, stacking redundant shadow mounts over
/sys/kernel/debug since they are never unmounted?
-- 
pw-bot: cr

Re: [PATCH net-next v2 6/7] selftests: rds: Collect pcaps on timeout

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: rds: Collect pcaps on timeout

This commit updates the RDS selftest to collect network packet captures
when a timeout occurs. It moves the packet capture cleanup logic into a
new helper function and calls it from the signal handler before exiting.

> diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
> --- a/tools/testing/selftests/net/rds/test.py
> +++ b/tools/testing/selftests/net/rds/test.py
[ ... ]
> @@ -70,11 +70,21 @@ def netns_socket(netns, *sock_args):
>      u1.close()
>      return socket.fromfd(fds[0], *sock_args)
>  
> +def collect_pcaps():
> +    """Stop tcpdump processes and move their pcaps into the log dir."""
> +    print("Stopping network packet captures")
> +    for proc, tmp_path, dest_path, fno in tcpdump_procs:
> +        proc.terminate()
> +        proc.wait()

Could this proc.wait() block indefinitely and defeat the test timeout?

If a child process is unresponsive, the timeout signal handler will get
stuck waiting for it. Using proc.wait(timeout=...) might ensure the
timeout path remains robust and doesn't hang the test infrastructure.

> +        os.close(fno)
> +        shutil.move(tmp_path, dest_path)

Can this cause unhandled exceptions if the timeout triggers during or
after the normal script cleanup phase?

Because collect_pcaps() is called from both the normal execution path and
the timeout signal handler, it might be executed twice. Since it loops
over the global tcpdump_procs list without clearing processed items, a
second run will attempt to close already closed file descriptors and move
already moved files.

In Python, this raises OSError (EBADF) and FileNotFoundError, which would
abort the signal handler with a traceback instead of executing the clean
sys.exit(1).

Re: [PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Jakub Kicinski]

On Wed, 29 Apr 2026 19:42:06 -0700 Jakub Kicinski wrote:
> > +# tcpdump saves pcaps to /tmp because it requires chown to save the
> > +# pcap but chown is not supported by 9p.  Mount tmpfs on /tmp if it is
> > +# not already a separate filesystem
> > +if ! mountpoint -q /tmp 2>/dev/null; then
> > +	mount -t tmpfs tmpfs /tmp
> > +fi
> > +  
> 
> Could this introduce a regression when the test is run directly on a host
> workstation rather than inside an isolated VM?
> 
> If /tmp is a regular directory on the root filesystem, mounting tmpfs over it
> will instantly hide all existing files and UNIX domain sockets in /tmp.
> 
> Since there is no cleanup trap to unmount it on exit or failure, does this
> leave the host system in a degraded state?

I share Sashiko's mixed feelings here. Do other tests mount /tmp ?
Seems like something that belongs in the "CI setup", external
to ksft itself.

point #2 my vng does have an overlayfs mounted over /tmp so the
mountpoint check doesn't trigger IDK if this is what you meant 
or I have a different version.

Re: [PATCH net-next v2 6/7] selftests: rds: Collect pcaps on timeout

[Allison Henderson]

On Wed, 2026-04-29 at 19:42 -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> selftests: rds: Collect pcaps on timeout
> 
> This commit updates the RDS selftest to collect network packet captures
> when a timeout occurs. It moves the packet capture cleanup logic into a
> new helper function and calls it from the signal handler before exiting.
> 
> > diff --git a/tools/testing/selftests/net/rds/test.py b/tools/testing/selftests/net/rds/test.py
> > --- a/tools/testing/selftests/net/rds/test.py
> > +++ b/tools/testing/selftests/net/rds/test.py
> [ ... ]
> > @@ -70,11 +70,21 @@ def netns_socket(netns, *sock_args):
> >      u1.close()
> >      return socket.fromfd(fds[0], *sock_args)
> >  
> > +def collect_pcaps():
> > +    """Stop tcpdump processes and move their pcaps into the log dir."""
> > +    print("Stopping network packet captures")
> > +    for proc, tmp_path, dest_path, fno in tcpdump_procs:
> > +        proc.terminate()
> > +        proc.wait()
> 
> Could this proc.wait() block indefinitely and defeat the test timeout?
> 
> If a child process is unresponsive, the timeout signal handler will get
> stuck waiting for it. Using proc.wait(timeout=...) might ensure the
> timeout path remains robust and doesn't hang the test infrastructure.
Good catch, I will add a timeout

> 
> > +        os.close(fno)
> > +        shutil.move(tmp_path, dest_path)
> 
> Can this cause unhandled exceptions if the timeout triggers during or
> after the normal script cleanup phase?
> 
> Because collect_pcaps() is called from both the normal execution path and
> the timeout signal handler, it might be executed twice. Since it loops
> over the global tcpdump_procs list without clearing processed items, a
> second run will attempt to close already closed file descriptors and move
> already moved files.
> 
> In Python, this raises OSError (EBADF) and FileNotFoundError, which would
> abort the signal handler with a traceback instead of executing the clean
> sys.exit(1).
I see, I think if we change the for loop to a while loop that pop() items out of tcpdump_procs, that should make it safe
to call twice, if the timeout fires during or after normal cleanup.


If that sounds good, I can send that in a v3.

Thanks!
Allison

Re: [PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Allison Henderson]

On Wed, 2026-04-29 at 19:48 -0700, Jakub Kicinski wrote:
> On Wed, 29 Apr 2026 19:42:06 -0700 Jakub Kicinski wrote:
> > > +# tcpdump saves pcaps to /tmp because it requires chown to save the
> > > +# pcap but chown is not supported by 9p.  Mount tmpfs on /tmp if it is
> > > +# not already a separate filesystem
> > > +if ! mountpoint -q /tmp 2>/dev/null; then
> > > +	mount -t tmpfs tmpfs /tmp
> > > +fi
> > > +  
> > 
> > Could this introduce a regression when the test is run directly on a host
> > workstation rather than inside an isolated VM?
> > 
> > If /tmp is a regular directory on the root filesystem, mounting tmpfs over it
> > will instantly hide all existing files and UNIX domain sockets in /tmp.
> > 
> > Since there is no cleanup trap to unmount it on exit or failure, does this
> > leave the host system in a degraded state?
> 
> I share Sashiko's mixed feelings here. Do other tests mount /tmp ?
> Seems like something that belongs in the "CI setup", external
> to ksft itself.
Ok, I did some looking around, and while I dont see any tests that mount /tmp, I did notice that mptcp_join.sh and
simult_flows.sh use a $SUDO_USER, presumably to get away from the chown issue. But without getting off the 9p fs
with some sort of scratch area, we still end up with empty pcaps.

> 
> point #2 my vng does have an overlayfs mounted over /tmp so the
> mountpoint check doesn't trigger IDK if this is what you meant 
> or I have a different version.

I did try using --overlay-rwdir, and I think that gives rw to the guest, but it's ro to the host, so we dont get to keep
the pcaps post mortem.

What we can do, if it sounds ok to you, is set up a temp scratch area in the rds_logs folder so it's not mounting over
/tmp, and then run.sh can handle the mount with a cleanup trap addressing Sashiko's concern

Let me know what you think?

Thank you for the reviews!
Allison

Re: [PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Jakub Kicinski]

On Fri, 01 May 2026 22:43:04 -0700 Allison Henderson wrote:
> > point #2 my vng does have an overlayfs mounted over /tmp so the
> > mountpoint check doesn't trigger IDK if this is what you meant 
> > or I have a different version.  
> 
> I did try using --overlay-rwdir, and I think that gives rw to the
> guest, but it's ro to the host, so we dont get to keep the pcaps post
> mortem.
> 
> What we can do, if it sounds ok to you, is set up a temp scratch area
> in the rds_logs folder so it's not mounting over /tmp, and then
> run.sh can handle the mount with a cleanup trap addressing Sashiko's
> concern
> 
> Let me know what you think?

Oh, you need these files for a post mortem analysis? I missed that
point. IDK if there's a well established way to save extra debug info
from the tests. runner has per_test_log_dir but I don't think it's
exposed to tests? Until ksft has such a thing I'd probably go with
an extra env variable to point the test to a specific dir.
Which dir will depend on the CI harness. If var is not set - don't
output the logs or use /tmp and clean up when test exits.

Re: [PATCH net-next v2 5/7] selftests: rds: Fix gcov and pcap collection

[Allison Henderson]

On Sat, 2026-05-02 at 09:38 -0700, Jakub Kicinski wrote:
> On Fri, 01 May 2026 22:43:04 -0700 Allison Henderson wrote:
> > > point #2 my vng does have an overlayfs mounted over /tmp so the
> > > mountpoint check doesn't trigger IDK if this is what you meant 
> > > or I have a different version.  
> > 
> > I did try using --overlay-rwdir, and I think that gives rw to the
> > guest, but it's ro to the host, so we dont get to keep the pcaps post
> > mortem.
> > 
> > What we can do, if it sounds ok to you, is set up a temp scratch area
> > in the rds_logs folder so it's not mounting over /tmp, and then
> > run.sh can handle the mount with a cleanup trap addressing Sashiko's
> > concern
> > 
> > Let me know what you think?
> 
> Oh, you need these files for a post mortem analysis? I missed that
> point. IDK if there's a well established way to save extra debug info
> from the tests. runner has per_test_log_dir but I don't think it's
> exposed to tests? Until ksft has such a thing I'd probably go with
> an extra env variable to point the test to a specific dir.
> Which dir will depend on the CI harness. If var is not set - don't
> output the logs or use /tmp and clean up when test exits.

Ok, I will see if I can do some refactoring and pull the log dir out
of the test scripts and into an environment variable.  I think that
should work if I adopt the $SUDO_USER pattern the other tests use.

Thank you for the reviews!
Allison