Page MenuHomeFreeBSD

Add capability for newsyslog to write RFC5424 compliant rotation message.
ClosedPublic

Authored by dab on Apr 3 2017, 3:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 4:48 PM
Unknown Object (File)
Fri, Nov 15, 7:18 PM
Unknown Object (File)
Wed, Nov 6, 10:50 AM
Unknown Object (File)
Oct 27 2024, 6:43 PM
Unknown Object (File)
Oct 23 2024, 11:43 AM
Unknown Object (File)
Oct 17 2024, 11:25 AM
Unknown Object (File)
Oct 12 2024, 8:36 PM
Unknown Object (File)
Oct 6 2024, 2:49 PM
Subscribers

Details

Summary

This modification adds the capability to newsyslog to write the
rotation message in a format that is compliant with RFC5424. This
capability is enabled on a per-log file basis through a new value of
the flags field in newsyslog.conf. This would be useful on systems
that use the RFC5424 format for log files so that the rotation message
format matches the format of the other log messages. There has been
recent mention of adding an RFC5424 compliant mode to syslogd and
there are alternative system log daemons (such as rsyslogd) that
already have the capability to use that format.

The flag value chosen, "T", is arbitrary. It stems from the author's
conception that the most significant difference in the two log formats
is the timestamp used on each line of the log file. However, that
certainly is not the only difference. Suggestions for better
alternative flag values are welcome.

It should be noted that an alternative to using the flags field to
specify RFC5424 format was considered; namely, to have a newsyslog
command line option to specify the mode. That alternative was rejected
because using the flags field approach is more flexible, allowing
multiple formats of log files to be used on a single system.

The man page for newsyslog.conf is updated accordingly. While here, a
couple manlint and igor warnings in the man pages for newsyslog and
newsyslog.conf are also fixed.

Test Plan
  1. Create a newsyslog.conf file that includes a log file (e.g., rfc5424.log) that specifies the RFC5424 format flag and a log file (e.g., legacy.log) that does not specify the RFC5424 format flag.
  1. Invoke newsyslog to rotate the log files (either using the force flag or appropriate time or size parameters in newsyslog.conf).
  1. Examine the log files to ensure that the format is in RFC5424 format for that log file so configured and that the format is in the traditional (RFC3164) format for the other log file.

This plan has been implemented as a revision to the tests/legacy_test.sh script.

Diff Detail

Event Timeline

vangyzen added inline comments.
usr.sbin/newsyslog/ptimes.c
490

style(9) says this declaration should be squeezed into two lines.

502

style(9) says the usage of optional braces should be consistent within a function. Since line 542 has them, add them here and line 504.

531

There is extra space in tz_mins =.

This revision is now accepted and ready to land.Apr 3 2017, 5:05 PM
usr.sbin/newsyslog/newsyslog.8
128 ↗(On Diff #26971)

This mandoc change should be committed in a separate cleanup commit.

usr.sbin/newsyslog/newsyslog.c
1313

I would commit the typo fix separately in a cleanup commit.

2111

I would commit the typo fix separately in a cleanup commit.

2271

Unnecessary space between (int) and getpid()

2274

Really? The pid isn't in square brackets in the RFC-5424 format?

usr.sbin/newsyslog/newsyslog.conf.5
245

I would separate content changes like this from the overall change.

399–401

Interesting! I didn't know about those macros -- thanks :)

usr.sbin/newsyslog/ptimes.c
490

Yes. The correct way would be something like this:

char *
ptimeget_ctime_rfc5424(const struct ptime_data *ptime,
    char *timebuf, size_t bufsize)
{
usr.sbin/newsyslog/ptimes.c
541

I would use nilvalue instead of NILVALUE for consistency.

usr.sbin/newsyslog/ptimes.c
541

NILVALUE is the way it is referenced in the RFC, which is why I used that in commentary. Obviously, I also have the lower-case variable meaning the same thing. I suppose I could have avoided the case issue by using:

#define NILVALUE "-"

usr.sbin/newsyslog/newsyslog.c
2274

No. The RFC-5424 format defines a distinct field for the pid, which it calls PROCID. It also doesn't actually have to be the pid: the RFC states the PROCID has "no interoperable meaning, except that a change in the value indicates there has been a discontinuity in syslog reporting".

usr.sbin/newsyslog/newsyslog.c
2271

The space between (int) and getpid() came from copying the original lines (2285-2293). I presume you would like me to get rid of the extra space on all lines 2268-2293?

usr.sbin/newsyslog/newsyslog.8
128 ↗(On Diff #26971)

While I realize one can get carried away with drive-by changes, it seems to me that forcing all minor changes like this (and several of the others) to separate commits is a dis-incentive to fixing little problems found while working in the source and leads to lingering instances of such problems. However, that's just IMHO. I'll back out this change and some of the other manlint, igor, and typo fixes as you suggest.

Separating the commits is good code hygiene. While the project doesn't always do it, it's an important skill for our contributors to have (knowing how to do the splits as well as using tools that make that easy). Otherwise, we tend to wind up with long, meandering commits that are hard to bisect when there's the inevitable problem.

While these may or may not be worth doing as a separate commit, it's a big leap to call it demotivating.

usr.sbin/newsyslog/newsyslog.conf.5
399–401

I was quite happy to stumble across them myself!

dab edited edge metadata.

Update to respond to review comments.

This revision now requires review to proceed.Apr 3 2017, 9:03 PM
This revision is now accepted and ready to land.Apr 3 2017, 9:10 PM
dab edited edge metadata.
  • Rebase on HEAD
  • Add a test for RFC5424 functionality.
This revision now requires review to proceed.May 19 2017, 4:13 PM
  • Revise RFC5424 test so it both verifies that the RFC-5424 timestamp feature works and that the legacy timestamp format also continues to work.
This revision is now accepted and ready to land.May 19 2017, 5:34 PM
dab edited edge metadata.
  • Revise the test script to ensure that legacy logs do not simply contain no RFC-5424 formatted lines but that they /do/ have RFC-3164 formatted lines.

(Sorry for the churn; I think I'm done now)

This revision now requires review to proceed.May 19 2017, 5:46 PM
This revision is now accepted and ready to land.May 19 2017, 5:48 PM
ngie added inline comments.
usr.sbin/newsyslog/ptimes.c
531

This comment is still valid.

541

Ok -- I wasn't aware of that. Could the variable be named NILVALUE then, for consistency, along with a comment?

usr.sbin/newsyslog/tests/legacy_test.sh
114 ↗(On Diff #28578)

This could use grep -cE instead of the grep -E | wc -l | cut gymnastics

128 ↗(On Diff #28578)

Same comment here as above.

usr.sbin/newsyslog/newsyslog.c
2287

Spurious space after (int) cast here and on the next few lines.

Why are you casting it to (int) though? It's __int32_t per sys/_types.h .

dab marked 12 inline comments as done.May 19 2017, 6:28 PM
dab added inline comments.
usr.sbin/newsyslog/newsyslog.c
2287

The cast (and following space) are from the existing code. I'll eliminate.

dab edited edge metadata.
dab marked an inline comment as done.
  • Cleanup suggested by code review.
This revision now requires review to proceed.May 19 2017, 6:28 PM
This revision is now accepted and ready to land.May 19 2017, 6:37 PM
dab marked 3 inline comments as done.May 19 2017, 6:41 PM
This revision was automatically updated to reflect the committed changes.