From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/4954 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Implement fmtmsg Date: Fri, 25 Apr 2014 10:49:21 -0400 Message-ID: <20140425144921.GO26358@brightrain.aerifal.cx> References: <20140425050559.GA5487@muslin> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1398437383 11875 80.91.229.3 (25 Apr 2014 14:49:43 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 25 Apr 2014 14:49:43 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-4958-gllmg-musl=m.gmane.org@lists.openwall.com Fri Apr 25 16:49:37 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1WdhRY-0001oF-9v for gllmg-musl@plane.gmane.org; Fri, 25 Apr 2014 16:49:36 +0200 Original-Received: (qmail 9351 invoked by uid 550); 25 Apr 2014 14:49:34 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 9339 invoked from network); 25 Apr 2014 14:49:34 -0000 Content-Disposition: inline In-Reply-To: <20140425050559.GA5487@muslin> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:4954 Archived-At: 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 > +#include > +#include > +#include > +#include > +#include > + > +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