Re: [PATCH v3 04/17] tools/rv: Add selftests

On 6/29/26 14:51, Gabriele Monaco wrote:

Please cut down the context a bit more next time, it makes it much
easier to find your review.

On Mon, 2026-06-29 at 01:10 +0800, Wen Yang wrote:

quoted

On 6/25/26 20:14, Gabriele Monaco wrote:

quoted

+	eval "$TIMEOUT" "$command" &> check_output.$$ &
+	bgpid=$!
+	pid=$(pgrep -f "${command%%[|;&>]*}" | tail -n1)

The pgrep runs may immediately after the background fork, before the
child process has had time to exec.

Yeah I'm aware of this but kind of ignored it for now and never seen it
making troubles in practice..

I could add some delay waiting for the task like:

   while [ -z "$pid" ]; do
     sleep .5
     pid=$(pgrep -f "${command%%[|;&>]*}" | tail -n1)
   done

With probably a maximum of some N retrials in case the task never
started or we messed up the pattern.
That may still race in case the command exits before we pgrep it, but in
practice that shouldn't be a problem in our tests.

Any better idea? We cannot really rely on the shell's $! because command
is using a combination of eval+timer and we'd get the wrong pid.

- Since $bgpid is the timeout process, its direct child is exactly the
    command we want. Using pgrep -P $bgpid avoids the fragile pattern
    matching of pgrep -f and won't accidentally match unrelated
    processes with a similar command string, eg:

      for i in $(seq 10); do
          pid=$(pgrep -P "$bgpid" | head -1)
          [ -n "$pid" ] && break
          sleep 0.5
      done

    Note: a bounded retry loop may be necessary; without an upper limit 
the loop hangs indefinitely if the command fails to exec.

- For the verbose test specifically ("my pid is $pid"), the pid already
    appears in rv's own output. An alternative is to match it with a
    numeric pattern instead:

      "my pid is [0-9]\+"

    This sidesteps the race entirely for that test case.

--
Best wishes,
Wen