The default system log rotation mechanism (newsyslog(8)) requires ability to send signal to a daemon in order to properly complete rotation of the logs in an "atomic" manner without having to making a copy and truncating original file. Unfortunately our built-in mechanism to convert "dumb" programs into daemons has no way to handle this rotation properly. This change adds this ability, to be enabled by supplying -H option in addition to the -o option.
Details
Details
#!/bin/sh set -e rm -f test.out* ./daemon -H -o test.out -P test.pid sh -c "echo 1; sleep 5; echo 2" sleep 2 mv test.out test.out1 kill -HUP `cat test.pid` sleep 10 if [ "`cat test.out`" = "2" -a "`cat test.out1`" = "1" ] then echo Success else echo FAILURE false fi
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 33745
Event Timeline
Comment Actions
Reset do_log_reopen earlier, so that we don't stuck in a loop calling this function repeatedly if error happens.
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 89 | Instead of "later on," we can be more specific and say "after a SIGHUP." | |
| usr.sbin/daemon/daemon.c | ||
| 182 | What if outfn == NULL but log_reopen == 1? Should it be allowed? | |
| 585 | It is more idiomatic to declare signo with __unused. | |
| 595 | We are duplicating the open() call from main() here. I think we should have a subroutine to do it, instead of duplicating flags, mode, etc. | |
| 599 | I believe syslogd unconditionally closes log files upon SIGHUP. Here, we keep the old fd open if open() fails. Is there a reason for the difference? | |
Comment Actions
o (void)xyz -> xyz __unused; o De-duplicate open(2) call; o close old log on SIGHUP regardless of the outcome of the new open() call to match syslogd(8) behaviour. o Small manpage wording improvements.
Suggested by: @markj
Comment Actions
Thank for a review Mark! 4 out of 5 suggestions have been incorporated, please check.
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 182 |
Well, I always feel a bit cautious of adding too much input parameters validation. In reality, pretty much any utility with more than few input parameters will have too many permutations to check, so the rule of thumb I apply in those cases is "whether the combination produces some unexpected/unwanted behavior or just a NOP". If it's a NOP then I tend to leave it be. Otherwise parameter validation code has a potential in itself to become liability and source of errors. In this case, SIGHUP handler is only armed if we have a file to rotate, so having -H without -o will be a NOP, just as one might expect it to be. Famous example perhaps is something like "mov rX,rX" instruction that is present in 99.9% of CPUs. However that all have been said I can add validation if you feel strongly about it. | |
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 74 | and re-open it when signal SIGHUP is received, for interoperability with .Xr newsyslog 1 . | |
Comment Actions
Looks ok to me, my comments are just nitpicking.
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 182 | That seems reasonable, especially since this program apparently does very little validation to begin with. I would add a sentence to the man page mentioning this though, like "If .Fa o is not specified, this flag is ignored." | |
| 593 | Style: missing parens around the return value. Also, the blank line at the beginning of functions with no local vars is now optional. | |
Comment Actions
Thanks, Mark! I'll add manpage note. With regards to style(9), I still kinda like blank line rule (old habits die hard?), with regards to the return statement yes, I am aware of that, but the file uses form without parens in all other places and I did not feel like converting everything or introducing inconsistency.
$ grep 'return ' usr.sbin/daemon/daemon.c return cp->c_val; return -1; return 1; return 1; return 1; return 0; return 0; return open(outfn, O_CREAT | O_WRONLY | O_APPEND | O_CLOEXEC, 0600);