Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 21 Jun 2014 10:39:02 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v3] Implement fmtmsg

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.

> Also, I thought there was an issue with my code in _strcolcmp() for a moment,
> since 
> _strcolcmp("label:sevarity","label:severity") == 1;
> But that's not an issue, since 
> one side is always null-terminated at the end of the current component
> (we compare to msgs[i]).

I agree that doesn't matter here.

> > int fmtmsg(long classification, const char *label, int severity,
> > 	       	const char *text, const char *action, const char *tag)
> > {
> > 	int ret = 0, i, consolefd = 0, verb = 0;
> > 	char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB");
> > 	char *const msgs[] = {
> > 		"label", "severity", "text", "action", "tag", NULL
> > 	};
> > 	int cs;
> > 
> > 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> > 
> > 	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.

Rich

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.