Add a new option (-H) to daemon(8) to catch SIGHUP and re-open output_file file when received.

[sobomax]

Summary

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.

Test Plan

#!/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

[sobomax]

Use signal handler in a safe way.

[sobomax]

Reset do_log_reopen earlier, so that we don't stuck in a loop calling this function repeatedly if error happens.

[markj]

usr.sbin/daemon/daemon.8

89 ↗

(On Diff #77398)

Instead of "later on," we can be more specific and say "after a SIGHUP."

usr.sbin/daemon/daemon.c

182 ↗

(On Diff #77398)

What if outfn == NULL but log_reopen == 1? Should it be allowed?

584 ↗

(On Diff #77398)

It is more idiomatic to declare signo with __unused.

594 ↗

(On Diff #77398)

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.

598 ↗

(On Diff #77398)

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?

[sobomax]

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

[sobomax]

Thank for a review Mark! 4 out of 5 suggestions have been incorporated, please check.

usr.sbin/daemon/daemon.c

182 ↗

(On Diff #77398)

What if outfn == NULL but log_reopen == 1? Should it be allowed?

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.

[rpokala]

usr.sbin/daemon/daemon.8

74 ↗

(On Diff #77434)

and re-open it when signal SIGHUP is received, for interoperability with
.Xr newsyslog 1 .

[sobomax]

Clarify intended use case for the new option.

Suggested by: @rpokala

[sobomax]

Thanks Ravi for suggestion, it has been incorporated.

[markj]

Looks ok to me, my comments are just nitpicking.

usr.sbin/daemon/daemon.c

593 ↗

(On Diff #77458)

Style: missing parens around the return value.

Also, the blank line at the beginning of functions with no local vars is now optional.

182 ↗

(On Diff #77398)

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

[sobomax]

In D26526#590868, @markj wrote:

Looks ok to me, my comments are just nitpicking.

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