Page MenuHomeFreeBSD

S"timefmt" flag to newsyslog(8)
AcceptedPublic

Authored by hrs on Aug 27 2019, 6:33 PM.

Details

Reviewers
bcr
jilles
Group Reviewers
manpages
Summary

This patch adds S"timefmt" flag to newsyslog(8). This allows to specify
a time format used in filenames of the rotated log files instead of
sequential ones on a per-file basis.

Test Plan

The following config file entry will trim /var/log/a.log and create
/var/log/a.log-20190820T032934.bz2, for example:

/var/log/a.log 644 2 1000 * JS"%Y%m%dT%H%M%S"C

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 26149
Build 24666: arc lint + arc unit

Event Timeline

hrs created this revision.Aug 27 2019, 6:33 PM
bcr added a subscriber: bcr.Aug 27 2019, 7:04 PM

A few minor additions to the man page.
I like this change, thanks for implementing it!

usr.sbin/newsyslog/newsyslog.8
274–279

s/default each/default, each/

282–287

Same here as above.

usr.sbin/newsyslog/newsyslog.conf.5
326

s/to/to the/

hrs updated this revision to Diff 61369.Aug 27 2019, 7:19 PM

Updated to include bcr's suggestions and fix a missing length check.

bcr accepted this revision.Aug 27 2019, 7:43 PM

Good to go!

This revision is now accepted and ready to land.Aug 27 2019, 7:43 PM
jilles added a subscriber: jilles.Aug 27 2019, 9:21 PM
jilles added inline comments.
usr.sbin/newsyslog/newsyslog.c
171

It would be more consistent with the rest of the values here to create a separate allocation for the string (char *suffix). This would also avoid the need to depend on the maximum path length, which I like to avoid if it is easy, since there are situations (not here) where MAXPATHLEN is too short.

1316

q cannot be NULL here and in the below new lines, particularly because it has just been incremented.

1317

Allowing ' as well as " (undocumented) seems unnecessary extra complexity, especially because a ' can end a string starting with " and vice versa.

1332

Suggest memcpy, or overwriting the terminating " with a '\0' (as occurs earlier in this function) and strdup (combined with my above remark about the type of struct conf_entry.suffix). Even though it is correct here due to the above check, the dstsize parameter to strlcpy should be the destination buffer's size; it should not be abused to truncate a string. Note that strlcpy calls strlen on src.

1759

I can see some benefit in a hard-coded maximum length here.

hrs updated this revision to Diff 61383.Aug 27 2019, 11:24 PM

Update based on jilles's comments:

  • Dynamic memory allocation of suffix.
  • Fix a bug in NULL check of q.
  • Use memcpy() to get a substring.
  • Accept ["] only, not ['].
This revision now requires review to proceed.Aug 27 2019, 11:24 PM
hrs added inline comments.Aug 27 2019, 11:32 PM
usr.sbin/newsyslog/newsyslog.c
171

I think the (theoretical) maximum length of the suffix is limited to MAXPATHLEN because it is eventually used as a filename. Can you elaborate the case where MAXPATHLEN is too short?

1759

datetimestr is an expanded string from the suffix and the total length is also shorter than MAXPATHLEN. Why is a 30-byte array specified in a numeric value better?

jilles accepted this revision as: jilles.Aug 28 2019, 10:05 AM
jilles added inline comments.
usr.sbin/newsyslog/newsyslog.c
171

Our value of 1024 for MAXPATHLEN is less than Linux's value of 4096 and therefore people sometimes do things where our value is too low. As I said, that's not an issue in this particular case.

1759

I considered MAXPATHLEN hard-coded too, but in any case it should be long enough for a date and time.

This revision is now accepted and ready to land.Aug 28 2019, 10:05 AM