Re: error.h implementation

[James Y Knight <jyknight@google.com>]

[-- Attachment #1: Type: text/plain, Size: 6098 bytes --]

On Mon, Jul 22, 2019 at 9:16 PM Rich Felker <dalias@libc.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.

[-- Attachment #2: 0001-Add-support-for-the-glibc-specific-error.h-header.patch --]
[-- Type: text/x-patch, Size: 3170 bytes --]

From 988181b71ee24df0c349a2c0ad0a0e656eefe7e9 Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.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