* [PATCH] lib: Fix unused parameter warning in lib/error.c @ 2021-09-08 18:21 Colin Cross 2021-09-09 10:09 ` Mark Wielaard 0 siblings, 1 reply; 4+ messages in thread From: Colin Cross @ 2021-09-08 18:21 UTC (permalink / raw) To: elfutils-devel; +Cc: Colin Cross Mark the errnum parameter with __attribute__((unused)). Signed-off-by: Colin Cross <ccross@google.com> --- lib/error.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/error.c b/lib/error.c index 75e964fd..651a0f3d 100644 --- a/lib/error.c +++ b/lib/error.c @@ -35,7 +35,7 @@ unsigned int error_message_count = 0; -void error(int status, int errnum, const char *format, ...) { +void error(int status, int errnum __attribute__ ((unused)), const char *format, ...) { va_list argp; va_start(argp, format); -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib: Fix unused parameter warning in lib/error.c 2021-09-08 18:21 [PATCH] lib: Fix unused parameter warning in lib/error.c Colin Cross @ 2021-09-09 10:09 ` Mark Wielaard 2021-09-10 18:07 ` [PATCH] lib: Make error.c more like error(3) Colin Cross 0 siblings, 1 reply; 4+ messages in thread From: Mark Wielaard @ 2021-09-09 10:09 UTC (permalink / raw) To: Colin Cross, elfutils-devel Hi Colin (CC Saleem who introduced this new error replacement function), On Wed, 2021-09-08 at 11:21 -0700, Colin Cross via Elfutils-devel wrote: > Mark the errnum parameter with __attribute__((unused)). Thanks, that is interesting. But I think this is an actual bug in the code. Rereviewing the new replacement error function: > #if !defined(HAVE_ERROR_H) && defined(HAVE_ERR_H) > #include <stdarg.h> > #include <stdlib.h> > #include <err.h> > > unsigned int error_message_count = 0; > > void error(int status, int errnum, const char *format, ...) { > va_list argp; > > va_start(argp, format); > verr(status, format, argp); > va_end(argp); > > if (status) > exit(status); > ++error_message_count; > } > #endif I see three issues with the code: 1) error is supposed to first flush stdout before printing to stderr. This is minor, but might make the error message appear differently when stdout and stderr are mixed. 2) errnum isn't actually used as you noticed. error is supposed to print strerror(errnum) if errnum is not zero. Instead verr print strerror(errno), even when errno is zero. So I think the code should use errnum (if it is not zero), save the current value of errno, assign errnum to errno, call verr and set errno back. 3) error ignores status if it is zero, but verr seems to call exit (0) meaning it terminates the program instead of simply printing an error and increasing error_message_count. Could the error replacement function be rewritten to behave more correctly? Thanks, Mark ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] lib: Make error.c more like error(3) 2021-09-09 10:09 ` Mark Wielaard @ 2021-09-10 18:07 ` Colin Cross 2021-09-12 21:00 ` Mark Wielaard 0 siblings, 1 reply; 4+ messages in thread From: Colin Cross @ 2021-09-10 18:07 UTC (permalink / raw) To: elfutils-devel; +Cc: Colin Cross Fix some issues with the error reimplementation to make it match the specification for error(3). Flush stdout before printing to stderr. Also flush stderr afterwards, which is not specified in the man page for error(3), but is what bionic does. error(3) prints strerror(errnum) if and only if errnum is nonzero, but verr prints strerror(errno) unconditionaly. When errnum is nonzero copy it to errno and use verr, and when it is not set use verrx that doesn't print errno. error(3) only exits if status is nonzero, but verr exits uncondtionally. Use vwarn/vwarnx when status is zero, which don't exit. Signed-off-by: Colin Cross <ccross@google.com> --- lib/error.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/error.c b/lib/error.c index 75e964fd..dba046ef 100644 --- a/lib/error.c +++ b/lib/error.c @@ -29,7 +29,9 @@ #include <config.h> #if !defined(HAVE_ERROR_H) && defined(HAVE_ERR_H) +#include <errno.h> #include <stdarg.h> +#include <stdio.h> #include <stdlib.h> #include <err.h> @@ -37,13 +39,32 @@ unsigned int error_message_count = 0; void error(int status, int errnum, const char *format, ...) { va_list argp; + int saved_errno = errno; + + fflush(stdout); va_start(argp, format); - verr(status, format, argp); + if (status) { + if (errnum) { + errno = errnum; + verr(status, format, argp); + } else { + verrx(status, format, argp); + } + } else { + if (errnum) { + errno = errnum; + vwarn(format, argp); + } else { + vwarnx(format, argp); + } + } va_end(argp); - if (status) - exit(status); + fflush(stderr); + ++error_message_count; + + errno = saved_errno; } #endif -- 2.33.0.309.g3052b89438-goog ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib: Make error.c more like error(3) 2021-09-10 18:07 ` [PATCH] lib: Make error.c more like error(3) Colin Cross @ 2021-09-12 21:00 ` Mark Wielaard 0 siblings, 0 replies; 4+ messages in thread From: Mark Wielaard @ 2021-09-12 21:00 UTC (permalink / raw) To: Colin Cross; +Cc: elfutils-devel [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] Hi Colin, On Fri, Sep 10, 2021 at 11:07:16AM -0700, Colin Cross via Elfutils-devel wrote: > Fix some issues with the error reimplementation to make it match > the specification for error(3). > > Flush stdout before printing to stderr. Also flush stderr afterwards, > which is not specified in the man page for error(3), but is what > bionic does. > > error(3) prints strerror(errnum) if and only if errnum is nonzero, > but verr prints strerror(errno) unconditionaly. When errnum is nonzero > copy it to errno and use verr, and when it is not set use verrx that > doesn't print errno. > > error(3) only exits if status is nonzero, but verr exits uncondtionally. > Use vwarn/vwarnx when status is zero, which don't exit. > > Signed-off-by: Colin Cross <ccross@google.com> This is really nice. Thanks. I pushed it with a ChangeLog entry and reformatted the patch to GNU style as attached. Note that there are still some small differences between "real" error and our error implementation, which don't seem to matter in practice, but do cause some testsuite failures because of silly formatting. e.g. run-stack-i.sh fails with: -stack: tid 13654: shown max number of frames (6, use -n 0 for unlimited) +/home/mark/src/elfutils/src/stack: tid 13654: shown max number of frames (6, use -n 0 for unlimited) I have not tries to fix these. The testcases probably should be slightly less picky about the exact error message format. Cheers, Mark [-- Attachment #2: 0001-lib-Make-error.c-more-like-error-3.patch --] [-- Type: text/x-diff, Size: 2488 bytes --] From 7582a0d3e09ee154961bbba9285a224e5d09f407 Mon Sep 17 00:00:00 2001 From: Colin Cross <ccross@google.com> Date: Fri, 10 Sep 2021 11:07:16 -0700 Subject: [PATCH] lib: Make error.c more like error(3) Fix some issues with the error reimplementation to make it match the specification for error(3). Flush stdout before printing to stderr. Also flush stderr afterwards, which is not specified in the man page for error(3), but is what bionic does. error(3) prints strerror(errnum) if and only if errnum is nonzero, but verr prints strerror(errno) unconditionaly. When errnum is nonzero copy it to errno and use verr, and when it is not set use verrx that doesn't print errno. error(3) only exits if status is nonzero, but verr exits uncondtionally. Use vwarn/vwarnx when status is zero, which don't exit. Signed-off-by: Colin Cross <ccross@google.com> --- lib/ChangeLog | 5 +++++ lib/error.c | 32 +++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/lib/ChangeLog b/lib/ChangeLog index 96eaa330..c72452b1 100644 --- a/lib/ChangeLog +++ b/lib/ChangeLog @@ -1,3 +1,8 @@ +2021-09-10 Colin Cross <ccross@google.com> + + * error.c (error): Call fflush on stdout and stderr. Setup errno and + call verr, verrx, vwarn or vwarnx based on status and errnum. + 2021-09-06 Dmitry V. Levin <ldv@altlinux.org> * color.c (parse_opt): Replace asprintf followed by error(EXIT_FAILURE) diff --git a/lib/error.c b/lib/error.c index 75e964fd..5186fc15 100644 --- a/lib/error.c +++ b/lib/error.c @@ -29,7 +29,9 @@ #include <config.h> #if !defined(HAVE_ERROR_H) && defined(HAVE_ERR_H) +#include <errno.h> #include <stdarg.h> +#include <stdio.h> #include <stdlib.h> #include <err.h> @@ -37,13 +39,37 @@ unsigned int error_message_count = 0; void error(int status, int errnum, const char *format, ...) { va_list argp; + int saved_errno = errno; + + fflush (stdout); va_start(argp, format); - verr(status, format, argp); + if (status) + { + if (errnum) + { + errno = errnum; + verr (status, format, argp); + } + else + verrx (status, format, argp); + } + else + { + if (errnum) + { + errno = errnum; + vwarn (format, argp); + } + else + vwarnx (format, argp); + } va_end(argp); - if (status) - exit(status); + fflush (stderr); + ++error_message_count; + + errno = saved_errno; } #endif -- 2.32.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-12 21:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-08 18:21 [PATCH] lib: Fix unused parameter warning in lib/error.c Colin Cross 2021-09-09 10:09 ` Mark Wielaard 2021-09-10 18:07 ` [PATCH] lib: Make error.c more like error(3) Colin Cross 2021-09-12 21:00 ` Mark Wielaard
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 read-only IMAP folder(s) and NNTP newsgroup(s).