[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 21 Jun 2014 15:56:38 +0000
From: Isaac Dunham <ibid.ag@...il.com>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v3] Implement fmtmsg
On Sat, Jun 21, 2014 at 10:39:02AM -0400, Rich Felker wrote:
> On Sat, Jun 21, 2014 at 01:41:02PM +0000, Isaac Dunham wrote:
> > On Sat, Jun 21, 2014 at 12:46:26AM -0400, Rich Felker wrote:
> > > On Fri, Jun 20, 2014 at 07:32:01AM -0700, Isaac Dunham wrote:
> > > > Here's a third revision of fmtmsg.
> > >
> > > Thanks, it looks good. I'm doing a bit of cleanup before I commit --
> > > mostly things like mismatches spaces/tabs in indention/alignment,
> > > trailing spaces, etc. Also taking out the useless if around the while
> > > loop that contains the if condition as part of the while condition,
> > > debloating the header a bit (all the hex constants), adding protection
> > > against pthread cancellation, and fixing one error in the header:
> > > 0xffffffff is not a valid value for MM_NOTOK since it doesn't fit in
> > > int.
> > >
> > > I'm a bit tired now so rather than just commit and possibly have
> > > stupid mistakes, I'm sending my draft revisions here (attached). If
> > > you get a chance please take a look and see if they look ok.
> >
> > All your changes look good, but I wrote one line that's unneeded.
> > (see below).
>
> Thanks.
>
> > > if (classification & MM_CONSOLE)
> > > consolefd = open("/dev/console", O_WRONLY);
> > > if (consolefd < 0) {
> > > classification &= ~MM_CONSOLE;
> >
> > I don't think this is needed.
> > After all, MM_CONSOLE is not checked after this.
>
> Indeed. But your pointing this out to me revealed another error below:
>
> > > ret = MM_NOCON;
> > > consolefd = 0;
> > > }
> > > if (severity == MM_HALT) errstring = "HALT: ";
> > > else if (severity == MM_ERROR) errstring = "ERROR: ";
> > > else if (severity == MM_WARNING) errstring = "WARNING: ";
> > > else if (severity == MM_INFO) errstring = "INFO: ";
> > > if (consolefd) {
>
> 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)
-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().
* keep "classification &= ~MM_CONSOLE;"
* remove
- "consolefd = 0;"
* change the test as follows:
- if (consolefd) {
+ if (classification &= MM_CONSOLE) {
>
> Rich
Thanks,
Isaac Dunham
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
