[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140425144921.GO26358@brightrain.aerifal.cx>
Date: Fri, 25 Apr 2014 10:49:21 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Implement fmtmsg
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
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.
