Miscelaneous fixes and cleanups (respin of #11) #18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Forcing the CMAKE_BUILD_TYPE with set() makes it difficult for the user to set a different build type (e.g. MinSizeRel). Leave it at the default.
We either take a directory and search it for all relevant files by extension, or we take a filename that resolves to a regular file. The codepaths to search for a default file inside a directory were unreachable, remove them.
We must use json_object_put() on the root of the json_object tree after we have rendered the output, otherwise we leak the entire json_object tree.
* Remove whitespace at the end of lines. * Replace all-whitespace lines with an empty line. The correctness of this patch can be trivially checked using "git diff --ignore-all-space" This cleanup made it MUCH easier to split independent changes into separate branches.
Do not load hidden files (those starting with '.') when iterating over directories. This follows the least-surprise principle.
The compiler knows foo(void) takes no parameters and will warn accordingly, but it cannot do that for K&R-style foo() function declarations.
lmapd_killalli() is not used outside of its source file, nor referenced in a header file. Make it static.
The correct constants for action.state are LMAP_ACTION_STATE_*, not LMAP_SCHEDULE_STATE_*. Fortunately, these constants are numerically equivalent, so this did not result in any runtime bug. This issue propagated to json-io.c when it was expanted to match capabilities with xml-io.c, and also needs to be fixed there.
|
Supersedes #11 |
There isn't a flock() call anywhere in the whole lmapd.
Ignore illegal PIDs in a pidfile (negative or zero), and check if a process with the PID read from the pidfile actually exists before considering it valid. This simple check can't handle the case when PID rollover (or a reboot on a system where pidfiles are persistent) caused a process to have the same PID as the PID in the stale pidfile, but it is far better than no check at all. Without this, lmapd will refuse to restart (until something deletes the stale pidfile) if it ever gets killed by a signal (e.g. SIGBUS due to a NULL pointer derreference). The simple check suffices to avoid this as long as process churn is not so high as to cause PID rollover, and system misconfiguration does not cause undue persistence of stale pidfiles across reboots. It also improves "lmapctl running" a bit, subject to the same caveats described above. A proper fix requires a full rework of the pidfile support, and likely requires the use of flock() and fcntl(FD_CLOEXEC) since the pidfile must be kept open and locked by the main [possibly daemonized] lmapd process *only*, and closed in all other threads/children. As for lmapctl, a proper fix would be to refactor its communication with lmapd to use UNIX sockets instead of signal-driven update of data files. Again, that'd be a very large change.
|
Force pushed to fix an incorrect version of the "stale pidfile" enhancement patch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a respin of #11, which I closed by mistake by an incorrect branch delection.
Functional changes:
"Cosmetic" changes:
"Source correctness" changes (no runtime changes):