[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 20 Jun 2014 04:46:55 +0000
From: Isaac Dunham <ibid.ag@...il.com>
To: musl@...ts.openwall.com
Subject: Re: Re: [PATCH v2] Implement fmtmsg
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
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.
