[checkmk-commits] Windows agent: Refactor use of OpenHardwareMonitor

Module: check_mk Branch: master Commit: 6f4f60e1c40d01a05c96a3a0456f3f08b394ecd0 URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=6f4f60e1c40d01… Author: Jukka Aro &lt;ja(a)mathias-kettner.de&gt; Date: Fri Dec 15 11:58:33 2017 +0100 Windows agent: Refactor use of OpenHardwareMonitor * Wrap all raw handles to managing classes. * Avoid unnecessary use of unique_ptr. * Use more descriptive names. * Increase const correctness. Change-Id: I0cd635340ab28ac91c00c7e9c211bdcb458297a3 --- agents/windows/OHMMonitor.cc | 74 +++++++++++++---------------------- agents/windows/OHMMonitor.h | 35 ++++++++++++----- agents/windows/build_version | 2 +- agents/windows/sections/SectionOHM.cc | 9 ++--- agents/windows/sections/SectionOHM.h | 3 +- 5 files changed, 59 insertions(+), 64 deletions(-) diff --git a/agents/windows/OHMMonitor.cc b/agents/windows/OHMMonitor.cc index af14903..1b782a0 100644 --- a/agents/windows/OHMMonitor.cc +++ b/agents/windows/OHMMonitor.cc @@ -27,83 +27,65 @@ #include "WinApiAdaptor.h" #include "types.h" -OHMMonitor::OHMMonitor(const std::string &bin_path, Logger *logger, - const WinApiAdaptor &winapi) - : _exe_path(bin_path + "\\OpenHardwareMonitorCLI.exe") - , _logger(logger) - , _winapi(winapi) { - _available = (_winapi.GetFileAttributes(_exe_path.c_str()) != - INVALID_FILE_ATTRIBUTES); -} - -OHMMonitor::~OHMMonitor() { - if (_current_process != INVALID_HANDLE_VALUE) { - DWORD exitCode = 0; - if (!_winapi.GetExitCodeProcess(_current_process, &exitCode)) { - // invalid handle - _winapi.CloseHandle(_current_process); - } else { - if (exitCode == STILL_ACTIVE) { - // shut down ohm process - _winapi.TerminateProcess(_current_process, 0); - } - _winapi.CloseHandle(_current_process); - } - } -} +namespace { HANDLE dev_null(const WinApiAdaptor &winapi) { - SECURITY_ATTRIBUTES secattr = {}; - secattr.nLength = sizeof(SECURITY_ATTRIBUTES); - secattr.lpSecurityDescriptor = NULL; - secattr.bInheritHandle = TRUE; + SECURITY_ATTRIBUTES secattr{sizeof(SECURITY_ATTRIBUTES), nullptr, TRUE}; return winapi.CreateFile("nul:", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, &secattr, OPEN_EXISTING, 0, nullptr); } -bool OHMMonitor::checkAvailabe() { +} // namespace + +OHMMonitor::OHMMonitor(const std::string &bin_path, Logger *logger, + const WinApiAdaptor &winapi) + : _exe_path(bin_path + "\\OpenHardwareMonitorCLI.exe") + , _available(winapi.GetFileAttributes(_exe_path.c_str()) != + INVALID_FILE_ATTRIBUTES) + , _current_process(winapi) + , _logger(logger) + , _winapi(winapi) {} + +OHMMonitor::~OHMMonitor() {} + +bool OHMMonitor::startProcess() { if (!_available) { return false; } - if (_current_process != INVALID_HANDLE_VALUE) { - DWORD exitCode = 0; - if (!_winapi.GetExitCodeProcess(_current_process, &exitCode)) { - // handle invalid??? - Debug(_logger) << "ohm process handle invalid"; - _winapi.CloseHandle(_current_process); - _current_process = INVALID_HANDLE_VALUE; - } else { + if (_current_process) { + DWORD exitCode = STILL_ACTIVE; + if (!_winapi.GetExitCodeProcess(_current_process.get(), &exitCode) || + exitCode != STILL_ACTIVE) { if (exitCode != STILL_ACTIVE) { Debug(_logger) << "OHM process ended with exit code " << exitCode; - _winapi.CloseHandle(_current_process); - _current_process = INVALID_HANDLE_VALUE; } + _current_process = {INVALID_HANDLE_VALUE, _winapi}; } } - if (_current_process == INVALID_HANDLE_VALUE) { - STARTUPINFO si = {}; + if (!_current_process) { + STARTUPINFO si{0}; si.cb = sizeof(STARTUPINFO); si.dwFlags |= STARTF_USESTDHANDLES; si.hStdOutput = si.hStdError = dev_null(_winapi); + WrappedHandle<InvalidHandleTraits> fileHandle{si.hStdOutput, _winapi}; - OnScopeExit close_stdout([&]() { _winapi.CloseHandle(si.hStdOutput); }); - - PROCESS_INFORMATION pi = {}; + PROCESS_INFORMATION pi{0}; if (!_winapi.CreateProcess(_exe_path.c_str(), nullptr, nullptr, nullptr, TRUE, 0, nullptr, nullptr, &si, &pi)) { Error(_logger) << "failed to run %s" << _exe_path; return false; } else { - _current_process = pi.hProcess; + _current_process = {pi.hProcess, _winapi}; Debug(_logger) << "started " << _exe_path << " (pid " << pi.dwProcessId << ")"; - _winapi.CloseHandle(pi.hThread); + WrappedHandle<NullHandleTraits> threadHandle{pi.hThread, _winapi}; } } + return true; } diff --git a/agents/windows/OHMMonitor.h b/agents/windows/OHMMonitor.h index 84cb39e..cb6ebed 100644 --- a/agents/windows/OHMMonitor.h +++ b/agents/windows/OHMMonitor.h @@ -27,17 +27,32 @@ #include <winsock2.h> #include <windows.h> #include <string> +#include "types.h" class Logger; class WinApiAdaptor; -/** - * Ensure the Open Hardware Monitor is running (if it's available) - **/ +struct OhmProcessHandleTraits { + using HandleT = HANDLE; + static HandleT invalidValue() { return INVALID_HANDLE_VALUE; } + + static void closeHandle(HandleT value, const WinApiAdaptor &winapi) { + DWORD exitCode = 0; + if (winapi.GetExitCodeProcess(value, &exitCode) && + exitCode == STILL_ACTIVE) { + // shut down ohm process + winapi.TerminateProcess(value, 0); + } + winapi.CloseHandle(value); + } +}; + +using OhmProcessHandle = WrappedHandle<OhmProcessHandleTraits>; + class OHMMonitor { - std::string _exe_path; - bool _available; - HANDLE _current_process{INVALID_HANDLE_VALUE}; + const std::string _exe_path; + const bool _available; + OhmProcessHandle _current_process; Logger *_logger; const WinApiAdaptor &_winapi; @@ -46,7 +61,9 @@ public: const WinApiAdaptor &winapi); ~OHMMonitor(); - // this call actually starts OHM if necessary and returns - // true if it was already running or was successfully started - bool checkAvailabe(); + /** + * Ensure the Open Hardware Monitor is running (if it's available). + * Return true if it was already running or was successfully started. + **/ + bool startProcess(); }; diff --git a/agents/windows/build_version b/agents/windows/build_version index 95a3b3e..be8773d 100644 --- a/agents/windows/build_version +++ b/agents/windows/build_version @@ -1 +1 @@ -3026 +3028 diff --git a/agents/windows/sections/SectionOHM.cc b/agents/windows/sections/SectionOHM.cc index d85d24f..703f447 100644 --- a/agents/windows/sections/SectionOHM.cc +++ b/agents/windows/sections/SectionOHM.cc @@ -33,16 +33,13 @@ SectionOHM::SectionOHM(Configuration &config, Logger *logger, const WinApiAdaptor &winapi) : SectionWMI("openhardwaremonitor", "openhardwaremonitor", config.getEnvironment(), logger, winapi) - , _bin_path(_env.binDirectory()) { + , _ohm_monitor(_env.binDirectory(), _logger, _winapi) { withNamespace(L"Root\\OpenHardwareMonitor"); withObject(L"Sensor"); } void SectionOHM::startIfAsync() { - if (_ohm_monitor.get() == nullptr) { - _ohm_monitor.reset(new OHMMonitor(_bin_path, _logger, _winapi)); - _ohm_monitor->checkAvailabe(); - } + _ohm_monitor.startProcess(); } bool SectionOHM::produceOutputInner(std::ostream &out) { @@ -54,7 +51,7 @@ bool SectionOHM::produceOutputInner(std::ostream &out) { Debug(_logger) << "ComException: " << e.what(); res = false; } - if (!res && !_ohm_monitor->checkAvailabe()) { + if (!res && !_ohm_monitor.startProcess()) { Debug(_logger) << "ohm not installed or not runnable -> section disabled"; suspend(3600); diff --git a/agents/windows/sections/SectionOHM.h b/agents/windows/sections/SectionOHM.h index 8b594b1..8b37671 100644 --- a/agents/windows/sections/SectionOHM.h +++ b/agents/windows/sections/SectionOHM.h @@ -42,8 +42,7 @@ protected: virtual bool produceOutputInner(std::ostream &out) override; private: - std::unique_ptr<OHMMonitor> _ohm_monitor; - std::string _bin_path; + OHMMonitor _ohm_monitor; }; #endif // SectionOHM_h