Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.