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

[Wen Yang <wen.yang@linux.dev>]

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:
>> On 6/25/26 20:14, Gabriele Monaco wrote:
>>> +	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