Re: [PATCH] selftests/net: fix kill() argument order in fin_ack_lat

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=1
---
--- Patch [1]: [PATCH] selftests/net: fix kill() argument order in fin_ack_lat ---
commit c8d7888fff6c43f1844cd283115315b6ce892cad
Author: Qingshuang Fu <[email protected]>

selftests/net: fix kill() argument order in fin_ack_lat

The patch swaps the arguments of the kill() call in sig_handler() so the
server forwards SIGTERM to the client instead of passing a pid as the
signal number, and adds a child_pid > 0 guard so the client (which
inherits the handler and sees child_pid == 0) does not signal the whole
process group.

> diff --git a/tools/testing/selftests/net/fin_ack_lat.c b/tools/testing/selftests/net/fin_ack_lat.c
> index 70187494b57af..f985e01354b44 100644
> --- a/tools/testing/selftests/net/fin_ack_lat.c
> +++ b/tools/testing/selftests/net/fin_ack_lat.c
> @@ -103,7 +103,8 @@ static void server(int sock, struct sockaddr_in address)
>  
>  static void sig_handler(int signum)
>  {
> -	kill(SIGTERM, child_pid);
> +	if (child_pid > 0)
> +		kill(child_pid, SIGTERM);
>  	exit(0);
>  }

[Severity: Low]
[Finding: main-2-1]
[Sources: opus-5]
Now that the forwarding actually works, can the wrapper script's cleanup
path start failing?  Both processes carry the comm name fin_ack_lat
because main() forks:

  child_pid = fork();
  if (!child_pid)
          client(ntohs(laddr.sin_port));
  else
          server(sock, laddr);

so pidof returns two pids in fin_ack_lat.sh:

  set -e
  ...
  cleanup() {
          kill $(pidof fin_ack_lat)
          rm -f $tmpfile
  }

  trap cleanup EXIT

If bash signals the server pid first, the server now runs sig_handler(),
forwards SIGTERM to the client and exits.  The client then exits, is
reparented to init and reaped.  If that completes before bash issues the
second kill(2), the builtin gets ESRCH and returns non-zero.

With set -e in effect, wouldn't that abort the EXIT trap, so the script
exits non-zero even when do_test() saw zero spikes, and rm -f $tmpfile is
skipped, leaving /tmp/fin_ack_latency.*.log behind?

Before this change the forwarding call always failed with EINVAL, so the
client was still alive when bash reached its pid and this window did not
exist.  Would it make sense to make the wrapper tolerant, for example
kill $(pidof fin_ack_lat) 2>/dev/null || true, or to kill only the server
pid?

[Severity: Medium]
[Finding: gpt-5-6-sol-3-4]
[Sources: gpt-5-6-sol, opus-5]
This isn't a bug introduced by this patch, but the new child_pid > 0 guard
is the only place the fork() failure sentinel is considered, and it handles
it by doing nothing.  main() never checks fork() for failure:

  child_pid = fork();
  if (!child_pid)
          client(ntohs(laddr.sin_port));
  else
          server(sock, laddr);

On failure child_pid is -1, the !child_pid test is false, and the process
falls into server()'s infinite accept() loop with no client ever
connecting, so nothing is printed.

fin_ack_lat.sh's do_test() only counts lines and treats an empty log as
success:

  NR_SPIKES=$(wc -l $tmpfile | awk '{print $1}')
  if [ $NR_SPIKES -gt 0 ]

so a fork() failure (EAGAIN/ENOMEM, RLIMIT_NPROC) is scored as a passing
test.  Since this file already uses error() for every other syscall, would
adding if (child_pid < 0) error(-1, errno, "fork"); be appropriate here?
-- 
pw-bot: changes-requested