This patch was sent to me by Antti Kettunen <ank@iki.fi>
Details
Details
- Reviewers
jilles mjg feld - Group Reviewers
manpages - Commits
- rS307769: daemon: Allow logging daemon stdout/stderr to file or syslog.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 66 ↗ | (On Diff #20591) | Use the serial comma: facility, priority, and tag, respectively. |
| 68 ↗ | (On Diff #20591) | Why include the complexity of the and/or statement? That is not really relevant to this flag, which controls where the output goes, not which output. So maybe just Redirect output from the daemonized process to |
| 73 ↗ | (On Diff #20591) | This is not a path (.Pa), but also seems unnecessarily redundant. The sentence can end after (0) and this part can be deleted: or neither (0). |
| 74 ↗ | (On Diff #20591) | Please start new sentences on new lines. |
| 119 ↗ | (On Diff #20591) | Please start new sentences on new lines. |
| 126 ↗ | (On Diff #20591) | Again, this is not a path, and seems redundant. Just Log the output of the daemonized process to syslog. |
| 128 ↗ | (On Diff #20591) | Please avoid "the following", especially when the subject is immediately following: These priorities are accepted: emerg, alert, crit, err, warning, |
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 75 ↗ | (On Diff #20591) | The default is 3, since neither the shell nor daemon(8) normally recognize things like STDOUT_FILENO. |
| 204 ↗ | (On Diff #20591) | appending |
| usr.sbin/daemon/daemon.c | ||
| 51 ↗ | (On Diff #20591) | This is 4 more than 2**13. Is there a reason for this? |
| 67 ↗ | (On Diff #20591) | POSIX is clear that variables to communicate with asynchronous signal handlers should be volatile sig_atomic_t, not just sig_atomic_t. However, if you restore the masking, normal variables will do. |
| 106 ↗ | (On Diff #20591) | If you #define SYSLOG_NAMES, <syslog.h> provides similar tables so you don't need to duplicate code. |
| 244 ↗ | (On Diff #20591) | Please keep the spaces here. |
| 284 ↗ | (On Diff #20591) | This can be made to work in the (admittedly unlikely) case that fd 1 and 2 are not open by closing pfd[0] before any dup2 and only closing pfd[1] if it is not equal to something it is to be dup2'ed to. |
| 325 ↗ | (On Diff #20591) | This wakes up every second for no reason, which the old version did not do. Keeping the signal masking from the old version and unmasking around async-signal safe read() allows using sigsuspend() or sigwait() here. |
| 430 ↗ | (On Diff #20591) | Perhaps, output should be collected into lines here. |
| 435 ↗ | (On Diff #20591) | Hmm, making outfile a file descriptor would lead to slightly less code. |
| 466 ↗ | (On Diff #20591) | The old code is not confused by unknown child processes but leaves them as zombies. I think the change to waitpid(-1 is good but successful return values other than pid should be ignored. Example: (sleep 10 & exec daemon ...) |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 51 ↗ | (On Diff #20591) | Memory failure, corrected to an exact exponent. |
| 106 ↗ | (On Diff #20591) | Thanks, noted. I gave a look for this, because it felt like replication, but clearly didn't look hard enough. |
| 284 ↗ | (On Diff #20591) | Noted, hopefully correctly. |
| 325 ↗ | (On Diff #20591) | Noted, hopefully correctly. |
| 430 ↗ | (On Diff #20591) | Noted as well. I'd appreciate a careful review of the coming changes which implement this. |
| 466 ↗ | (On Diff #20591) | Noted. Kept this at waitpid(-1, ...) but now reaping until pid or error is seen. |
Comment Actions
Addressed the review comments. Attempted to treat the documentation mishaps, signal handling, child-waiting, and dividing logging output into carriage return-separated lines. The last should be most beneficial for syslog purposes.
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 471 ↗ | (On Diff #20686) | Needs bytes_read to be zeroed. |
Comment Actions
When updating the patch, I probably missed something so apparently linting and the compile test didn't run. Perhaps I should've manually added the source repository as rS FreeBSD src repository when submitting the updated revision and/or done something else?
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 485 ↗ | (On Diff #20686) | Would make much more sense to assert(len < LBUF_SIZE);. It's unlikely that the buffer size would exceed INT_MAX. |
| 519 ↗ | (On Diff #20686) | Probably we need to be extra careful here, because pid is -1 before forking. We wouldn't want to do kill(-1, SIGTERM)... |
Comment Actions
I think this is quite nice.
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 302 ↗ | (On Diff #20686) | I think this can certainly be improved (e.g. optionally using procctl(PROC_REAP_ACQUIRE) and friends) but that should be in a later commit. |
| 395 ↗ | (On Diff #20686) | Common style is strcmp(...) == 0. |
| 422 ↗ | (On Diff #20686) | The latter four arguments pertain to where logs are going, and could be grouped into a structure to avoid dragging them along a few times. |
| 426 ↗ | (On Diff #20686) | An initializer is unnecessary here, and { 0 } might (wrongly) cause compiler warnings. |
| 463 ↗ | (On Diff #20686) | It would be wrong to retry after unknown errors, since the error would likely occur again immediately. |
| 504 ↗ | (On Diff #20686) | If the signal handler only switches between two different code paths, why not simply use one signal handler per signal? :) |
| 519 ↗ | (On Diff #20686) | Perhaps leave signals blocked during that time so there is never any question of races with pid. |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 302 ↗ | (On Diff #20686) | Noted, thanks. |
Comment Actions
Fixes for the latest round of review
- Split signal handler
- Block SIGTERM until we have forked - currently we don't reblock it after the first fork
- Some style(9) issues
- Deliver logging parameters inside of a struct
- assert more sensibly
- Zero the read buffer counter when EOF flushing
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 65 ↗ | (On Diff #20591) | But the second sentence is still on the same line... |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 204 ↗ | (On Diff #20720) | The two handlers do indeed interfere, since calling waitpid() from the SIGCHLD handler invalidates the pid used by the SIGTERM handler (you'll get [ESRCH] or kill an unrelated process). The SIGTERM handler doesn't check for this yet (child_gone, probably, together with blocking SIGTERM before goto restart). Note that this kind of PID reuse bug is inherent to the concept of pidfiles, but in this case it can be avoided. |
| 296 ↗ | (On Diff #20720) | Looks good. I don't care about the case where someone wants to pass through a closed fd 1 log what's written to fd 2 (in that case fd 1 will also be a copy of the pipe). |
| usr.sbin/daemon/daemon.8 | ||
|---|---|---|
| 65 ↗ | (On Diff #20591) | Thank you for noticing, I fumbled. |
| usr.sbin/daemon/daemon.c | ||
| 204 ↗ | (On Diff #20720) | Thank you again for the keen eye. I now explicitly block the opposites, and SIGTERM is blocked after leaving the handling loop. I suppose this still leaves a small window for a short-lived fork if we happen to block a SIGTERM just before the restart. |
Comment Actions
Adjusted the previous patch according to the received comments:
- Fixed a man page issue
- Explicitly block SIGTERM when handling SIGCHLD and vice versa
- Block SIGTERM harder & observe child_gone to avoid signaling an already departed/nonexistent child
Additionally the following was done:
- terminate = 1 is now done unconditionally when handling SIGTERM
- Some clarifications to the man page to more coherently explain the meaning of and interplay of different options
- Minor style issues were adjusted
- Removed the remaining XXX comments after addressing the concerns
Comment Actions
My apologies for the late addition. One more mistake fixed:
- Do not leak the log file descriptor to child by setting FD_CLOEXEC
Comment Actions
Is this version now accepted? If so, can one of the technical reviewers commit it? If one of the reviewers approves I could commit it as well.
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 174–175 ↗ | (On Diff #21238) | Oops, this is quite important. It can be done more easily using the open flag O_CLOEXEC. |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 173 ↗ | (On Diff #21238) | Why does this exit here as opposed to doing goto exit? Should the file be unlinked on failure of daemon(3)? |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 173 ↗ | (On Diff #21238) | Please not the above. |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 173 ↗ | (On Diff #21238) | If the fcntl() call failed, then we would have left a potentially empty file hanging around. Perhaps the users wouldn't expect this. There might have been a scenario, where we succesfully open() a log file for appending with some previous contents or append to a file, where something else is also writing. In these cases at least, I would definitely avoid unlinking. I suppose by handling the FD leak prevention through open(... O_CLOEXEC) we don't have to worry about this, but please correct me if I'm wrong. |
| 174–175 ↗ | (On Diff #21238) | Yep, open() is easier and saves us the trouble of separately handling the possible (unlikely?) fcntl() failure. |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 173 ↗ | (On Diff #21238) | The unlink was a minor point. I gave an example when it matters; daemon(3) call failing. The actual point was the call to err as opposed to doing goto exit, which fails to clean up pidfiles. |
| usr.sbin/daemon/daemon.c | ||
|---|---|---|
| 173 ↗ | (On Diff #21238) | Ah, that's exactly right. It would be better to attempt the log open() before opening the pid files. |