From df9008bfc4aba3715044f88fe3ae9ee441b0143b Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 7 Sep 2022 11:46:46 +0200 Subject: [PATCH 1/4] StartUnixWorker(): watch forked child via waitpid(), not SIGCHLD handler Before: On SIGCHLD from the forked worker the umbrella process sets a failure flag. StartUnixWorker() recognises that and does waitpid(), failure message, etc.. On OpenBSD we can't tell the signal source, so we always set the failure flag. That's not how our IPC shall work, that breaks the IPC sooner or later. After: No SIGCHLD handling and no failure flag setting. Instead StartUnixWorker()'s wait loop uses waitpid(x,y,WNOHANG) to avoid false positives while watching the forked worker. --- lib/cli/daemoncommand.cpp | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/lib/cli/daemoncommand.cpp b/lib/cli/daemoncommand.cpp index fb1dd0289..f3c7d7895 100644 --- a/lib/cli/daemoncommand.cpp +++ b/lib/cli/daemoncommand.cpp @@ -381,13 +381,6 @@ static void UmbrellaSignalHandler(int num, siginfo_t *info, void*) l_CurrentlyStartingUnixWorkerState.store(UnixWorkerState::LoadedConfig); } break; - case SIGCHLD: - if (l_CurrentlyStartingUnixWorkerState.load() == UnixWorkerState::Pending - && (info->si_pid == 0 || info->si_pid == l_CurrentlyStartingUnixWorkerPid.load()) ) { - // The seamless worker currently being started by StartUnixWorker() failed - l_CurrentlyStartingUnixWorkerState.store(UnixWorkerState::Failed); - } - break; case SIGINT: case SIGTERM: // Someone requested our termination @@ -570,28 +563,21 @@ static pid_t StartUnixWorker(const std::vector& configs, bool close NotifyWatchdog(); #endif /* HAVE_SYSTEMD */ - switch (l_CurrentlyStartingUnixWorkerState.load()) { - case UnixWorkerState::LoadedConfig: - Log(LogNotice, "cli") - << "Worker process successfully loaded its config"; - break; - case UnixWorkerState::Failed: - Log(LogNotice, "cli") - << "Worker process couldn't load its config"; + if (waitpid(pid, nullptr, WNOHANG) > 0) { + Log(LogNotice, "cli") + << "Worker process couldn't load its config"; - while (waitpid(pid, nullptr, 0) == -1 && errno == EINTR) { -#ifdef HAVE_SYSTEMD - NotifyWatchdog(); -#endif /* HAVE_SYSTEMD */ - } - pid = -2; - break; - default: - Utility::Sleep(0.2); - continue; + pid = -2; + break; } - break; + if (l_CurrentlyStartingUnixWorkerState.load() == UnixWorkerState::LoadedConfig) { + Log(LogNotice, "cli") + << "Worker process successfully loaded its config"; + break; + } + + Utility::Sleep(0.2); } // Reset flags for the next time From 3de714489c063bf2b0b66f5ac1d2e2d523ed1afb Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 7 Sep 2022 12:08:33 +0200 Subject: [PATCH 2/4] Remove unused UnixWorkerState::Failed --- lib/cli/daemoncommand.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/cli/daemoncommand.cpp b/lib/cli/daemoncommand.cpp index f3c7d7895..ebd2e6d16 100644 --- a/lib/cli/daemoncommand.cpp +++ b/lib/cli/daemoncommand.cpp @@ -330,8 +330,7 @@ int RunWorker(const std::vector& configs, bool closeConsoleLog = fa enum class UnixWorkerState : uint_fast8_t { Pending, - LoadedConfig, - Failed + LoadedConfig }; // The signals to block temporarily in StartUnixWorker(). From 22bfcf9ac52b37437804906a1e26b0e00461572f Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 7 Sep 2022 12:12:09 +0200 Subject: [PATCH 3/4] icinga2 daemon: remove no-op SIGCHLD handling 1. Don't set a custom handler for SIGCHLD (in the umbrella process) as that handler doesn't actually handle SIGCHLD anymore 2. Don't reset the SIGCHLD handler (in the worker process) as there's nothing to reset anymore due to the above change 3. Don't block SIGCHLD across fork(2) as its handler doesn't change anymore due to the above changes --- lib/cli/daemoncommand.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/cli/daemoncommand.cpp b/lib/cli/daemoncommand.cpp index ebd2e6d16..b2abc18de 100644 --- a/lib/cli/daemoncommand.cpp +++ b/lib/cli/daemoncommand.cpp @@ -338,7 +338,6 @@ static const sigset_t l_UnixWorkerSignals = ([]() -> sigset_t { sigset_t s; (void)sigemptyset(&s); - (void)sigaddset(&s, SIGCHLD); (void)sigaddset(&s, SIGUSR1); (void)sigaddset(&s, SIGUSR2); (void)sigaddset(&s, SIGINT); @@ -475,7 +474,7 @@ static pid_t StartUnixWorker(const std::vector& configs, bool close } /* Block the signal handlers we'd like to change in the child process until we changed them. - * Block SIGUSR2 and SIGCHLD handlers until we've set l_CurrentlyStartingUnixWorkerPid. + * Block SIGUSR2 handler until we've set l_CurrentlyStartingUnixWorkerPid. */ (void)sigprocmask(SIG_BLOCK, &l_UnixWorkerSignals, nullptr); @@ -505,7 +504,6 @@ static pid_t StartUnixWorker(const std::vector& configs, bool close sa.sa_handler = SIG_DFL; - (void)sigaction(SIGCHLD, &sa, nullptr); (void)sigaction(SIGUSR1, &sa, nullptr); (void)sigaction(SIGHUP, &sa, nullptr); } @@ -719,7 +717,6 @@ int DaemonCommand::Run(const po::variables_map& vm, const std::vector Date: Fri, 7 Oct 2022 15:14:33 +0200 Subject: [PATCH 4/4] Replace two-variants enum with bool --- lib/cli/daemoncommand.cpp | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/lib/cli/daemoncommand.cpp b/lib/cli/daemoncommand.cpp index b2abc18de..a03976191 100644 --- a/lib/cli/daemoncommand.cpp +++ b/lib/cli/daemoncommand.cpp @@ -324,15 +324,6 @@ int RunWorker(const std::vector& configs, bool closeConsoleLog = fa } #ifndef _WIN32 -/** - * The possible states of a seamless worker being started by StartUnixWorker(). - */ -enum class UnixWorkerState : uint_fast8_t -{ - Pending, - LoadedConfig -}; - // The signals to block temporarily in StartUnixWorker(). static const sigset_t l_UnixWorkerSignals = ([]() -> sigset_t { sigset_t s; @@ -351,7 +342,7 @@ static const sigset_t l_UnixWorkerSignals = ([]() -> sigset_t { static Atomic l_CurrentlyStartingUnixWorkerPid (-1); // The state of the seamless worker currently being started by StartUnixWorker() -static Atomic l_CurrentlyStartingUnixWorkerState (UnixWorkerState::Pending); +static Atomic l_CurrentlyStartingUnixWorkerReady (false); // The last temination signal we received static Atomic l_TermSignal (-1); @@ -373,10 +364,10 @@ static void UmbrellaSignalHandler(int num, siginfo_t *info, void*) l_RequestedReopenLogs.store(true); break; case SIGUSR2: - if (l_CurrentlyStartingUnixWorkerState.load() == UnixWorkerState::Pending + if (!l_CurrentlyStartingUnixWorkerReady.load() && (info->si_pid == 0 || info->si_pid == l_CurrentlyStartingUnixWorkerPid.load()) ) { // The seamless worker currently being started by StartUnixWorker() successfully loaded its config - l_CurrentlyStartingUnixWorkerState.store(UnixWorkerState::LoadedConfig); + l_CurrentlyStartingUnixWorkerReady.store(true); } break; case SIGINT: @@ -568,7 +559,7 @@ static pid_t StartUnixWorker(const std::vector& configs, bool close break; } - if (l_CurrentlyStartingUnixWorkerState.load() == UnixWorkerState::LoadedConfig) { + if (l_CurrentlyStartingUnixWorkerReady.load()) { Log(LogNotice, "cli") << "Worker process successfully loaded its config"; break; @@ -579,7 +570,7 @@ static pid_t StartUnixWorker(const std::vector& configs, bool close // Reset flags for the next time l_CurrentlyStartingUnixWorkerPid.store(-1); - l_CurrentlyStartingUnixWorkerState.store(UnixWorkerState::Pending); + l_CurrentlyStartingUnixWorkerReady.store(false); try { Application::InitializeBase();