[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190806161331.GX9017@brightrain.aerifal.cx>
Date: Tue, 6 Aug 2019 12:13:31 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: error.h implementation
On Tue, Jul 30, 2019 at 12:03:56PM -0400, James Y Knight wrote:
> On Mon, Jul 22, 2019 at 9:16 PM Rich Felker <dalias@...c.org> wrote:
> >
> > > 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.
>
> Thanks for the confirmation, fixed.
>
> > > 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.
>
>
> Yeah. It's really rather unfortunate that copy relocations even exist
> and this list is even needed at all. Sigh.
>
> > > +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.
>
> Fixed.
>
> > > +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.
>
> indeed, sorry about that. In this case, it looks like usually the
> function signature is generally not wrapped in musl, even when it's
> rather long (in this case 111 columns)? Assuming that's the desired
> style, since it's commonly used elsewhere, I've simply unwrapped this
> line (and the same for the other functions).
>
> Digression -- while I agree in the abstract that the
> indent-vs-alignment distinction is a great idea, it's uncommonly
> supported by editors, and is invisible when you get it wrong. This
> means it requires a good deal of human attention. I think it might be
> worth considering switching to a spaces-only indentation style.
>
> > 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.
>
> Fixed.
>
> >
> > > + ++__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.
>
> That's a great principle! Fixed. I wonder if it'd be a good idea to
> shift around the source code layout for a more obvious layering --
> putting the portable library code into one subdir and the OS-specific
> bits into another.
>
> > > + 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.
>
> Indeed.
>
> > > + 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..?
>
> Nope, I agree, this seems superfluous. Removed. :)
>
> >
> > 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.
>
> The previous fflush is flushing stdout, before writing to stderr, so I
> think it doesn't belong within the locked region. The stdout flushing
> is part of the documented behavior of this function.
>
> > > +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.
>
> Yes, this design is awful, and this undocumented requirement is
> unfortunate. Yet, indeed, this is the implied contract. :(
> Fortunately, the use of the 'error_one_per_line = 1' feature is rather
> rare, e.g. see <https://codesearch.debian.net/search?q=error_one_per_line%5Cs*%3D>.
>
> Attached updated patch.
> From 988181b71ee24df0c349a2c0ad0a0e656eefe7e9 Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@...gle.com>
> Date: Tue, 30 Jul 2019 11:59:25 -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 | 4 +++
> include/error.h | 21 ++++++++++++++
> src/legacy/error.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 94 insertions(+)
> create mode 100644 include/error.h
> create mode 100644 src/legacy/error.c
>
> diff --git a/dynamic.list b/dynamic.list
> index ee0d363b..9c8450f4 100644
> --- a/dynamic.list
> +++ b/dynamic.list
> @@ -42,4 +42,8 @@ __progname;
> __progname_full;
>
> __stack_chk_guard;
> +
> +error_print_progname;
> +error_message_count;
> +error_one_per_line;
> };
> diff --git a/include/error.h b/include/error.h
> new file mode 100644
> index 00000000..03b1ca41
> --- /dev/null
> +++ b/include/error.h
> @@ -0,0 +1,21 @@
> +#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, int, const char *, ...);
> +void error_at_line(int, int, const char *, unsigned int, const char *, ...);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/src/legacy/error.c b/src/legacy/error.c
> new file mode 100644
> index 00000000..c5000fa4
> --- /dev/null
> +++ b/src/legacy/error.c
> @@ -0,0 +1,69 @@
> +#include <error.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.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)
> +{
> + ++error_message_count;
> +
> + fflush(stdout);
> + flockfile(stderr);
> +
> + if (error_print_progname)
> + error_print_progname();
> + 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);
> +
> + funlockfile(stderr);
> +
> + if (status)
> + exit(status);
> +}
> +
> +void error(int status, int errnum, const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + errorv(status, errnum, NULL, 0, fmt, ap);
> + va_end(ap);
> +}
> +
> +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;
> + }
> +
> + va_list ap;
> + va_start(ap, fmt);
> + errorv(status, errnum, file, linenum, fmt, ap);
> + va_end(ap);
> +}
> --
> 2.22.0.709.g102302147b-goog
One other question: what is the thread-safety of these interfaces
supposed to be? The globals are accessed without any synchronization,
but flockfile is used to synchronize with concurrent access to stderr.
This is plausibly correct (if the error.h functions themselves can't
be used from multiple threads, but can be used in a multithreaded
application), but at least questionable.
The error_at_line stuff is probably not possible to make thread-safe
without thread-local state for saved_file/linenum, which would be a
disproportionate cost for these interfaces.
I'm not objecting to anything with these comments, just wanting to
confirm that you think it's reasonable as-is.
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.
