wait code is fishy

Good day,

I don't report a bug here, but some rather contrived code.

Looking at waitproc() first. Hmm, what that
"break" doing? Aha:

                 do
                         err = waitpid(-1, status, flags);
                 while (err < 0 && errno == EINTR);

+               /* Return if error (for example, ECHILD); or if pid found;
+                * or if "block" is DOWAIT_NONBLOCK (=0), in this case return -1.
+                */
                 if (err || (err = -!block))
                         break;

                 sigblockall(&oldmask);
                 while (!gotsigchld && !pending_sig)
                         sigsuspend(&oldmask);
                 sigclearmask();
	} while (gotsigchld);
         return err;

(Maybe rewrite it to less cryptic code?
if (err)
         return err;
if (block == DOWAIT_NONBLOCK)
         return -1;
)

So, looks like waitproc() never returns 0. It's always -1 or pid > 0?

NOPE. It's non-obvious, but if we exit the loop if got a !SIGCHLD signal,
err will be 0.

(Which would be more obvious if it would say "return 0" instead of "return err").

It has only one caller, waitone().
Which always returns waitproc() return value as-is.
waitone(), in turn, has only one caller, dowait(). Which does this:

        if (block == DOWAIT_NONBLOCK && !gotchld)
                return 1;
        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;
+

(If you modify this code, please move waitproc() definition up,
so that they are in more customary sequence in the source:

waitproc() {...}
waitone() {...}
dowait() {...}

instead of current

waitone() {...}
dowait() {...}
waitproc() {...}

And maybe fix the wrong function name in this TRACE:
static int waitone(int block, struct job *job)
{
...
         TRACE(("dowait(%d) called\n", block));

)