[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140621143902.GF179@brightrain.aerifal.cx>
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.
