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: gnats-bugs%NetBSD.org@localhost, christos%NetBSD.org@localhost
Subject: Re: bin/47524: make(1) loops in Job_CatchOutput()
Date: Wed, 6 Feb 2013 11:40:22 +0200

 David Laight wrote:
 >  That is a riduculous number of tests, you can't see what the code
 >  is doing any more.
 >  
 >  Indeed, you now error out if the read(childExitJob.inPipe, &token, 1)
 >  manages to fail to return some data. I wouldn't like to guarantee
 >  that all systems don't have some point where such a read can get
 >  interrupted (even though non-blocking) before committing to copy
 >  a byte to userspace.
 
 That is not supposed to happen since bmake_signal() sets SA_RESTART.
 With the changes in my patch, at least we will get a diagnostic if
 SA_RESTART turns out to be ineffective on some platform, and can then
 add the necessary retry-on-EINTR loops around any *other* read/write
 calls where this situation actually matters.
 
 >  The 'token = 0' assignent is enough that it doesn't matter whether
 >  the read fails of not.
 >
 >  Apart from issues with poll() actually returning a read error - so
 >  it doesn't block next time around either, it really doesn't matter
 >  why the code woke up.
 >  
 >  I might even have decided that if the poll() call was interrupted
 >  we might as well act as it the childExitJob pipe had a character in it.
 
 Or in other words, the code was working before revision 1.165; I concur.
 Reverting job.c to version 1.164 would be acceptable, preferably with
 some comments added to explain the above reasoning.
 
 If the purpose of 1.165 was specifically to suppress a compiler
 warning on Linux, surely there must be some way of doing that without
 changing the generated object code.  On the other hand, if what we
 want is to change the behavior of the code to actually check the
 return value instead of just suppressing a warning, then I think the
 complexity of my "ridiculous number of tests" is pretty much
 unavoidable.
 -- 
 Andreas Gustafsson, gson%gson.org@localhost