[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140620032359.GR179@brightrain.aerifal.cx>
Date: Thu, 19 Jun 2014 23:23:59 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Re: [PATCH v2] Implement fmtmsg
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
