On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote: > > > >> Stepping back a bit, would this be really needed? Even if the ferror() > >> does not update errno, the original stdio operation that failed > >> would have, no? > > > > Sure, but we have no clue what happened in between. > > Hmm, so we are protecting against somebody who does "errno = 0" > explicitly, because she knows that she's dealt with the error from > stdio earlier? Such a careful person would have called clearerr() > as well, I would guess. I'm not sure I understand what you are saying here. If somebody calls clearerr(), our ferror() handling does not trigger at all, and do not care what is in errno either way. They can reset errno or not when they clearerr(), but it is not relevant. If you are asking about somebody who sets errno to "0" and _doesn't_ call clearerr(), then I don't know what that person is trying to accomplish. Setting errno to "0" is not the right way to clear an error. And they certainly should not be relying on it not to get overwritten before we make it to the final ferror()/fclose(). > So let's assume we only care about the case where some other error > was detected and errno was updated by a system library call. Right. > > I think our emails crossed, but our patches are obviously quite similar. > > My commit message maybe explains a bit more of my thinking. > > Yes, but ;-) > > If we are trying to make sure that the caller would not say "failed > to close tempfile: ERRNO" with an ERRNO that is unrelated to any > stdio opration, I am not sure if the patch improves things. The > caller did not fail to close (most likely we successfully closed > it), and no matter what futzing we do to errno, the message supplied > by such a caller will not be improved. Right. EIO is almost certainly _not_ the error we saw. But I would rather consistently say "I/O error" and have the user scratch their head, look up this thread, and say "ah, it was probably a deferred error", as opposed to the alternative: the user sees something nonsensical like ENOMEM or EBADF. Those are more misleading, and worse, may change from run to run based on what other code runs or fails in between. > If the caller used "noticed an earlier error while closing tempfile: > ERRNO", such a message would describe the situation more correctly, > but then ERRNO that is not about stdio is probably acceptable in the > context of that message (the original ERRNO might be ENOSPC that is > even more specific than EIO, FWIW). So I am not sure if the things > will improve from the status quo. Yes, that's I suggested that xfclose() is probably not a good direction. The _best_ thing we can do is have the caller not report errno at all (or even say "there was an earlier error, I have no idea what errno was"). And xfclose() works in the opposite direction. The only reason I do not think we should do so for close_tempfile() is that the fclose is typically far away from the code that actually calls error(). We'd have to pass the tristate (success, fail, fail-with-errno) state up through the stack (most of the calls indirectly come from commit_lock_file(), I would think). -Peff