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

[Richard Dawe]

Hi Dirk,

Dirk Jagdmann wrote:
> Hello Rich,
> 
> thank you for your comments on my patch.

You're welcome. I hope I wasn't too critical!

>>> 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.

I see what you mean!

syslogd(8) from FreeBSD 6.1 (and 6.0) says:

"-v   Verbose logging.  If specified once, the numeric facility and
priority are logged with each locally-written message.  If specified
more than once, the names of the facility and priority are logged with
each locally-written message." (NB: edited for readability)

http://www.freebsd.org/cgi/man.cgi?query=syslogd&sektion=8&apropos=0&manpath=FreeBSD+6.1-RELEASE

So you've implemented what is equivalent to "syslogd -v -v" on *BSD.

Perhaps you could make your patch compatible with *BSD's behaviour ("-v"
= numeric, "-v -v" => names)?

Is your verbose format the same as *BSD's syslogd?

Out of interest, I took a look to see what other syslogd on Linux do.
syslogd from sysklogd on Fedora Core 6 has this behaviour:

"-v     Print version and exit."

syslogd from syslog-ng doesn't seem to have a -v option, but it has
flexible output with templates -- see
<http://www.campin.net/syslog-ng/faq.html#template>.

>> 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.

Good point. Perhaps it could have a different name -- "msg_" is a bit ugly.

I think it should also be a static buffer or a buffer that's allocated
once, since it's a ~1KB buffer. It would not be good for syslogd to run
out of memory in logmsg().

E.g.:

static const size_t verbosebuflen = MAXLINE + 22;
static char *verbosebuf = NULL;

if (Verbose)
  {
    verbosebuf = malloc(verbosebuflen);
    /* ...handle malloc failure here... */
  }

>> 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.

I usually put assumptions like that in a comment in the code.

Sergey/Alfred: What is the general practice with inetutils?

>> 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.
[snip]

Thanks for updating your patch!

I noticed that you've reformatted the text. I think you need to make a
couple more changes to be in the GNU style:

> +  if(Verbose && msg[0]!='{')

if (Verbose && msg[0] != '{')

> +      /* print {fac.pri} */
> +      len=snprintf(p, plen, "{%s}", textpri(pri));

len = snprintf (p, plen, "{%s}", textpri (pri));

Also, I think you can do everything in one snprintf() (including the
padding). E.g.:

static char pad[19];
size_t padlen;
const char *prefix = textpri (pri);

padlen = 18 - strlen (prefix);
if (padlen < 0)
  padlen = 0;
memset(pad, " ", padlen);
pad[padlen] = '\0';

int len
  = snprintf (verbosebuf, verbosebuflen, "{%s}%s%s", prefix, pad, msg);
/* ...handle snprintf failure here... */
msg = verbosebuf;

[snip]
> @@ -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.  */
[snip]

Bye, Rich =]

-- 
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]

"You just amass stuff while you are alive. It's like stuff washed up
 on a beach somewhere, and that somewhere is you." -- Damien Hirst