[v2,4/7] misc: syslog: Simplify implementation

Message ID 20220218142322.432304-5-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Use RFC5424 for syslog |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Netto Feb. 18, 2022, 2:23 p.m. UTC
  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

Paul Eggert Feb. 19, 2022, 3:15 a.m. UTC | #1
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?
  
Adhemerval Zanella Netto Feb. 21, 2022, 1:15 p.m. UTC | #2
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?
  
Andreas Schwab Feb. 21, 2022, 2:06 p.m. UTC | #3
On Feb 18 2022, Paul Eggert wrote:

> Also, the old code crashed when __localtime_r returns NULL

How can that happen?
  
Paul Eggert Feb. 21, 2022, 10:23 p.m. UTC | #4
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.
  
Paul Eggert Feb. 21, 2022, 10:25 p.m. UTC | #5
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.
  

Patch

diff --git a/misc/syslog.c b/misc/syslog.c
index 07a6938bff..e46cf1103b 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -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))