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

[christos%zoulas.com@localhost (Christos Zoulas)]

On Feb 4,  9:23pm, gson%gson.org@localhost (Andreas Gustafsson) wrote:
-- Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()

| 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.

I agree with that (magic loop count), but poll returning < 0 should not
attempt a read.  the the sentinel can be removed from the loop.

christos