* [PATCH] Implement fmtmsg @ 2014-04-25 5:06 Isaac Dunham 2014-04-25 14:49 ` Rich Felker 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-04-25 5:06 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 321 bytes --] Having noticed commit b427d847, I've implemented fmtmsg(). Here's the patch. I'm also attaching a small program I used for testing fmtmsg. This program is not the standard usage and is not really proper (due to a lack of sanity checking); but it allows me to cover the various edge cases fairly well. HTH, Isaac Dunham [-- Attachment #2: fmtmsg.diff --] [-- Type: text/plain, Size: 4050 bytes --] diff --git a/include/fmtmsg.h b/include/fmtmsg.h new file mode 100644 index 0000000..bdc4a52 --- /dev/null +++ b/include/fmtmsg.h @@ -0,0 +1,53 @@ +#ifndef _FMTMSG_H +#define _FMTMSG_H + +/* Major: source of problem */ +#define MM_HARD 0x0001 +#define MM_SOFT 0x0002 +#define MM_FIRM 0x0004 + +/* Source subclassification: code encountering the problem */ +#define MM_APPL 0x0008 +#define MM_UTIL 0x0010 +#define MM_OPSYS 0x0020 + +/* Display subclassification */ +#define MM_PRINT 0x0100 +#define MM_CONSOLE 0x0200 + +/* Can we recover? */ +#define MM_RECOVER 0x0040 +#define MM_NRECOV 0x0080 + +#define MM_NULLMC 0L + +/* Severity */ +#define MM_HALT 0x1 +#define MM_ERROR 0x2 +#define MM_WARNING 0x3 +#define MM_INFO 0x4 +#define MM_NOSEV 0x0 + +/* Return */ +#define MM_OK 0x00000000 +#define MM_NOTOK 0xffffffff +#define MM_NOMSG 0x00000001 +#define MM_NOCON 0x00000004 + +#define MM_NULLLBL (char*)0 +#define MM_NULLTXT (char*)0 +#define MM_NULLACT (char*)0 +#define MM_NULLTAG (char*)0 +#define MM_NULLSEV 0 + + +#ifdef __cplusplus +extern "C" { +#endif +int fmtmsg(long, const char *, int, const char *, const char *, const char *); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c new file mode 100644 index 0000000..1cc6fdd --- /dev/null +++ b/src/misc/fmtmsg.c @@ -0,0 +1,113 @@ +/* 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> + +static char *_msgtok[] = { +"label", "severity", "text", "action", "tag", NULL +}; + +/* + * 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; +} + +static int _validenv(char *evar) +{ + size_t i; + + while (evar && evar[0]) { + for(i=0; _msgtok[i]; i++) { + if (!_strcolcmp(_msgtok[i], evar)) break; + } + if (_msgtok[i] == NULL) return 0; + evar = strchr(evar, ':'); + if (evar) evar++; + } + return 1; +} + +static int _writemsg(const char *msg) +{ + int buflen = strlen(msg); + if (write(2, msg, buflen) < buflen) return MM_NOMSG; + return 0; +} + +int fmtmsg(long classification, const char *label, int severity, + const char *text, const char *action, const char *tag) +{ + int ret = 0, i = 0, consolefd = 0; + char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB"); + + if (classification & MM_CONSOLE) + consolefd = open("/dev/console", O_WRONLY); + if (consolefd < 0) { + classification &= ~MM_CONSOLE; + 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) { + if (dprintf(consolefd, "%s: %s: %s\nTO FIX: %s %s\n", + label, errstring, text, action, tag)<15) + ret = MM_NOCON; + } + + if (cmsg && !_validenv(cmsg)) cmsg = (char *)0; + while ((classification & MM_PRINT) && (i < 6)) { + if (cmsg) i = 0; + while (cmsg && _msgtok[i]) { + if (!_strcolcmp(_msgtok[i], cmsg)) break; + i++; + } + i++; + switch (i) { + case 1: + ret |= _writemsg(label); + ret |= _writemsg(": "); + break; + case 2: + if (errstring){ + ret |= _writemsg(errstring); + ret |= _writemsg(": "); + } + break; + case 3: + ret |= _writemsg(text); + break; + case 4: + ret |= _writemsg("\nTO FIX: "); + ret |= _writemsg(action); + break; + case 5: + ret |= _writemsg(" "); + ret |= _writemsg(tag); + break; + } + if (cmsg) { + cmsg = strchr(cmsg, ':'); + if (cmsg) cmsg++; + if (!cmsg || !cmsg[0]) i = 6; + } + } + if (classification & MM_PRINT) _writemsg("\n"); + if ((ret & (MM_NOCON|MM_NOMSG) ) == (MM_NOCON|MM_NOMSG)) + ret = MM_NOTOK; + return ret; +} [-- Attachment #3: testfmt.c --] [-- Type: text/plain, Size: 395 bytes --] #include <fmtmsg.h> /* fmtmsg test program, by Isaac Dunham * Released into the public domain with no guarantees whatsoever. * * Usage: * testfmt LABEL N TEXT ACTION TAG */ int main (int argc, char **argv) { int sev = MM_NOSEV; //close(2); if (argc > 1) sev = atoi(argv[2]); fmtmsg(MM_SOFT|MM_APPL|MM_PRINT|MM_CONSOLE|MM_RECOVER, argv[1], sev, argv[3], argv[4], argv[5]); } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Implement fmtmsg 2014-04-25 5:06 [PATCH] Implement fmtmsg Isaac Dunham @ 2014-04-25 14:49 ` Rich Felker 2014-04-25 16:10 ` [PATCH v2] " Isaac Dunham 0 siblings, 1 reply; 14+ messages in thread From: Rich Felker @ 2014-04-25 14:49 UTC (permalink / raw) To: musl On Thu, Apr 24, 2014 at 10:06:01PM -0700, Isaac Dunham wrote: > Having noticed commit b427d847, I've implemented fmtmsg(). > Here's the patch. > > I'm also attaching a small program I used for testing fmtmsg. > This program is not the standard usage and is not really proper (due to > a lack of sanity checking); but it allows me to cover the various edge > cases fairly well. A couple notes inline: > diff --git a/include/fmtmsg.h b/include/fmtmsg.h > new file mode 100644 > index 0000000..bdc4a52 > --- /dev/null > +++ b/include/fmtmsg.h > @@ -0,0 +1,53 @@ > +#ifndef _FMTMSG_H > +#define _FMTMSG_H > + > +/* Major: source of problem */ In general musl doesn't have comments in the public headers. > +#ifdef __cplusplus > +extern "C" { > +#endif For files that need extern "C", it should generally go around the whole file rather than buried next to the declarations. This is less error-prone if other things where it matters are added later (probably not an issue for fmtmsg, but that's the general idea). > diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c > new file mode 100644 > index 0000000..1cc6fdd > --- /dev/null > +++ b/src/misc/fmtmsg.c > @@ -0,0 +1,113 @@ > +/* 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> > + > +static char *_msgtok[] = { > +"label", "severity", "text", "action", "tag", NULL > +}; This should be const, but even that will leave relocations (and thus occupy non-const memory) for the shared library, so it would be preferable to use an approach that avoids this. > +int fmtmsg(long classification, const char *label, int severity, > + const char *text, const char *action, const char *tag) > +{ > + int ret = 0, i = 0, consolefd = 0; > + char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB"); > + > + if (classification & MM_CONSOLE) > + consolefd = open("/dev/console", O_WRONLY); IIRC in one other place that had console output optional (syslog?), musl causes it to always-fail, but maybe we should revisit this. It might be nice to be consistent. This could at least use some discussion. > + if (consolefd < 0) { > + classification &= ~MM_CONSOLE; > + 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) { > + if (dprintf(consolefd, "%s: %s: %s\nTO FIX: %s %s\n", > + label, errstring, text, action, tag)<15) > + ret = MM_NOCON; > + } In any case, dprintf is fine. musl's version is async-signal-safe and intended to always be so. Rich ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] Implement fmtmsg 2014-04-25 14:49 ` Rich Felker @ 2014-04-25 16:10 ` Isaac Dunham 2014-04-25 16:12 ` Isaac Dunham 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-04-25 16:10 UTC (permalink / raw) To: musl Thanks for the review, Rich. Here's a second version, with the following changes: fmtmsg.h: remove comments, wrap whole header in extern "C" fmtmsg.c: inline _msgtok[] (as msgs[]) and _validenv() fmtmsg still prints to the console if requested and permitted, as per POSIX. This detail will probably only be relevant for daemons. HTH, Isaaac Dunham ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Implement fmtmsg 2014-04-25 16:10 ` [PATCH v2] " Isaac Dunham @ 2014-04-25 16:12 ` Isaac Dunham 2014-06-20 3:23 ` Rich Felker 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-04-25 16:12 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 464 bytes --] Forgot to attach the patch. On Fri, Apr 25, 2014 at 09:10:19AM -0700, Isaac Dunham wrote: > Thanks for the review, Rich. > > Here's a second version, with the following changes: > fmtmsg.h: remove comments, wrap whole header in extern "C" > fmtmsg.c: inline _msgtok[] (as msgs[]) and _validenv() > > fmtmsg still prints to the console if requested and permitted, as per POSIX. > This detail will probably only be relevant for daemons. > > HTH, > Isaac Dunham [-- Attachment #2: fmtmsg.diff --] [-- Type: text/plain, Size: 3784 bytes --] diff --git a/include/fmtmsg.h b/include/fmtmsg.h new file mode 100644 index 0000000..4bc34d3 --- /dev/null +++ b/include/fmtmsg.h @@ -0,0 +1,47 @@ +#ifndef _FMTMSG_H +#define _FMTMSG_H + +#ifdef __cplusplus +extern "C" { +#endif + +#define MM_HARD 0x0001 +#define MM_SOFT 0x0002 +#define MM_FIRM 0x0004 + +#define MM_APPL 0x0008 +#define MM_UTIL 0x0010 +#define MM_OPSYS 0x0020 + +#define MM_PRINT 0x0100 +#define MM_CONSOLE 0x0200 + +#define MM_RECOVER 0x0040 +#define MM_NRECOV 0x0080 + +#define MM_NULLMC 0L + +#define MM_HALT 0x1 +#define MM_ERROR 0x2 +#define MM_WARNING 0x3 +#define MM_INFO 0x4 +#define MM_NOSEV 0x0 + +#define MM_OK 0x00000000 +#define MM_NOTOK 0xffffffff +#define MM_NOMSG 0x00000001 +#define MM_NOCON 0x00000004 + +#define MM_NULLLBL (char*)0 +#define MM_NULLTXT (char*)0 +#define MM_NULLACT (char*)0 +#define MM_NULLTAG (char*)0 +#define MM_NULLSEV 0 + +int fmtmsg(long, const char *, int, const char *, const char *, const char *); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c new file mode 100644 index 0000000..be75142 --- /dev/null +++ b/src/misc/fmtmsg.c @@ -0,0 +1,108 @@ +/* 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> + + +/* + * 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; +} + +static int _writemsg(const char *msg) +{ + int buflen = strlen(msg); + if (write(2, msg, buflen) < buflen) return MM_NOMSG; + 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; + char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB"); + char *const msgs[] = { + "label", "severity", "text", "action", "tag", NULL + }; + + if (classification & MM_CONSOLE) + consolefd = open("/dev/console", O_WRONLY); + if (consolefd < 0) { + classification &= ~MM_CONSOLE; + 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) { + if (dprintf(consolefd, "%s: %s: %s\nTO FIX: %s %s\n", + label, errstring, text, action, tag)<15) + ret = MM_NOCON; + } + + if (cmsg) { + char *evar = cmsg; + while (evar && evar[0]) { + for(i=0; msgs[i]; i++) { + if (!_strcolcmp(msgs[i], evar)) break; + } + if (msgs[i] == NULL) cmsg = 0; + evar = strchr(evar, ':'); + if (evar) evar++; + } + } + for (i=0; (classification & MM_PRINT) && (i < 6);) { + if (cmsg) i = 0; + while (cmsg && msgs[i]) { + if (!_strcolcmp(msgs[i], cmsg)) break; + i++; + } + i++; + switch (i) { + case 1: + ret |= _writemsg(label); + ret |= _writemsg(": "); + break; + case 2: + if (errstring){ + ret |= _writemsg(errstring); + ret |= _writemsg(": "); + } + break; + case 3: + ret |= _writemsg(text); + break; + case 4: + ret |= _writemsg("\nTO FIX: "); + ret |= _writemsg(action); + break; + case 5: + ret |= _writemsg(" "); + ret |= _writemsg(tag); + break; + } + if (cmsg) { + cmsg = strchr(cmsg, ':'); + if (cmsg) cmsg++; + if (!cmsg || !cmsg[0]) i = 6; + } + } + if (classification & MM_PRINT) _writemsg("\n"); + if ((ret & (MM_NOCON|MM_NOMSG) ) == (MM_NOCON|MM_NOMSG)) + ret = MM_NOTOK; + return ret; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH v2] Implement fmtmsg 2014-04-25 16:12 ` Isaac Dunham @ 2014-06-20 3:23 ` Rich Felker 2014-06-20 4:46 ` Isaac Dunham 2014-06-20 14:32 ` [PATCH v3] " Isaac Dunham 0 siblings, 2 replies; 14+ messages in thread From: Rich Felker @ 2014-06-20 3:23 UTC (permalink / raw) To: musl Sorry it's taken me so long to getting around to reviewing this! I finally have meaningful comments for you. On Fri, Apr 25, 2014 at 09:12:33AM -0700, Isaac Dunham wrote: > Forgot to attach the patch. > > On Fri, Apr 25, 2014 at 09:10:19AM -0700, Isaac Dunham wrote: > > Thanks for the review, Rich. > > > > Here's a second version, with the following changes: > > fmtmsg.h: remove comments, wrap whole header in extern "C" > > fmtmsg.c: inline _msgtok[] (as msgs[]) and _validenv() > > > > fmtmsg still prints to the console if requested and permitted, as per POSIX. > > This detail will probably only be relevant for daemons. > > > > HTH, > > Isaac Dunham > diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c > new file mode 100644 > index 0000000..be75142 > --- /dev/null > +++ b/src/misc/fmtmsg.c > @@ -0,0 +1,108 @@ > +/* 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> > + > + > +/* > + * 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; > +} > + > +static int _writemsg(const char *msg) > +{ > + int buflen = strlen(msg); > + if (write(2, msg, buflen) < buflen) return MM_NOMSG; > + 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; > + char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB"); > + char *const msgs[] = { > + "label", "severity", "text", "action", "tag", NULL > + }; > + > + if (classification & MM_CONSOLE) > + consolefd = open("/dev/console", O_WRONLY); > + if (consolefd < 0) { > + classification &= ~MM_CONSOLE; > + 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) { > + if (dprintf(consolefd, "%s: %s: %s\nTO FIX: %s %s\n", > + label, errstring, text, action, tag)<15) > + ret = MM_NOCON; > + } > + > + if (cmsg) { > + char *evar = cmsg; > + while (evar && evar[0]) { > + for(i=0; msgs[i]; i++) { > + if (!_strcolcmp(msgs[i], evar)) break; > + } > + if (msgs[i] == NULL) cmsg = 0; > + evar = strchr(evar, ':'); > + if (evar) evar++; > + } > + } > + for (i=0; (classification & MM_PRINT) && (i < 6);) { > + if (cmsg) i = 0; > + while (cmsg && msgs[i]) { > + if (!_strcolcmp(msgs[i], cmsg)) break; > + i++; > + } It looks like you've gone to some trouble to allow MSGVERB to reorder the output, but per the standard I don't think it's supposed to do this. The wording is not very clear but it simply says (1) "If MSGVERB contains a keyword for a component and the component's value is not the component's null value, fmtmsg() shall include that component in the message when writing the message to standard error" and (2) "The keywords may appear in any order". My reading of "include" is that it does not impose an ordering on the output. Unfortunately the examples don't clarify this. Can you check what existing implementation do? > + i++; > + switch (i) { > + case 1: > + ret |= _writemsg(label); > + ret |= _writemsg(": "); > + break; > + case 2: > + if (errstring){ > + ret |= _writemsg(errstring); > + ret |= _writemsg(": "); > + } > + break; > + case 3: > + ret |= _writemsg(text); > + break; > + case 4: > + ret |= _writemsg("\nTO FIX: "); > + ret |= _writemsg(action); > + break; > + case 5: > + ret |= _writemsg(" "); > + ret |= _writemsg(tag); > + break; > + } > + if (cmsg) { > + cmsg = strchr(cmsg, ':'); > + if (cmsg) cmsg++; > + if (!cmsg || !cmsg[0]) i = 6; > + } All these individual small writes are pretty costly in syscall overhead. It would be really nice if a single dprintf could do all of them together (it buffers), which is definitely possible if MSGVERB doesn't need to reorder them. The strategy for getting dprintf to do arbitrary subsets would be using %.*s with a precision argument of either -1 or 0 to control whether the field is printed or not. I think this would also debloat the code a bit. One other thing I noticed: "If MSGVERB is not defined, if its value is the null string, if its value is not of the correct format, or if it contains keywords other than the valid ones listed above, fmtmsg() shall select all components." I think it's necessary to validate the MSGVERB string to comply with this requirement. Rich ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH v2] Implement fmtmsg 2014-06-20 3:23 ` Rich Felker @ 2014-06-20 4:46 ` Isaac Dunham 2014-06-20 14:32 ` [PATCH v3] " Isaac Dunham 1 sibling, 0 replies; 14+ messages in thread From: Isaac Dunham @ 2014-06-20 4:46 UTC (permalink / raw) To: musl On Thu, Jun 19, 2014 at 11:23:59PM -0400, Rich Felker wrote: > Sorry it's taken me so long to getting around to reviewing this! I > finally have meaningful comments for you. > > On Fri, Apr 25, 2014 at 09:12:33AM -0700, Isaac Dunham wrote: > > Forgot to attach the patch. > > > > On Fri, Apr 25, 2014 at 09:10:19AM -0700, Isaac Dunham wrote: > > > Thanks for the review, Rich. > > > > > > Here's a second version, with the following changes: > > > fmtmsg.h: remove comments, wrap whole header in extern "C" > > > fmtmsg.c: inline _msgtok[] (as msgs[]) and _validenv() > > > > > > fmtmsg still prints to the console if requested and permitted, as per POSIX. > > > This detail will probably only be relevant for daemons. > > > > > > HTH, > > > Isaac Dunham > > > diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c > > new file mode 100644 > > index 0000000..be75142 > > --- /dev/null > > +++ b/src/misc/fmtmsg.c > > @@ -0,0 +1,108 @@ > > +/* 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> > > + > > + > > +/* > > + * 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; > > +} > > + > > +static int _writemsg(const char *msg) > > +{ > > + int buflen = strlen(msg); > > + if (write(2, msg, buflen) < buflen) return MM_NOMSG; > > + 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; > > + char *errstring = MM_NULLSEV, *cmsg = getenv("MSGVERB"); > > + char *const msgs[] = { > > + "label", "severity", "text", "action", "tag", NULL > > + }; > > + > > + if (classification & MM_CONSOLE) > > + consolefd = open("/dev/console", O_WRONLY); > > + if (consolefd < 0) { > > + classification &= ~MM_CONSOLE; > > + 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) { > > + if (dprintf(consolefd, "%s: %s: %s\nTO FIX: %s %s\n", > > + label, errstring, text, action, tag)<15) > > + ret = MM_NOCON; > > + } > > + > > + if (cmsg) { > > + char *evar = cmsg; > > + while (evar && evar[0]) { > > + for(i=0; msgs[i]; i++) { > > + if (!_strcolcmp(msgs[i], evar)) break; > > + } > > + if (msgs[i] == NULL) cmsg = 0; > > + evar = strchr(evar, ':'); > > + if (evar) evar++; > > + } I should have commented this part. It's supposed to validate MSGVERB. How, or even that, it does so is very non-obvious, so I'll try to explain. _strcolcmp is comparing colon-separated portions of MSGVERB with the legal components of MSGVERB. If it doesn't find a match, it eliminates MSGVERB (cmsg=0). If you encounter "::", it will also eliminate MSGVERB, since _strcolcmp sees ":...", which corresponds to "\0..." (a null string), which doesn't match. Of course, I should have made it bail as soon as cmsg = 0. > > + } > > + for (i=0; (classification & MM_PRINT) && (i < 6);) { > > + if (cmsg) i = 0; > > + while (cmsg && msgs[i]) { > > + if (!_strcolcmp(msgs[i], cmsg)) break; > > + i++; > > + } > > It looks like you've gone to some trouble to allow MSGVERB to reorder > the output, but per the standard I don't think it's supposed to do > this. The wording is not very clear but it simply says (1) "If MSGVERB > contains a keyword for a component and the component's value is not > the component's null value, fmtmsg() shall include that component in > the message when writing the message to standard error" and (2) "The > keywords may appear in any order". My reading of "include" is that it > does not impose an ordering on the output. Unfortunately the examples > don't clarify this. Can you check what existing implementation do? I wasn't sure abut that myself; I'll test, though I can only do so with a very old glibc version at present (I installed Alpine on a formerly NetBSD system just in this past week; my current main computer has a leftover install of Jaunty; and my preferred laptop has Squeeze, Sid/Jessie...and some overheating issue). Reading the NetBSD code online, it seems that they use something like what you suggest down below, which indicates that they read it the way you do. > > > + i++; > > + switch (i) { > > + case 1: > > + ret |= _writemsg(label); > > + ret |= _writemsg(": "); > > + break; > > + case 2: > > + if (errstring){ > > + ret |= _writemsg(errstring); > > + ret |= _writemsg(": "); > > + } > > + break; > > + case 3: > > + ret |= _writemsg(text); > > + break; > > + case 4: > > + ret |= _writemsg("\nTO FIX: "); > > + ret |= _writemsg(action); > > + break; > > + case 5: > > + ret |= _writemsg(" "); > > + ret |= _writemsg(tag); > > + break; > > + } > > + if (cmsg) { > > + cmsg = strchr(cmsg, ':'); > > + if (cmsg) cmsg++; > > + if (!cmsg || !cmsg[0]) i = 6; > > + } > > All these individual small writes are pretty costly in syscall > overhead. It would be really nice if a single dprintf could do all of > them together (it buffers), which is definitely possible if MSGVERB > doesn't need to reorder them. The strategy for getting dprintf to do > arbitrary subsets would be using %.*s with a precision argument of > either -1 or 0 to control whether the field is printed or not. I think > this would also debloat the code a bit. As far as I can tell, this would probably be proper. (NetBSD does this with fprintf; I didn't want to call fprintf on /dev/console, because that requires using fopen, which in turn needs malloc() to succeed). > One other thing I noticed: "If MSGVERB is not defined, if its value is > the null string, if its value is not of the correct format, or if it > contains keywords other than the valid ones listed above, fmtmsg() > shall select all components." I think it's necessary to validate the > MSGVERB string to comply with this requirement. See my explanation of how I did this. > > Rich Thanks, Isaac ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] Implement fmtmsg 2014-06-20 3:23 ` Rich Felker 2014-06-20 4:46 ` Isaac Dunham @ 2014-06-20 14:32 ` Isaac Dunham 2014-06-21 4:46 ` Rich Felker 1 sibling, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-06-20 14:32 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 739 bytes --] Here's a third revision of fmtmsg. As per Rich's comments, my testing with glibc, and my reading of netbsd, fmtmsg no longer lets MSGVERB reorder output. This involved a switch from _writemsg to dprintf, as well as using an int to store the components to print. Note that any argument can be 0 or (char*)0; this should result in no output of that field. I also made console output handle that, which could be done in one of two ways: dprintf("%s...", msg?msg:"", ...) or if (!msg) msg=""; ... dprintf("%s...", msg, ...) The latter actually won't work too well, now that I think about it, since the field separators cannot be removed that way. The other change is one comment in the section that checks MSGVERB. ;) Thanks, Isaac Dunham [-- Attachment #2: fmtmsg.diff --] [-- Type: text/plain, Size: 3567 bytes --] diff --git a/include/fmtmsg.h b/include/fmtmsg.h new file mode 100644 index 0000000..4bc34d3 --- /dev/null +++ b/include/fmtmsg.h @@ -0,0 +1,47 @@ +#ifndef _FMTMSG_H +#define _FMTMSG_H + +#ifdef __cplusplus +extern "C" { +#endif + +#define MM_HARD 0x0001 +#define MM_SOFT 0x0002 +#define MM_FIRM 0x0004 + +#define MM_APPL 0x0008 +#define MM_UTIL 0x0010 +#define MM_OPSYS 0x0020 + +#define MM_PRINT 0x0100 +#define MM_CONSOLE 0x0200 + +#define MM_RECOVER 0x0040 +#define MM_NRECOV 0x0080 + +#define MM_NULLMC 0L + +#define MM_HALT 0x1 +#define MM_ERROR 0x2 +#define MM_WARNING 0x3 +#define MM_INFO 0x4 +#define MM_NOSEV 0x0 + +#define MM_OK 0x00000000 +#define MM_NOTOK 0xffffffff +#define MM_NOMSG 0x00000001 +#define MM_NOCON 0x00000004 + +#define MM_NULLLBL (char*)0 +#define MM_NULLTXT (char*)0 +#define MM_NULLACT (char*)0 +#define MM_NULLTAG (char*)0 +#define MM_NULLSEV 0 + +int fmtmsg(long, const char *, int, const char *, const char *, const char *); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/src/misc/fmtmsg.c b/src/misc/fmtmsg.c new file mode 100644 index 0000000..98fdaca --- /dev/null +++ b/src/misc/fmtmsg.c @@ -0,0 +1,84 @@ +/* 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> + + +/* + * 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 + }; + + if (classification & MM_CONSOLE) + consolefd = open("/dev/console", O_WRONLY); + if (consolefd < 0) { + classification &= ~MM_CONSOLE; + 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) { + 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; + } + + if (cmsg) { + 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 (classification & MM_PRINT) { + 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; + return ret; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-20 14:32 ` [PATCH v3] " Isaac Dunham @ 2014-06-21 4:46 ` Rich Felker 2014-06-21 13:41 ` Isaac Dunham 0 siblings, 1 reply; 14+ messages in thread From: Rich Felker @ 2014-06-21 4:46 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 794 bytes --] 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. Thanks! Rich [-- Attachment #2: fmtmsg.c --] [-- Type: text/plain, Size: 2467 bytes --] /* 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 (classification & MM_CONSOLE) consolefd = open("/dev/console", O_WRONLY); if (consolefd < 0) { classification &= ~MM_CONSOLE; 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) { 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; } 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 (classification & MM_PRINT) { 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; } [-- Attachment #3: fmtmsg.h --] [-- Type: text/plain, Size: 741 bytes --] #ifndef _FMTMSG_H #define _FMTMSG_H #ifdef __cplusplus extern "C" { #endif #define MM_HARD 1 #define MM_SOFT 2 #define MM_FIRM 4 #define MM_APPL 8 #define MM_UTIL 16 #define MM_OPSYS 32 #define MM_RECOVER 64 #define MM_NRECOV 128 #define MM_PRINT 256 #define MM_CONSOLE 512 #define MM_NULLMC 0L #define MM_HALT 1 #define MM_ERROR 2 #define MM_WARNING 3 #define MM_INFO 4 #define MM_NOSEV 0 #define MM_OK 0 #define MM_NOTOK (-1) #define MM_NOMSG 1 #define MM_NOCON 4 #define MM_NULLLBL ((char*)0) #define MM_NULLTXT ((char*)0) #define MM_NULLACT ((char*)0) #define MM_NULLTAG ((char*)0) #define MM_NULLSEV 0 int fmtmsg(long, const char *, int, const char *, const char *, const char *); #ifdef __cplusplus } #endif #endif ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 4:46 ` Rich Felker @ 2014-06-21 13:41 ` Isaac Dunham 2014-06-21 14:39 ` Rich Felker 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-06-21 13:41 UTC (permalink / raw) To: musl 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). 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]). > Thanks! > > Rich 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 (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. > 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) { > 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; > } > > 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 (classification & MM_PRINT) { > 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; > } > #ifndef _FMTMSG_H > #define _FMTMSG_H > > #ifdef __cplusplus > extern "C" { > #endif > > #define MM_HARD 1 > #define MM_SOFT 2 > #define MM_FIRM 4 > > #define MM_APPL 8 > #define MM_UTIL 16 > #define MM_OPSYS 32 > > #define MM_RECOVER 64 > #define MM_NRECOV 128 > > #define MM_PRINT 256 > #define MM_CONSOLE 512 > > #define MM_NULLMC 0L > > #define MM_HALT 1 > #define MM_ERROR 2 > #define MM_WARNING 3 > #define MM_INFO 4 > #define MM_NOSEV 0 > > #define MM_OK 0 > #define MM_NOTOK (-1) > #define MM_NOMSG 1 > #define MM_NOCON 4 > > #define MM_NULLLBL ((char*)0) > #define MM_NULLTXT ((char*)0) > #define MM_NULLACT ((char*)0) > #define MM_NULLTAG ((char*)0) > #define MM_NULLSEV 0 > > int fmtmsg(long, const char *, int, const char *, const char *, const char *); > > #ifdef __cplusplus > } > #endif > > #endif ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 13:41 ` Isaac Dunham @ 2014-06-21 14:39 ` Rich Felker 2014-06-21 15:56 ` Isaac Dunham 0 siblings, 1 reply; 14+ messages in thread From: Rich Felker @ 2014-06-21 14:39 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 14:39 ` Rich Felker @ 2014-06-21 15:56 ` Isaac Dunham 2014-06-21 16:18 ` Rich Felker 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-06-21 15:56 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 15:56 ` Isaac Dunham @ 2014-06-21 16:18 ` Rich Felker 2014-06-21 17:13 ` Isaac Dunham 0 siblings, 1 reply; 14+ messages in thread From: Rich Felker @ 2014-06-21 16:18 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 1170 bytes --] 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. Rich [-- Attachment #2: fmtmsg.c --] [-- Type: text/plain, Size: 2473 bytes --] /* 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; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 16:18 ` Rich Felker @ 2014-06-21 17:13 ` Isaac Dunham 2014-06-21 23:32 ` Rich Felker 0 siblings, 1 reply; 14+ messages in thread From: Isaac Dunham @ 2014-06-21 17:13 UTC (permalink / raw) To: musl 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; > } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] Implement fmtmsg 2014-06-21 17:13 ` Isaac Dunham @ 2014-06-21 23:32 ` Rich Felker 0 siblings, 0 replies; 14+ messages in thread From: Rich Felker @ 2014-06-21 23:32 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-06-21 23:32 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-04-25 5:06 [PATCH] Implement fmtmsg Isaac Dunham 2014-04-25 14:49 ` Rich Felker 2014-04-25 16:10 ` [PATCH v2] " Isaac Dunham 2014-04-25 16:12 ` Isaac Dunham 2014-06-20 3:23 ` Rich Felker 2014-06-20 4:46 ` Isaac Dunham 2014-06-20 14:32 ` [PATCH v3] " Isaac Dunham 2014-06-21 4:46 ` Rich Felker 2014-06-21 13:41 ` Isaac Dunham 2014-06-21 14:39 ` Rich Felker 2014-06-21 15:56 ` Isaac Dunham 2014-06-21 16:18 ` Rich Felker 2014-06-21 17:13 ` Isaac Dunham 2014-06-21 23:32 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).