Re: [PATCH v3] Implement fmtmsg

[Rich Felker <dalias@libc.org>]

On Sat, Jun 21, 2014 at 05:13:12PM +0000, Isaac Dunham wrote:
> On Sat, Jun 21, 2014 at 12:18:14PM -0400, Rich Felker wrote:
> > On Sat, Jun 21, 2014 at 03:56:38PM +0000, Isaac Dunham wrote:
> > > > This is the wrong check for consoldfd being valid; 0 is a valid fd,
> > > > whereas consolefd==-1 passes the test.
> > > > 
> > > > So I think consolefd should be initialized to -1 not 0 and the check
> > > > here should be >=0. Or we could just reorder the severity lines above
> > > > the opening of the console so that the opening and use of consolefd
> > > > aren't separated like this.
> > > 
> > > As far as I can tell:
> > > -severity really should go above this (grouping related subjects)
> > 
> > OK we agree on this.
> > 
> > > -initializing to -1 will expose a bit of brokenness in here:
> > > 	RET=MM_NOCON;
> > > would be executed whenever we did not successfully open the console,
> > > whether or not we tried to open it.
> > > It would be better to
> > >  * leave the initialization alone, or move the error block into the same
> > > if() {} block as the call to open().
> > 
> > The latter is better. Using 0 to mean "none" for a file descriptor
> > variable is bad practice in general.
> > 
> > BTW while making these changes I noticed that you never close
> > consolefd. I added that in the same place. Also made a few other
> > improvements. See if the attached file looks better.
> 
> It looks much better.
> At this point, consolefd need not be initialized, and I think that will be
> the last change.

Committed. If you see anything else that looks like it needs changing,
just let me know.

Rich