[PATCH] Implement fmtmsg

[PATCH] Implement fmtmsg

[Isaac Dunham]

[-- 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]);
}

Re: [PATCH] Implement fmtmsg

[Rich Felker]

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

[PATCH v2] Implement fmtmsg

[Isaac Dunham]

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

Re: [PATCH v2] Implement fmtmsg

[Isaac Dunham]

[-- 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;
+}

Re: Re: [PATCH v2] Implement fmtmsg

[Rich Felker]

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

Re: Re: [PATCH v2] Implement fmtmsg

[Isaac Dunham]

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

[PATCH v3] Implement fmtmsg

[Isaac Dunham]

[-- 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;
+}

Re: [PATCH v3] Implement fmtmsg

[Rich Felker]

[-- 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

Re: [PATCH v3] Implement fmtmsg

[Isaac Dunham]

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

Re: [PATCH v3] Implement fmtmsg

[Rich Felker]

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

Re: [PATCH v3] Implement fmtmsg

[Isaac Dunham]

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

Re: [PATCH v3] Implement fmtmsg

[Rich Felker]

[-- 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;
}

Re: [PATCH v3] Implement fmtmsg

[Isaac Dunham]

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;
> }

Re: [PATCH v3] Implement fmtmsg

[Rich Felker]

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