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 _______________________________________________ Bug-inetutils mailing list [email protected] http://lists.gnu.org/mailman/listinfo/bug-inetutils
