[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug-inetutils] [patch] syslogd verbose patch to include facility.pr
From: |
Richard Dawe |
Subject: |
Re: [bug-inetutils] [patch] syslogd verbose patch to include facility.priority in message |
Date: |
Fri, 22 Dec 2006 22:37:21 +0000 |
User-agent: |
Thunderbird 1.5.0.9 (X11/20061219) |
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