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

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

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

From: Andreas Gustafsson <gson%gson.org@localhost>
To: dsl%NetBSD.org@localhost
Cc: christos%NetBSD.org@localhost,
    gnats-bugs%NetBSD.org@localhost,
    gnats-admin%netbsd.org@localhost,
    netbsd-bugs%netbsd.org@localhost,
    gson%gson.org@localhost (Andreas Gustafsson)
Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()
Date: Mon, 4 Feb 2013 21:23:02 +0200

 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