[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: |
Sat, 16 Dec 2006 20:15:58 +0000 |
User-agent: |
Thunderbird 1.5.0.8 (X11/20061107) |
Hello.
I have a few comments about this patch.
Dirk Jagdmann wrote:
[snip]
> 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.
Now some comments about the code:
[snip]
> Index: syslogd/syslogd.c
> ===================================================================
> --- syslogd.orig/syslogd.c 2006-12-15 21:18:15.000000000 +0100
> +++ syslogd/syslogd.c 2006-12-15 23:51:20.000000000 +0100
[snip]
> @@ -1042,6 +1049,7 @@
> int omask;
> #endif
>
> + char msg_[MAXLINE+22];
Why not define this buffer in the block where it is used. E.g.: ...
> const char *timestamp;
>
> dbg_printf ("(logmsg): %s (%d), flags %x, from %s, msg %s\n",
> @@ -1070,6 +1078,22 @@
> timestamp = msg;
> msg += 16;
> msglen -= 16;
> + }
> +
> + /* prepend facility.priority */
> + if(Verbose && msg[0]!='{')
> + {
...define msg_ here.
> + char *p=msg_, *tp=textpri(pri);
> + int padlen=16-strlen(tp);
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.
> + /* print fac.pri */
> + p+=sprintf(p, "{%s}", tp);
What happens if sprintf() fails?
> + /* pad to 16 chars */
> + while(padlen > 0)
> + *p++=' ', --padlen;
> + /* append message */
> + strcpy(p, msg);
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.
> + 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