Re: bin/47524: make(1) loops in Job_CatchOutput()

[Andreas Gustafsson <gson%gson.org@localhost>]

Hi David,

You wrote:
>  > I believe the "while" loop added by this change is where make is now
>  > looping.
>  
>  I'd guess that fd in non-blocking

Yes - I believe you made it non-blocking as part of job.c revision
1.122, where the commit message says "Common up the code that creates
all the pipes - making them all non-block on the read side in the
process".

> so EAGAIN would just mean that the pipe is empty.

Yes.

>  The purpose of the read() is to empty the pipe, If it is already empty
>  it won't matter.

Agreed.

>  I think you'll find the sigchild handler writes into it

Yes, I know - I wrote that code back in 2002.

>  Most likely two children finishing at once only write 1 character,

I don't think that can trigger the bug.  The read() happens right
after a poll(), and regardless of how many characters are in the pipe,
if the poll indicates the fd is readable, it should be readable.

>  or some path comes in with nready < 0 ...

That seems more likely.  There is actually only one "path in" - nready
is set by the poll on the previous line

   nready = poll(fds + 1 - wantToken, nfds - 1 + wantToken, POLL_MSEC);

Can you please explain the reasoning behind the following change you
made in job.c 1.122 - why is a failing poll treated as if the
childExitJob fd is ready?

-    nready = poll(fds + 1 - wantToken, nfds + 1 - wantToken, POLL_MSEC);
-    if (nready <= 0)
-       return;
+    nready = poll(fds + 1 - wantToken, nfds - 1 + wantToken, POLL_MSEC);

-    if (readyfd(&childExitJob)) {
+    if (nready < 0 || readyfd(&childExitJob)) {

In any case, christos' "fix" of looping five times can't possibly be
correct.  The code worked before the loop was added, and should be
fixed by removing the loop, not by looping some magic number of times.
-- 
Andreas Gustafsson, gson%gson.org@localhost