[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190723041615.GB15665@brightrain.aerifal.cx>
Date: Tue, 23 Jul 2019 00:16:15 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: error.h implementation
On Mon, Jul 22, 2019 at 11:53:33PM -0400, James Y Knight wrote:
> While these glibc-introduced functions are, IMO, ugly and not really a
> worthwhile addition to a libc interface, they do get used. So, in the
> interests of compatibility with existing code, here's an
> implementation...they're standalone, and pretty trivial, so it seems
> reasonable to include, despite it being unfortunate that they exist at
> all.
Not sure on this yet, but some review anyway, inline below.
> A question: is the weak_alias stuff required in this case? I'm not
> 100% clear as to when that's required, and when it's not, but I think
> perhaps it's not actually needed here, because nothing else in the
> libc refers to these nonstandard symbols, and the file defines no
> standard symbols?
Not needed. It would only be needed if they were being used to
implement something else in one of the standards-governed namespaces.
> From b6f37d7555a68392ea976621aa9d408efec75c9e Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@...gle.com>
> Date: Mon, 22 Jul 2019 20:39:26 -0400
> Subject: [PATCH] Add support for the glibc-specific error.h header.
>
> This includes the functions 'error', 'error_at_line', and their
> associated global variables.
> ---
> dynamic.list | 7 ++++
> include/error.h | 23 +++++++++++++
> src/legacy/error.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 110 insertions(+)
> create mode 100644 include/error.h
> create mode 100644 src/legacy/error.c
>
> diff --git a/dynamic.list b/dynamic.list
> index ee0d363b..d8f57b85 100644
> --- a/dynamic.list
> +++ b/dynamic.list
> @@ -42,4 +42,11 @@ __progname;
> __progname_full;
>
> __stack_chk_guard;
> +
> +error_print_progname;
> +error_message_count;
> +error_one_per_line;
> +__error_print_progname;
> +__error_message_count;
> +__error_one_per_line;
> };
This is a rather bleh aspect of adding them. At least three of those
would be gone with the aliases removed.
> diff --git a/include/error.h b/include/error.h
> new file mode 100644
> index 00000000..d4e7fad6
> --- /dev/null
> +++ b/include/error.h
> @@ -0,0 +1,23 @@
> +#ifndef _ERROR_H
> +#define _ERROR_H
> +
> +#include <features.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern void (*error_print_progname) (void);
> +extern unsigned int error_message_count;
> +extern int error_one_per_line;
> +
> +void error(int status, int errnum, const char *fmt, ...);
> +void error_at_line(int status, int errnum,
> + const char *file, unsigned int linenum,
> + const char *fmt, ...);
We don't use argument names in header files. Also the convention of
aligning continuations of function arguments like this is not
generally used (and not often needed without names taking up lots of
column space), but not objectionable as long as it's aligned right.
> diff --git a/src/legacy/error.c b/src/legacy/error.c
> new file mode 100644
> index 00000000..2b820954
> --- /dev/null
> +++ b/src/legacy/error.c
> @@ -0,0 +1,80 @@
> +#include <error.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include "stdio_impl.h"
> +#include "libc.h"
> +
> +void (*__error_print_progname) (void) = 0;
> +unsigned int __error_message_count = 0;
> +int __error_one_per_line = 0;
> +
> +static unsigned int saved_linenum = 0;
> +static const char *saved_file = 0;
> +
> +static void errorv(int status, int errnum,
> + const char *file, unsigned int linenum,
> + const char *fmt, va_list ap) {
But this one isn't done right -- it's mixing tabs and spaces. Tabs
should only be used for indention levels, spaces for alignment. So if
you were aligning a continued line in an already-indented context, the
continuation would start with the same number of tabs as the line
being continued, followed by spaces to align. This is necessary to be
agnostic to the width of tabs, which is an accessibility issue.
Also, while { on same line is used for ifs/loops, it's not used for
start of functions; { always goes on its own line here.
> + ++__error_message_count;
> +
> + fflush(stdout);
> + FLOCK(stderr);
Please avoid using anything from stdio_impl.h or other implementation
internals when implementing nonstandard functionality that can be done
with 100% portable code in terms of the standard functions (e.g.
flockfile). This principle avoids having "junk" interfaces like the
error.h stuff become maintenance burdens by requiring someone to
understand and change them if the internals change.
> + if (__error_print_progname)
> + __error_print_progname();
And since you're calling an application-provided callback, it's
probably even necessary to use flockfile. I'm not sure it's safe to
use the FLOCK() macro when application code may run in the context
holding the lock, and there's not an intent that it be safe.
> + else {
> + fprintf(stderr, "%s:", __progname_full);
> + if (!file)
> + fputc(' ', stderr);
> + }
> +
> + if (file)
> + fprintf(stderr, "%s:%u: ", file, linenum);
> +
> + vfprintf(stderr, fmt, ap);
> + if (errnum)
> + fprintf(stderr, ": %s", strerror(errnum));
> + fputc('\n', stderr);
> +
> + fflush(stderr);
> + FUNLOCK(stderr);
Is there a reason for the fflush? stderr is unbuffered by default, and
presumably if someone makes it buffered, they want it that way for a
reason..?
Also, the first fflush, if it's to be done, should be done after
locking, not before. Otherwise you just take and release the lock
twice for no reason.
> +void __error_at_line(int status, int errnum,
> + const char *file, unsigned int linenum,
> + const char *fmt, ...) {
> + if (error_one_per_line) {
> + if(saved_linenum == linenum && file != NULL &&
> + saved_file != NULL && !strcmp(file, saved_file))
> + return;
> + saved_linenum = linenum;
> + // Assuming that the lifetime of the passed in file name extends
> + // until the next call is rather questionable, but appears to be
> + // the expected semantics.
> + saved_file = file;
> + }
I'm not sure if this is okay or not. It probably should have been
designed as a macro that passes __FILE__ and __LINE__ opaquely to the
application, so that the callee can assume file points to a literal
with static storage, but of course everything about these functions is
awful and maybe the implied contract is that you're supposed to use it
that way.
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.
