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

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

The following reply was made to PR bin/47524; it has been noted by GNATS.

From: christos%zoulas.com@localhost (Christos Zoulas)
To: Andreas Gustafsson <gson%gson.org@localhost>, dsl%NetBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost, gnats-admin%netbsd.org@localhost, 
netbsd-bugs%netbsd.org@localhost
Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()
Date: Mon, 4 Feb 2013 18:06:02 -0500

 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