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

[Allison Henderson <achender@kernel.org>]

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