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

[Jakub Kicinski <kuba@kernel.org>]

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).