* error.h implementation
@ 2019-07-23 3:53 James Y Knight
2019-07-23 4:16 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: James Y Knight @ 2019-07-23 3:53 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
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.
The added test case here passes with both musl and glibc.
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?
[-- Attachment #2: 0001-Add-support-for-the-glibc-specific-error.h-header.patch --]
[-- Type: application/octet-stream, Size: 3685 bytes --]
From b6f37d7555a68392ea976621aa9d408efec75c9e Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.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;
};
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, ...);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
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) {
+ ++__error_message_count;
+
+ fflush(stdout);
+ FLOCK(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);
+
+ fflush(stderr);
+ FUNLOCK(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);
+}
+
+
+weak_alias(__error_print_progname, error_print_progname);
+weak_alias(__error_message_count, error_message_count);
+weak_alias(__error_one_per_line, error_one_per_line);
+
+weak_alias(__error, error);
+weak_alias(__error_at_line, error_at_line);
--
2.22.0.657.g960e92d24f-goog
[-- Attachment #3: 0001-Add-tests-for-error.h-functions.patch --]
[-- Type: application/octet-stream, Size: 2341 bytes --]
From 751247d2cdcf3f64aa27da0dc9801169b78abec5 Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Mon, 22 Jul 2019 22:57:39 -0400
Subject: [PATCH] Add tests for <error.h> functions.
---
src/functional/error.c | 68 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
create mode 100644 src/functional/error.c
diff --git a/src/functional/error.c b/src/functional/error.c
new file mode 100644
index 0000000..e0b063c
--- /dev/null
+++ b/src/functional/error.c
@@ -0,0 +1,68 @@
+#include <error.h>
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include "test.h"
+
+#define ASSERT(c) do { \
+ errno = 0; \
+ if (!(c)) { \
+ t_error("%s failed (errno: %s)\n", #c, strerror(errno)); \
+ exit(1); \
+ } \
+} while(0)
+
+void my_print_progname(void) {
+ fputs("Progname:", stderr);
+}
+
+const char *expected_output =
+ "src/functional/error.exe: Test1\n"
+ "src/functional/error.exe:File:44: Test2 hello\n"
+ "Progname:File:44: Test3 4\n"
+ "Progname:Test4 hello: No such file or directory\n"
+ "Progname:OtherFile:44: Test6\n"
+ "Progname:OtherFile:47: Test7\n"
+ "error_message_count:6\n"
+ "Progname:Last error\n";
+
+int main() {
+ int fd, pid, status;
+ int pipefds[2];
+ ASSERT(pipe(pipefds) == 0);
+
+ ASSERT((pid = fork()) >= 0);
+ if (pid == 0) {
+ ASSERT(dup2(pipefds[1], 1) == 1);
+ ASSERT(dup2(pipefds[1], 2) == 2);
+
+ error(0, 0, "Test1");
+ error_at_line(0, 0, "File", 44, "Test2 %s", "hello");
+
+ error_print_progname = my_print_progname;
+
+ error_one_per_line = 1;
+ error_at_line(0, 0, "File", 44, "Test3 %d", 4);
+ error(0, ENOENT, "Test4 %s", "hello");
+ error_at_line(0, 0, "File", 44, "Test5 (skipped)");
+ error_at_line(0, 0, "OtherFile", 44, "Test6");
+ error_at_line(0, 0, "OtherFile", 47, "Test7");
+ printf("error_message_count:%d\n", error_message_count);
+ error(77, 0, "Last error");
+ }
+ ASSERT(waitpid(pid, &status, 0) == pid);
+ ASSERT(WIFEXITED(status) && WEXITSTATUS(status) == 77);
+
+ char buf[1000];
+ ssize_t count;
+ ASSERT((count = read(pipefds[0], buf, sizeof(buf)-1)) >= 0);
+ buf[count] = 0;
+ if (strcmp(buf, expected_output)) {
+ t_error("Unexpected output. Got:\n%s\n", buf);
+ }
+ return t_status;
+}
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: error.h implementation
2019-07-23 3:53 error.h implementation James Y Knight
@ 2019-07-23 4:16 ` Rich Felker
2019-07-30 16:03 ` James Y Knight
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2019-07-23 4:16 UTC (permalink / raw)
To: musl
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@google.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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: error.h implementation
2019-07-23 4:16 ` Rich Felker
@ 2019-07-30 16:03 ` James Y Knight
2019-08-06 16:13 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: James Y Knight @ 2019-07-30 16:03 UTC (permalink / raw)
To: musl
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: error.h implementation
2019-07-30 16:03 ` James Y Knight
@ 2019-08-06 16:13 ` Rich Felker
2019-08-06 22:06 ` James Y Knight
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2019-08-06 16:13 UTC (permalink / raw)
To: musl
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@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.
> 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
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: error.h implementation
2019-08-06 16:13 ` Rich Felker
@ 2019-08-06 22:06 ` James Y Knight
2019-08-06 22:50 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: James Y Knight @ 2019-08-06 22:06 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 3778 bytes --]
On Tue, Aug 6, 2019, 12:13 PM Rich Felker <dalias@libc.org> wrote:
> > 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.
>
Hmm, good question! I expect most uses are single threaded but it's better
not to be thread-unsafe.
So, I think it's probably best to move everything within the stderr lock
(which means moving the flockfile out to the error and error_at_line
functions). That at least makes these safe w.r.t. themselves. Users
accessing the public globals will still have to take care, which is
unavoidable and fine.
I just noticed this is still using '__progname_full' instead of the
public 'program_invocation_name',
which I should also fix.
(But I'm out and won't be able to update anything for a couple weeks,
sorry!)
>
[-- Attachment #2: Type: text/html, Size: 5267 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: error.h implementation
2019-08-06 22:06 ` James Y Knight
@ 2019-08-06 22:50 ` Rich Felker
0 siblings, 0 replies; 6+ messages in thread
From: Rich Felker @ 2019-08-06 22:50 UTC (permalink / raw)
To: musl
On Tue, Aug 06, 2019 at 06:06:12PM -0400, James Y Knight wrote:
> On Tue, Aug 6, 2019, 12:13 PM Rich Felker <dalias@libc.org> wrote:
>
> > > 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.
> >
>
> Hmm, good question! I expect most uses are single threaded but it's better
> not to be thread-unsafe.
>
> So, I think it's probably best to move everything within the stderr lock
> (which means moving the flockfile out to the error and error_at_line
> functions). That at least makes these safe w.r.t. themselves. Users
> accessing the public globals will still have to take care, which is
> unavoidable and fine.
I don't think the error_one_per_line stuff can be made safe at all,
without TLS, due to the issue about the lifetime of the string passed
to it. So it's probably ok declaring this one as hard thread-unsafe
(and so broken it shouldn't be used). The rest of the cases then
should be safe if you just move the increment inside the lock, I
think.
> I just noticed this is still using '__progname_full' instead of the
> public 'program_invocation_name',
> which I should also fix.
>
> (But I'm out and won't be able to update anything for a couple weeks,
> sorry!)
Rather than leave things in limbo, I'm happy to polish up and commit
patches you have pending. I'll probably be making another release in
roughly this timeframe and would like to get these things (especially
the glob fix) in.
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-06 22:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 3:53 error.h implementation James Y Knight
2019-07-23 4:16 ` Rich Felker
2019-07-30 16:03 ` James Y Knight
2019-08-06 16:13 ` Rich Felker
2019-08-06 22:06 ` James Y Knight
2019-08-06 22:50 ` Rich Felker
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).