From c5a1b39b7eb5312eebaa5ae9618bda210fe48f49 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jan 2025 14:40:35 +0000 Subject: [PATCH 1/6] fix: Better errors on logger init failure --- src/main/Main.cpp | 10 ++++++++-- src/util/log/Logger.cpp | 23 +++++++++++++++-------- src/util/log/Logger.hpp | 3 ++- tests/unit/LoggerTests.cpp | 28 ++++++++++++++++++++++++++-- tests/unit/rpc/WorkQueueTests.cpp | 7 +------ 5 files changed, 52 insertions(+), 19 deletions(-) diff --git a/src/main/Main.cpp b/src/main/Main.cpp index 4dd39e712..6d1ff8e11 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -52,7 +52,10 @@ try { if (not app::parseConfig(run.configPath)) return EXIT_FAILURE; - util::LogService::init(gClioConfig); + if (auto const initSuccess = util::LogService::init(gClioConfig); not initSuccess) { + std::cerr << initSuccess.error() << std::endl; + return EXIT_FAILURE; + } app::ClioApplication clio{gClioConfig}; return clio.run(run.useNgWebServer); }, @@ -60,7 +63,10 @@ try { if (not app::parseConfig(migrate.configPath)) return EXIT_FAILURE; - util::LogService::init(gClioConfig); + if (auto const initSuccess = util::LogService::init(gClioConfig); not initSuccess) { + std::cerr << initSuccess.error() << std::endl; + return EXIT_FAILURE; + } app::MigratorApplication migrator{gClioConfig, migrate.subCmd}; return migrator.run(); } diff --git a/src/util/log/Logger.cpp b/src/util/log/Logger.cpp index 3f9a35dda..d6abd20b4 100644 --- a/src/util/log/Logger.cpp +++ b/src/util/log/Logger.cpp @@ -28,8 +28,6 @@ #include #include -#include -#include #include #include #include @@ -48,18 +46,20 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include #include -#include #include #include +#include #include #include @@ -111,7 +111,7 @@ getSeverityLevel(std::string_view logLevel) std::unreachable(); } -void +std::expected LogService::init(config::ClioConfigDefinition const& config) { namespace keywords = boost::log::keywords; @@ -132,9 +132,15 @@ LogService::init(config::ClioConfigDefinition const& config) auto const logDir = config.maybeValue("log_directory"); if (logDir) { - boost::filesystem::path dirPath{logDir.value()}; - if (!boost::filesystem::exists(dirPath)) - boost::filesystem::create_directories(dirPath); + std::filesystem::path dirPath{logDir.value()}; + if (not std::filesystem::exists(dirPath)) { + if (std::error_code error; not std::filesystem::create_directories(dirPath, error)) { + return std::unexpected{ + fmt::format("Couldn't create logs directory '{}': {}", dirPath.string(), error.message()) + }; + } + } + auto const rotationPeriod = config.get("log_rotation_hour_interval"); // the below are taken from user in MB, but boost::log::add_file_log needs it to be in bytes @@ -170,7 +176,7 @@ LogService::init(config::ClioConfigDefinition const& config) auto const& channelConfig = *it; auto const name = channelConfig.get("channel"); if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) - throw std::runtime_error("Can't override settings for log channel " + name + ": invalid channel"); + return std::unexpected{fmt::format("Can't override settings for log channel {}: invalid channel", name)}; minSeverity[name] = getSeverityLevel(channelConfig.get("log_level")); } @@ -189,6 +195,7 @@ LogService::init(config::ClioConfigDefinition const& config) filter = boost::log::filter{std::move(logFilter)}; boost::log::core::get()->set_filter(filter); LOG(LogService::info()) << "Default log level = " << defaultSeverity; + return {}; } Logger::Pump diff --git a/src/util/log/Logger.hpp b/src/util/log/Logger.hpp index a3df8ffa9..2aea168dd 100644 --- a/src/util/log/Logger.hpp +++ b/src/util/log/Logger.hpp @@ -46,6 +46,7 @@ #include #include +#include #include #include #include @@ -279,7 +280,7 @@ class LogService { * * @param config The configuration to use */ - static void + [[nodiscard]] static std::expected init(config::ClioConfigDefinition const& config); /** diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index a8f986bf5..fcece57ad 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -26,9 +26,11 @@ #include "util/newconfig/ConfigValue.hpp" #include "util/newconfig/Types.hpp" +#include #include #include #include +#include #include #include @@ -113,7 +115,7 @@ TEST_F(LoggerInitTest, DefaultLogLevel) ASSERT_FALSE(parsingErrors.has_value()); std::string const logString = "some log"; - LogService::init(config_); + EXPECT_TRUE(LogService::init(config_)); for (auto const& channel : Logger::kCHANNELS) { Logger const log{channel}; log.trace() << logString; @@ -151,7 +153,7 @@ TEST_F(LoggerInitTest, ChannelLogLevel) ASSERT_FALSE(parsingErrors.has_value()); std::string const logString = "some log"; - LogService::init(config_); + EXPECT_TRUE(LogService::init(config_)); for (auto const& channel : Logger::kCHANNELS) { Logger const log{channel}; log.trace() << logString; @@ -175,6 +177,28 @@ TEST_F(LoggerInitTest, ChannelLogLevel) } } +TEST_F(LoggerInitTest, InitReturnsErrorIfCouldNotCreateLogDirectory) +{ + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{{"log_directory", "/root"}}}); + ASSERT_FALSE(parsingErrors.has_value()); + + auto const result = LogService::init(config_); + EXPECT_FALSE(result); + EXPECT_THAT(result.error(), testing::HasSubstr("Couldn't create logs directory")); +} + +TEST_F(LoggerInitTest, InitReturnsErrorIfProvidedInvalidChannel) +{ + auto const parsingErrors = config_.parse(ConfigFileJson{ + boost::json::object{{"log_channels", boost::json::array{boost::json::object{{"channel", "SomeChannel"}}}}} + }); + ASSERT_FALSE(parsingErrors.has_value()); + + auto const result = LogService::init(config_); + EXPECT_FALSE(result); + EXPECT_EQ(result.error(), "Can't override settings for log channel SomeChannel: invalid channel"); +} + TEST_F(LoggerInitTest, LogSizeAndHourRotationCannotBeZero) { std::vector const keys{ diff --git a/tests/unit/rpc/WorkQueueTests.cpp b/tests/unit/rpc/WorkQueueTests.cpp index ccc4fa5e6..5253cc9e3 100644 --- a/tests/unit/rpc/WorkQueueTests.cpp +++ b/tests/unit/rpc/WorkQueueTests.cpp @@ -56,12 +56,7 @@ TEST_F(WorkQueueTest, WhitelistedExecutionCountAddsUp) std::atomic_uint32_t executeCount = 0u; for (auto i = 0u; i < kTOTAL; ++i) { - queue.postCoro( - [&executeCount](auto /* yield */) { - ++executeCount; - }, - true - ); + queue.postCoro([&executeCount](auto /* yield */) { ++executeCount; }, true); } queue.join(); From 9ee6f9b5d52bf149c56b6996e5bc4c47599bdcd5 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jan 2025 15:41:12 +0000 Subject: [PATCH 2/6] Use better directory --- tests/unit/LoggerTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index fcece57ad..8b683b4b2 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -179,7 +179,7 @@ TEST_F(LoggerInitTest, ChannelLogLevel) TEST_F(LoggerInitTest, InitReturnsErrorIfCouldNotCreateLogDirectory) { - auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{{"log_directory", "/root"}}}); + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{{"log_directory", "/proc/logs"}}}); ASSERT_FALSE(parsingErrors.has_value()); auto const result = LogService::init(config_); From c707e9e769475feffd1415a07a918aba658d1bd8 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jan 2025 15:49:57 +0000 Subject: [PATCH 3/6] Fix docs --- src/util/log/Logger.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/log/Logger.hpp b/src/util/log/Logger.hpp index 2aea168dd..eb6ddcaee 100644 --- a/src/util/log/Logger.hpp +++ b/src/util/log/Logger.hpp @@ -279,6 +279,7 @@ class LogService { * @brief Global log core initialization from a @ref Config * * @param config The configuration to use + * @return Void on success, error message on failure */ [[nodiscard]] static std::expected init(config::ClioConfigDefinition const& config); From 0a066ec591806f744a9536506a903074a83a2676 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jan 2025 16:57:23 +0000 Subject: [PATCH 4/6] Debug test --- src/util/log/Logger.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/log/Logger.cpp b/src/util/log/Logger.cpp index d6abd20b4..ce5abb988 100644 --- a/src/util/log/Logger.cpp +++ b/src/util/log/Logger.cpp @@ -175,6 +175,7 @@ LogService::init(config::ClioConfigDefinition const& config) for (auto it = overrides.begin(); it != overrides.end(); ++it) { auto const& channelConfig = *it; auto const name = channelConfig.get("channel"); + std::println("{}", name); if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) return std::unexpected{fmt::format("Can't override settings for log channel {}: invalid channel", name)}; From cff1352407c679ab9efee23382c21a6b0e554972 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jan 2025 17:35:48 +0000 Subject: [PATCH 5/6] Fix test --- src/util/log/Logger.cpp | 1 - tests/unit/LoggerTests.cpp | 13 ++++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/util/log/Logger.cpp b/src/util/log/Logger.cpp index ce5abb988..d6abd20b4 100644 --- a/src/util/log/Logger.cpp +++ b/src/util/log/Logger.cpp @@ -175,7 +175,6 @@ LogService::init(config::ClioConfigDefinition const& config) for (auto it = overrides.begin(); it != overrides.end(); ++it) { auto const& channelConfig = *it; auto const name = channelConfig.get("channel"); - std::println("{}", name); if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) return std::unexpected{fmt::format("Can't override settings for log channel {}: invalid channel", name)}; diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index 8b683b4b2..ac4ccc7fc 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -189,9 +189,16 @@ TEST_F(LoggerInitTest, InitReturnsErrorIfCouldNotCreateLogDirectory) TEST_F(LoggerInitTest, InitReturnsErrorIfProvidedInvalidChannel) { - auto const parsingErrors = config_.parse(ConfigFileJson{ - boost::json::object{{"log_channels", boost::json::array{boost::json::object{{"channel", "SomeChannel"}}}}} - }); + auto const jsonStr = R"json( + { + "log_channels": [ + { + "channel": "SomeChannel" + } + ] + })json"; + + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::parse(jsonStr).as_object()}); ASSERT_FALSE(parsingErrors.has_value()); auto const result = LogService::init(config_); From 0c6b06f625a7cdf1018d09ff4bc991ad98baed71 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Thu, 30 Jan 2025 12:34:30 +0000 Subject: [PATCH 6/6] Debug test --- src/util/log/Logger.cpp | 6 +++++- tests/unit/LoggerTests.cpp | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/log/Logger.cpp b/src/util/log/Logger.cpp index d6abd20b4..be597f3a7 100644 --- a/src/util/log/Logger.cpp +++ b/src/util/log/Logger.cpp @@ -175,10 +175,14 @@ LogService::init(config::ClioConfigDefinition const& config) for (auto it = overrides.begin(); it != overrides.end(); ++it) { auto const& channelConfig = *it; auto const name = channelConfig.get("channel"); - if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) + std::cout << "Channel: " << name << std::endl; + if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) { + std::cout << "Not found" << std::endl; return std::unexpected{fmt::format("Can't override settings for log channel {}: invalid channel", name)}; + } minSeverity[name] = getSeverityLevel(channelConfig.get("log_level")); + std::cout << "Severity set to: " << minSeverity[name] << std::endl; } auto logFilter = [minSeverity = std::move(minSeverity), diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index ac4ccc7fc..7d46377ba 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -198,7 +198,9 @@ TEST_F(LoggerInitTest, InitReturnsErrorIfProvidedInvalidChannel) ] })json"; - auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::parse(jsonStr).as_object()}); + auto json = boost::json::parse(jsonStr).as_object(); + std::cout << json << std::endl; + auto const parsingErrors = config_.parse(ConfigFileJson{json}); ASSERT_FALSE(parsingErrors.has_value()); auto const result = LogService::init(config_);