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

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.

quoted

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

[ ... ]

quoted

@@ -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

quoted

+        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