[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 21 Jun 2014 17:13:12 +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 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.
Thanks,
Isaac Dunham
> /* Public domain fmtmsg()
> * Written by Isaac Dunham, 2014
> */
> #include <fmtmsg.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <string.h>
> #include <stdlib.h>
> #include <pthread.h>
>
> /*
> * If lstr is the first part of bstr, check that the next char in bstr
> * is either \0 or :
> */
> static int _strcolcmp(const char *lstr, const char *bstr)
> {
> size_t i = 0;
> while (lstr[i] && bstr[i] && (bstr[i] == lstr[i])) i++;
> if ( lstr[i] || (bstr[i] && bstr[i] != ':')) return 1;
> return 0;
> }
>
> 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 (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 (classification & MM_CONSOLE) {
> consolefd = open("/dev/console", O_WRONLY);
> if (consolefd < 0) {
> ret = MM_NOCON;
> } else {
> if (dprintf(consolefd, "%s%s%s%s%s%s%s%s\n",
> label?label:"", label?": ":"",
> severity?errstring:"", text?text:"",
> action?"\nTO FIX: ":"",
> action?action:"", action?" ":"",
> tag?tag:"" )<1)
> ret = MM_NOCON;
> close(consolefd);
> }
> }
>
> if (classification & MM_PRINT) {
> while (cmsg && cmsg[0]) {
> for(i=0; msgs[i]; i++) {
> if (!_strcolcmp(msgs[i], cmsg)) break;
> }
> if (msgs[i] == NULL) {
> //ignore MSGVERB-unrecognized component
> verb =0xFF;
> break;
> } else {
> verb |= (1 << i);
> cmsg = strchr(cmsg, ':');
> if (cmsg) cmsg++;
> }
> }
> if (!verb) verb = 0xFF;
> if (dprintf(2, "%s%s%s%s%s%s%s%s\n",
> (verb&1 && label) ? label : "",
> (verb&1 && label) ? ": " : "",
> (verb&2 && severity) ? errstring : "",
> (verb&4 && text) ? text : "",
> (verb&8 && action) ? "\nTO FIX: " : "",
> (verb&8 && action) ? action : "",
> (verb&8 && action) ? " " : "",
> (verb&16 && tag) ? tag : "" ) < 1)
> ret |= MM_NOMSG;
> }
> if ((ret & (MM_NOCON|MM_NOMSG)) == (MM_NOCON|MM_NOMSG))
> ret = MM_NOTOK;
>
> pthread_setcancelstate(cs, 0);
>
> return ret;
> }
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.
