[lxc-devel] [PATCH v3 4/4] uniformly nullify std fds

[Serge Hallyn]

Quoting Robert Vogelgesang (vogel at users.sourceforge.net):
> Hi,
> 
> On Wed, Jun 10, 2015 at 12:04:24PM +0000, Serge Hallyn wrote:
> > Quoting Robert Vogelgesang (vogel at users.sourceforge.net):
> [...]
> > > > +		 * Here, we're taking advantage of C's short circuiting of
> > > > +		 * conditions: we should only fail if quiet is set and
> > > > +		 * null_stdfds fails.
> > > > +		 */
> > > > +		if (quiet && null_stdfds() < 0) {
> > > > +			exit(1);
> > > 
> > > My concern is not about someone not understanding short circuiting.
> > > There should be a warning against reversing the order of the two parts.
> > > 
> > > Short circuiting is rather common, but that "quiet" means to close
> > > fds, is unusual and not obvious.
> > > 
> > > If someone would change that to
> > > 
> > > > +		if (null_stdfds() < 0 && quiet) {
> > 
> > Then we would hopefully reject the patch.
> 
> Once bitten, twice shy:  In a different project (I won't name it here)
> I did trust maintainers that they won't do stupid things, with the
> consequence of lost data and a major outage, just because the
> maintainers did a "harmless" change to silence SOME messages, but
> in fact they killed logging of ALL messages of subprocesses.  All
> early warnings about upcoming problems were lost.  Since then,
> I'm checking each update of this software if they did it again...
> 
> A single line comment would make the intention obvious to any reader:
> 
> 	/* implement "quiet" by redirecting fds 0, 1, 2 to /dev/null */
> 	if (quiet && null_stdfds() < 0) {
> 
> Nobody with at least half a brain would then try to "clean up things"
> and reverse the order.

I think making it

	if (quiet)
		if (null_stdfds() < 0)
			exit(1);

would be more useful for that than a comment.  Or re-naming quiet
to need_null_stdfds.