Add capability for newsyslog to write RFC5424 compliant rotation message.

[dab]

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.

2. Invoke newsyslog to rotate the log files (either using the force flag or appropriate time or size parameters in newsyslog.conf).

3. 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.

[vangyzen]

usr.sbin/newsyslog/ptimes.c

490 ↗

(On Diff #26971)

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

502 ↗

(On Diff #26971)

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 ↗

(On Diff #26971)

There is extra space in tz_mins =.

[ngie]

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 ↗

(On Diff #26971)

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

2111 ↗

(On Diff #26971)

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

2271 ↗

(On Diff #26971)

Unnecessary space between (int) and getpid()

2274 ↗

(On Diff #26971)

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

usr.sbin/newsyslog/newsyslog.conf.5

245 ↗

(On Diff #26971)

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

399–401 ↗

(On Diff #26971)

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

usr.sbin/newsyslog/ptimes.c

490 ↗

(On Diff #26971)

Yes. The correct way would be something like this:

char *
ptimeget_ctime_rfc5424(const struct ptime_data *ptime,
    char *timebuf, size_t bufsize)
{

[ngie]

usr.sbin/newsyslog/ptimes.c

541 ↗

(On Diff #26971)

I would use nilvalue instead of NILVALUE for consistency.

[dab]

usr.sbin/newsyslog/ptimes.c

541 ↗

(On Diff #26971)

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 "-"

[dab]

usr.sbin/newsyslog/newsyslog.c

2274 ↗

(On Diff #26971)

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".

[dab]

usr.sbin/newsyslog/newsyslog.c

2271 ↗

(On Diff #26971)

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?

[dab]

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.

[imp]

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.

[dab]

usr.sbin/newsyslog/newsyslog.conf.5

399–401 ↗

(On Diff #26971)

I was quite happy to stumble across them myself!

[dab]

Update to respond to review comments.

[dab]

• Rebase on HEAD
• Add a test for RFC5424 functionality.

[dab]

• Revise RFC5424 test so it both verifies that the RFC-5424 timestamp feature works and that the legacy timestamp format also continues to work.

[dab]

• 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)

[ngie]

usr.sbin/newsyslog/ptimes.c

531 ↗

(On Diff #26971)

This comment is still valid.

541 ↗

(On Diff #26971)

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.

[ngie]

usr.sbin/newsyslog/newsyslog.c

2296 ↗

(On Diff #28578)

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]

usr.sbin/newsyslog/newsyslog.c

2296 ↗

(On Diff #28578)

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

[dab]

• Cleanup suggested by code review.