bug-inetutils
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]