From 160706aa387245ed96b1f13e5362fe1837e8fc4b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 3 Apr 2024 10:52:36 +0200 Subject: [PATCH 1/4] logging, refactor: make category special cases explicit Make special cases explicit in GetLogCategory() and LogCategoryToStr() functions. Simplify the LOG_CATEGORIES_BY_STR and LOG_CATEGORIES_BY_FLAG mappings and LogCategoriesList() function. This makes the maps `LOG_CATEGORIES_BY_STR` and `LOG_CATEGORIES_BY_FLAG` consistent (one is exactly the opposite of the other). --- src/logging.cpp | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 9c87cfd2b7..6aeab4dfb4 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -168,8 +168,6 @@ bool BCLog::Logger::DefaultShrinkDebugFile() const } static const std::map> LOG_CATEGORIES_BY_STR{ - {"0", BCLog::NONE}, - {"", BCLog::NONE}, {"net", BCLog::NET}, {"tor", BCLog::TOR}, {"mempool", BCLog::MEMPOOL}, @@ -201,8 +199,6 @@ static const std::map> LOG_CATEGORIES_ {"txreconciliation", BCLog::TXRECONCILIATION}, {"scan", BCLog::SCAN}, {"txpackages", BCLog::TXPACKAGES}, - {"1", BCLog::ALL}, - {"all", BCLog::ALL}, }; static const std::unordered_map LOG_CATEGORIES_BY_FLAG{ @@ -210,11 +206,8 @@ static const std::unordered_map LOG_CATEGORIES_BY_ [](const auto& in) { std::unordered_map out; for (const auto& [k, v] : in) { - switch (v) { - case BCLog::NONE: out.emplace(BCLog::NONE, ""); break; - case BCLog::ALL: out.emplace(BCLog::ALL, "all"); break; - default: out.emplace(v, k); - } + const bool inserted{out.emplace(v, k).second}; + assert(inserted); } return out; }(LOG_CATEGORIES_BY_STR) @@ -222,10 +215,14 @@ static const std::unordered_map LOG_CATEGORIES_BY_ bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str) { - if (str.empty()) { + if (str.empty() || str == "1" || str == "all") { flag = BCLog::ALL; return true; } + if (str == "0") { + flag = BCLog::NONE; + return true; + } auto it = LOG_CATEGORIES_BY_STR.find(str); if (it != LOG_CATEGORIES_BY_STR.end()) { flag = it->second; @@ -253,6 +250,9 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) std::string LogCategoryToStr(BCLog::LogFlags category) { + if (category == BCLog::ALL) { + return "all"; + } auto it = LOG_CATEGORIES_BY_FLAG.find(category); assert(it != LOG_CATEGORIES_BY_FLAG.end()); return it->second; @@ -278,10 +278,9 @@ static std::optional GetLogLevel(std::string_view level_str) std::vector BCLog::Logger::LogCategoriesList() const { std::vector ret; + ret.reserve(LOG_CATEGORIES_BY_STR.size()); for (const auto& [category, flag] : LOG_CATEGORIES_BY_STR) { - if (flag != BCLog::NONE && flag != BCLog::ALL) { - ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)}); - } + ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)}); } return ret; } From 8c6f3bf1634533a0dd268dcf5929e49429640a09 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 3 Apr 2024 11:00:20 +0200 Subject: [PATCH 2/4] logging, refactor: minor encapsulation improvement and use BCLog::NONE instead of 0 * Make the standalone function `LogCategoryToStr()` private inside `logging.cpp` (aka `static`) - it is only used in that file. * Make the method `Logger::GetLogPrefix()` `private` - it is only used within the class. * Use `BCLog::NONE` to initialize `m_categories` instead of `0`. We later check whether it is `BCLog::NONE` (in `Logger::DefaultShrinkDebugFile()`). --- src/logging.cpp | 2 +- src/logging.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 6aeab4dfb4..175eae2cc9 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -248,7 +248,7 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) assert(false); } -std::string LogCategoryToStr(BCLog::LogFlags category) +static std::string LogCategoryToStr(BCLog::LogFlags category) { if (category == BCLog::ALL) { return "all"; diff --git a/src/logging.h b/src/logging.h index 91f919e822..2ff58979cb 100644 --- a/src/logging.h +++ b/src/logging.h @@ -119,7 +119,7 @@ namespace BCLog { std::atomic m_log_level{DEFAULT_LOG_LEVEL}; /** Log categories bitfield. */ - std::atomic m_categories{0}; + std::atomic m_categories{BCLog::NONE}; void FormatLogStrInPlace(std::string& str, LogFlags category, Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const; @@ -132,6 +132,8 @@ namespace BCLog { void LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level) EXCLUSIVE_LOCKS_REQUIRED(m_cs); + std::string GetLogPrefix(LogFlags category, Level level) const; + public: bool m_print_to_console = false; bool m_print_to_file = false; @@ -145,8 +147,6 @@ namespace BCLog { fs::path m_file_path; std::atomic m_reopen_file{false}; - std::string GetLogPrefix(LogFlags category, Level level) const; - /** Send a string to the log output */ void LogPrintStr(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); From 74dd33cb0a967086df32e5140d58843ca1359d81 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 3 Apr 2024 12:57:25 +0200 Subject: [PATCH 3/4] rpc: make logging method reject "0" category and correct the help text Current logging RPC method documentation claims to accept "0" and "none" categories, but the "none" argument is actually rejected and the "0" argument is ignored. Update the implementation to refuse both categories, and remove the help text claiming to support them. --- src/logging.cpp | 4 ---- src/rpc/node.cpp | 1 - 2 files changed, 5 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 175eae2cc9..9a54a12b42 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -219,10 +219,6 @@ bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str) flag = BCLog::ALL; return true; } - if (str == "0") { - flag = BCLog::NONE; - return true; - } auto it = LOG_CATEGORIES_BY_STR.find(str); if (it != LOG_CATEGORIES_BY_STR.end()) { flag = it->second; diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp index 65b0a93cdd..54e2c8e226 100644 --- a/src/rpc/node.cpp +++ b/src/rpc/node.cpp @@ -221,7 +221,6 @@ static RPCHelpMan logging() "The valid logging categories are: " + LogInstance().LogCategoriesString() + "\n" "In addition, the following are available as category names with special meanings:\n" " - \"all\", \"1\" : represent all logging categories.\n" - " - \"none\", \"0\" : even if other logging categories are specified, ignore all of them.\n" , { {"include", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging", From a7432dd6ed3e13a272d62ecde535e6d562cc932c Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 2 Aug 2024 15:00:27 +0200 Subject: [PATCH 4/4] logging: clarify -debug and -debugexclude descriptions --- src/init/common.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/init/common.cpp b/src/init/common.cpp index 00ef879dc1..36142c2b9a 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman) { argsman.AddArg("-debuglogfile=", strprintf("Specify location of debug log file (default: %s). Relative paths will be prefixed by a net-specific datadir location. Pass -nodebuglogfile to disable writing the log to a file.", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-debug=", "Output debug and trace logging (default: -nodebug, supplying is optional). " - "If is not supplied or if = 1, output all debug and trace logging. can be: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.", + "If is not supplied or if is 1 or \"all\", output all debug logging. If is 0 or \"none\", any other categories are ignored. Other valid values for are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-debugexclude=", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-debugexclude=", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\"", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-loglevel=|:", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are %s (default=%s). The following levels are always logged: error, warning, info. If : is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. can be: %s.", LogInstance().LogLevelsString(), LogInstance().LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-logtimestamps", strprintf("Prepend debug output with timestamp (default: %u)", DEFAULT_LOGTIMESTAMPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);