[v2,4/7] misc: syslog: Simplify implementation
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
Remove unrequired includes, use a temporary buffer for strftime instead
of using internal libio members, simplify fprintf call on the memstream
and memory allocation, use dprintf instead of writev for LOG_PERROR.
Checked on x86_64-linux-gnu.
---
misc/syslog.c | 126 ++++++++++++++------------------------------------
1 file changed, 35 insertions(+), 91 deletions(-)
Comments
On 2/18/22 06:23, Adhemerval Zanella via Libc-alpha wrote:
> + char timebuf[3+1 /* "%h " */
> + + 2+1 /* "%e " */
> + + 2+1 + 2+1 + 2+1 + 1 /* "%T " */];
> + time_t now = time_now ();
> + struct tm now_tm;
> + __localtime_r (&now, &now_tm);
> + __strftime_l (timebuf, sizeof (timebuf), "%h %e %T ", &now_tm,
> + _nl_C_locobj_ptr);
> +
> + pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
> +
> + fprintf (f, "<%d>%s %n%s%s%.0d%s: ", pri, timebuf, &msgoff,
Doesn't this output two spaces after the expansion of "%T"? The old code
output just one.
Also, the old code crashed when __localtime_r returns NULL, whereas the
new code logs garbage that might be truncated in 'timebuf'; is that
change intended?
On 19/02/2022 00:15, Paul Eggert wrote:
> On 2/18/22 06:23, Adhemerval Zanella via Libc-alpha wrote:
>> + char timebuf[3+1 /* "%h " */
>> + + 2+1 /* "%e " */
>> + + 2+1 + 2+1 + 2+1 + 1 /* "%T " */];
>> + time_t now = time_now ();
>> + struct tm now_tm;
>> + __localtime_r (&now, &now_tm);
>> + __strftime_l (timebuf, sizeof (timebuf), "%h %e %T ", &now_tm,
>> + _nl_C_locobj_ptr);
>> +
>> + pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
>> +
>> + fprintf (f, "<%d>%s %n%s%s%.0d%s: ", pri, timebuf, &msgoff,
>
> Doesn't this output two spaces after the expansion of "%T"? The old code output just one.
You are right, I will fix it.
>
> Also, the old code crashed when __localtime_r returns NULL, whereas the new code logs garbage that might be truncated in 'timebuf'; is that change intended?
Not really, but afaiu __localtime_r fails on if __offtime return EOVERFLOW
which is *highly* unlikely. Also given this is a transitory patch (the
final version will use gmtime with a proper NULL tests), I can add check
but I am not sure what to print in such cases (the RFC5424 at least gives
us a way to express such error). Should we care for this?
On Feb 18 2022, Paul Eggert wrote:
> Also, the old code crashed when __localtime_r returns NULL
How can that happen?
On 2/21/22 06:06, Andreas Schwab wrote:
>> Also, the old code crashed when __localtime_r returns NULL
> How can that happen?
When the resulting tm_year would precede INT_MIN + 1900 or follow
INT_MAX + 1900. Although this shouldn't happen on systems with accurate
clocks for billions of years, syslog should work even when the clock is
outlandishly wrong, to help diagnose clock failures etc.
On 2/21/22 05:15, Adhemerval Zanella wrote:
> this is a transitory patch (the
> final version will use gmtime with a proper NULL tests),
Ah, I hadn't notice that. Never mind, then; it's not worth worrying about.
@@ -31,34 +31,16 @@
static char sccsid[] = "@(#)syslog.c 8.4 (Berkeley) 3/18/94";
#endif /* LIBC_SCCS and not lint */
-#include <sys/types.h>
+#include <libc-lock.h>
+#include <libio/libioP.h>
+#include <math_ldbl_opt.h>
+#include <paths.h>
+#include <stdarg.h>
#include <sys/socket.h>
-#include <sys/syslog.h>
+#include <syslog.h>
#include <sys/uio.h>
#include <sys/un.h>
-#include <netdb.h>
-
-#include <errno.h>
-#include <fcntl.h>
-#include <paths.h>
-#include <stdio.h>
#include <stdio_ext.h>
-#include <string.h>
-#include <time.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <libc-lock.h>
-#include <signal.h>
-#include <locale.h>
-
-#include <stdarg.h>
-
-#include <libio/libioP.h>
-#include <math_ldbl_opt.h>
-
-#include <kernel-features.h>
-
-#define ftell(s) _IO_ftell (s)
static int LogType = SOCK_DGRAM; /* type of socket connection */
static int LogFile = -1; /* fd for log */
@@ -139,13 +121,10 @@ void
__vsyslog_internal(int pri, const char *fmt, va_list ap,
unsigned int mode_flags)
{
- struct tm now_tm;
- time_t now;
- int fd;
FILE *f;
char *buf = 0;
size_t bufsize = 0;
- size_t msgoff;
+ int msgoff;
int saved_errno = errno;
char failbuf[3 * sizeof (pid_t) + sizeof "out of memory []"];
@@ -159,9 +138,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
/* Prepare for multiple users. We have to take care: most syscalls we are
using are cancellation points. */
- struct cleanup_arg clarg;
- clarg.buf = NULL;
- clarg.oldaction = NULL;
+ struct cleanup_arg clarg = { NULL, NULL };
__libc_cleanup_push (cancel_handler, &clarg);
__libc_lock_lock (syslog_lock);
@@ -175,51 +152,24 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
/* Build the message in a memory-buffer stream. */
f = __open_memstream (&buf, &bufsize);
- if (f == NULL)
- {
- /* We cannot get a stream. There is not much we can do but emitting an
- error messages. */
- char numbuf[3 * sizeof (pid_t)];
- char *nump;
- char *endp = __stpcpy (failbuf, "out of memory [");
- pid_t pid = __getpid ();
-
- nump = numbuf + sizeof (numbuf);
- /* The PID can never be zero. */
- do
- *--nump = '0' + pid % 10;
- while ((pid /= 10) != 0);
-
- endp = __mempcpy (endp, nump, (numbuf + sizeof (numbuf)) - nump);
- *endp++ = ']';
- *endp = '\0';
- buf = failbuf;
- bufsize = endp - failbuf;
- msgoff = 0;
- }
- else
+ if (f != NULL)
{
__fsetlocking (f, FSETLOCKING_BYCALLER);
- fprintf (f, "<%d>", pri);
- now = time_now ();
- f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
- f->_IO_write_end - f->_IO_write_ptr,
- "%h %e %T ",
- __localtime_r (&now, &now_tm),
- _nl_C_locobj_ptr);
- msgoff = ftell (f);
- if (LogTag == NULL)
- LogTag = __progname;
- if (LogTag != NULL)
- __fputs_unlocked (LogTag, f);
- if (LogStat & LOG_PID)
- fprintf (f, "[%d]", (int) __getpid ());
- if (LogTag != NULL)
- {
- __putc_unlocked (':', f);
- __putc_unlocked (' ', f);
- }
-
+ /* "%h %e %H:%M:%S " */
+ char timebuf[3+1 /* "%h " */
+ + 2+1 /* "%e " */
+ + 2+1 + 2+1 + 2+1 + 1 /* "%T " */];
+ time_t now = time_now ();
+ struct tm now_tm;
+ __localtime_r (&now, &now_tm);
+ __strftime_l (timebuf, sizeof (timebuf), "%h %e %T ", &now_tm,
+ _nl_C_locobj_ptr);
+
+ pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
+
+ fprintf (f, "<%d>%s %n%s%s%.0d%s: ", pri, timebuf, &msgoff,
+ LogTag == NULL ? __progname : LogTag,
+ pid != 0 ? "[" : "", pid, pid != 0 ? "]" : "");
/* Restore errno for %m format. */
__set_errno (saved_errno);
@@ -233,26 +183,19 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
/* Tell the cancellation handler to free this buffer. */
clarg.buf = buf;
}
+ else
+ {
+ /* We cannot get a stream. There is not much we can do but emitting an
+ error messages. */
+ bufsize = __snprintf (failbuf, sizeof failbuf, "out of memory[%d]",
+ __getpid ());
+ buf = failbuf;
+ }
/* Output to stderr if requested. */
if (LogStat & LOG_PERROR)
- {
- struct iovec iov[2];
- struct iovec *v = iov;
-
- v->iov_base = buf + msgoff;
- v->iov_len = bufsize - msgoff;
- /* Append a newline if necessary. */
- if (buf[bufsize - 1] != '\n')
- {
- ++v;
- v->iov_base = (char *) "\n";
- v->iov_len = 1;
- }
-
- /* writev is a cancellation point. */
- __writev (STDERR_FILENO, iov, v - iov + 1);
- }
+ __dprintf (STDERR_FILENO, "%s%s", buf + msgoff,
+ buf[bufsize - 1] != '\n' ? "\n" : "");
/* Get connected, output the message to the local logger. */
if (!connected)
@@ -281,6 +224,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
* Make sure the error reported is the one from the
* syslogd failure.
*/
+ int fd;
if (LogStat & LOG_CONS &&
(fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
| O_CLOEXEC, 0))