Re: [bug-inetutils] [patch] syslogd verbose patch to include facility.priority in message

[Dirk Jagdmann]

Hello Rich,

thank you for your comments on my patch.

> > I have made a patch to syslogd which will include the facility and
> > priority of each message in front of the message. This mode is activated
> > by the "-v" or "--verbose" command line option. To distinguish and
> > detect the facility and priorities I wrapped them in curly braces.
>
> I think "verbose" may be the wrong name for this option. I would expect
> verbose mode to make the syslogd daemon print out more information about
> what it is doing, not in the log messages.

I don't insinst on the option beeing called "verbose", I just took the 
name the FreeBSD syslogd is using for a similar functionality. But I can 
live with a different name too.

> Why not define this buffer in the block where it is used. E.g.: ...

The definition of the "msg_" buffer is at the right place, because it's 
scope does not end with the end of the newly introduced if(Verbose) 
block. If the block is executed a pointer is set at the last lines which 
must remain valid until the end of the logmsg() function.

> Some explanation of where the magic constant "16" comes from would be
> nice. You're assuming that textpri() will always return a string of 20
> bytes or less.

I assume that the textpri() function will return at most 16 bytes, 
because the names of the standard facilities+priorites+1 dot will not 
exceed 16 characters. Of course you never know what people define in 
their header files, but I have to decide on a width for alignment, so I 
stick to 16 characters.

> You're writing an arbitrary string into a buffer, without restricting
> the length of data written. This is generally a bad idea. snprintf()
> would be better.

No problem. I generally like snprintf(), however had doubts about a 
performance impact in such a heavily used function inside syslog. Anyway 
I have now made a second version based on your suggestions which should 
better protect against failed functions. It is attachted to this email.

--
---> Dirk Jagdmann ^ doj / cubic
----> http://cubic.org/~doj
-----> http://llg.cubic.org

Index: syslogd/syslogd.c
===================================================================
--- syslogd.orig/syslogd.c      2006-12-18 23:40:35.000000000 +0100
+++ syslogd/syslogd.c   2006-12-18 23:56:04.000000000 +0100
@@ -258,6 +258,7 @@
 time_t  now;                    /* Time use for mark and forward supending.  */
 int force_sync;                 /* GNU/Linux behaviour to sync on every line.
                                   This off by default. Set to 1 to enable.  */
+int Verbose = 0;               /* If greater 0 print <facility.priority> in 
messages. */
 
 char *program_name;
 extern int waitdaemon (int nochdir, int noclose, int maxwait);
@@ -318,6 +319,7 @@
   -s DOMAINLIST      List of domains which should be stripped from the FQDN\n\
                      of hosts before logging their name.\n\
       --help         Display this help and exit\n\
+  -v, --verbose      Prepend messages with <facility.priority>.\n\
   -V, --version      Output version information and exit");
 
       fprintf (stdout, "\nSubmit bug reports to %s.\n", PACKAGE_BUGREPORT);
@@ -325,7 +327,7 @@
   exit (err);
 }
 
-static const char *short_options = "a:dhf:Kl:m:np:rs:VS";
+static const char *short_options = "a:dhf:Kl:m:np:rs:vVS";
 static struct option long_options[] =
 {
   { "debug", no_argument, 0, 'd' },
@@ -341,6 +343,7 @@
   { "no-unixaf", no_argument, 0, 'U' },
   { "sync", no_argument, 0, 'S' },
   { "help", no_argument, 0, '&' },
+  { "verbose", no_argument, 0, 'v' },
   { "version", no_argument, 0, 'V' },
 #if 0 /* Not sure about the long name. Maybe move into conffile even.  */
   { "", required_argument, 0, 'a' },
@@ -416,6 +419,10 @@
          StripDomains = crunch_list (StripDomains, optarg);
          break;
 
+       case 'v':
+         Verbose++;
+         break;
+
        case 'P': /* Override pidfile.  */
          PidFile = optarg;
          break;
@@ -1042,6 +1049,7 @@
   int omask;
 #endif
 
+  char msg_[MAXLINE+22];
   const char *timestamp;
 
   dbg_printf ("(logmsg): %s (%d), flags %x, from %s, msg %s\n",
@@ -1070,6 +1078,37 @@
       timestamp = msg;
       msg += 16;
       msglen -= 16;
+    }
+
+  /* prepend facility.priority */
+  if(Verbose && msg[0]!='{')
+    {
+      char *p=msg_;
+      int len, plen=sizeof(msg_);
+
+      /* print {fac.pri} */
+      len=snprintf(p, plen, "{%s}", textpri(pri));
+      if(len>0)
+       {
+         p+=len;
+         plen-=len;
+
+         /* pad to 18 chars: 18 = (7 chars facility) + (1 char dot) + (8 chars 
priority) + (2 chars curly braces) */
+         len=18-len;
+         while(len > 0 && plen > 0)
+           {
+             *p++=' ';
+             --len;
+             --plen;
+           }
+
+         /* append message */
+         snprintf(p, plen, "%s", msg);
+
+         /* put new msg in place of old message */
+         msg=msg_;
+         msglen=strlen(msg);
+       }
     }
 
   /* Extract facility and priority level.  */
Index: syslogd/syslogd.8
===================================================================
--- syslogd.orig/syslogd.8      2006-12-18 23:40:35.000000000 +0100
+++ syslogd/syslogd.8   2006-12-18 23:42:02.000000000 +0100
@@ -46,6 +46,7 @@
 .Op Fl p Ar log_socket
 .Op Fl r
 .Op Fl s Ar domain_list
+.Op Fl v
 .Op Fl -no-klog
 .Op Fl -no-unixaf
 .Op Fl -no-forward
@@ -92,6 +93,9 @@
 .It Fl s
 A colon-seperated list of domainnames which should be stripped from
 the FQDNs of hosts when logging.
+.It \fB-v\fR, \fB--verbose\fR
+Prepend messages with the facility and priority string enclosed in curly
+braces.
 .It Fl -no-klog
 Do not listen to the kernel log device. This is only supported on
 systems which define a kernel log device, on all others this is already