Re: wait code is fishy

On 8/10/25 23:30, Denys Vlasenko wrote:

>         rpid = 1;
>         do {
>                  pid = waitone(block, jp);
>                  rpid &= !!pid;
>                  block &= ~DOWAIT_WAITCMD_ALL;
>                  if (!pid || (jp && jp->state != JOBRUNNING))
>                          block = DOWAIT_NONBLOCK;
>          } while (pid >= 0);
>          return rpid;
> }
> 
> So the dowait() return value is 0/1 "got signal" indicator:
> 0 is "we got a !SIGCHLD signal", 1 is "we didn't".
> 
> Not clear why we bother. We already have that indicator,
> it's "pending_sig" global variable.
> 
> So the calculation of "rpid" is redundant.
> The only caller which checks dowait()'s return value is waitcmd()
> and it can just look at pending_sig.
> 
> The reason for "pid == 0" retrying NONBLOCK wait for more processes
> is unclear: if we got here, we already did wait for processes
> just now, got none, waited for signals and got a not-SIGCHLD one,
> so there should not be more processes to wait for.
> A comment explaining it would be nice?
> 
> -                if (!pid || (jp && jp->state != JOBRUNNING))
> -                        block = DOWAIT_NONBLOCK;
> +                /* If got signal, do one more nonblocking wait (why?) */
> +                if (pid == 0)
> +                        block = DOWAIT_NONBLOCK;
> +                /* if waiting for a specific job, continue blocking wait unless it stopped running: */
> +                if (jp && jp->state != JOBRUNNING)
> +                        block = DOWAIT_NONBLOCK;

I thought about it a bit more. The entire loop in dowait()
is for these purposes:

- if we waited for a process in blocking mode and got one (got pid > 0),
but the job we were waiting for is still running
(jp->state == JOBRUNNING), we have to wait in blocking mode again,
until it's not JOBRUNNING.
The user is waitforjob(jp) -> dowait(DOWAIT_BLOCK, jp)

- if we waited for a process in non-blocking mode and got one (got pid > 0),
we should see whether more processes have terminated,
we have to wait in non-blocking mode again.
The users are
  showjobs() -> dowait(DOWAIT_NONBLOCK)
  waitforjob(NULL) -> dowait(DOWAIT_NONBLOCK)

Notice that in both cases, pid > 0. If it is not,
the loop does not seem to make sense: we did not see any processes
changing state, why would we want to check again immediately?

waitcmd() does its own looping while it sees at least one JOBRUNNING
job, so it does not need dowait() to loop - only to do
one nonblocking waiting, and if there are still processes running,
then sleep waiting for signals.

Should we change?

-	} while (pid >= 0)
+	} while (pid > 0); /* one process seen changing state, look for more */