* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Check dup2() results in syslogger
> > Consistently check the dup2() call results throughout syslogger.c.
> > It's pretty unlikely that they'll error out, but if they do,
> > ereport(FATAL) instead of blissfully continuing on.
>
> Meh. Have you actually tested that an ereport(FATAL) is capable of doing
> anything sane right there, with so much syslogger initialization left to
> do, and no working stderr?
Given that we were already doing so later on in the same function, it
struck me as appropriate. I agree that the case is a bit interesting to
consider.
> Please note also that the comment just above
> this implies that we are deliberately ignoring any failures here, so I
> think FATAL was probably the wrong thing in any case.
We were explicitly ignoring the errors from the close(), I don't believe
those comments were intended to apply to the dup2() calls as well, based
on my reading of the commit history.
> > Spotted by the Coverity scanner.
>
> I fear this is mere Coverity-appeasement that has broken code that used
> to work.
Those dup2() calls look likely to only fail in a case of running out of
file handles or similar, which struck me as a good reason to
ereport(FATAL), similar to the setsid() failure which is checked for
below. I could have just done (void) dup2() instead to make it clear
that we were intentionally ignoring the results to please Coverity, and
probably should have adjusted the close() calls that way.
The last adjustment to this code was actually to avoid the dup2() calls
causing failures *regardless* of our ignoring the result code due to a
Windows' feature in CRT called Paramter Validation (per Heikki's commit
message). Heikki, any thoughts on the right answer here? I admit that
I didn't go to the effort of forcing the dup2() calls to fail to see
what happens, though I did play around w/ killing off the syslogger
forcibly and making sure it came back, which appeared to work fine.
Thanks,
Stephen