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
Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()
Date: Tue, 5 Feb 2013 17:05:18 -0500

 On Feb 5, 11:46pm, gson%gson.org@localhost (Andreas Gustafsson) wrote:
 -- Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()
 
 | I think I now know how the infinite loop happened: poll() was
 | interrupted by a signal (probably a SIGCHILD) and returned nready= -1
 | with errno=EINTR.  As a result of your addition of the "nready < 0" in
 | "if (nready < 0 || readyfd(&childExitJob))", a read() was then
 | attempted on the childExitJob pipe even though the select had not
 | indicated it was ready.  Since the pipe was not ready, the read()
 | failed with EAGAIN, which used to be harmlessly ignored until Christos
 | turned it into an infinite loop.
 | 
 | I propose the following change, where the return values from both the
 | poll() and the read() are actually checked in the sense of reporting
 | unexpected errors instead of just looping on EAGAIN and silently
 | ignoring all other errors.  I have successfully tested it with one
 | native -j4 build and one -j12 cross-build from a Linux host.
 | 
 | The other loops Christos added should probably also be replaced
 | by something similar.
 | 
 | Index: job.c
 
 The whole logic is fishy. Why is nready decremented when it is not
 used anymore? Why treat some descriptors specially and not deal
 with them in the general way?
 
 christos
 
 http://www.netbsd.org/~christos/job.c.diff