From bb8e9e7c4c8d70914d0878a0d7c6a1371dae23c0 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Tue, 16 Dec 2025 15:37:21 +0000 Subject: [PATCH] logging: Move message formatting to util/log.h With this change, callers can use util/log.h to emit log messages and do not need to include the full logging implementation in logging.h. There's a potential performance impact with this change from an extra `strprintf` call in log statements where `Logger::WillLogCategoryLevel` returns true but `Logger::Enabled` returns false. This happens when bitcoind is run with `-noprinttoconsole -nodebuglogfile` options. For background, log macro arguments are supposed to be evaluated when `Logger::WillLogCategoryLevel` returns true, even if log output is not enabled. Changing this behavior would be reasonable but needs consideration in a separate PR since not evaluating arguments in log statements has the potential to change non-logging behavior. The extra `strprintf` call could have been avoided by expanding this change and making the `ShouldLog()` function return a tri-state DO_LOG / DO_NOT_LOG / DO_NOT_LOG_ONLY_EVALUATE_ARGS value instead of a bool, but this complexity did not seem warranted. Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space Co-authored-by: Ryan Ofsky --- src/logging.cpp | 13 +++++++++++++ src/logging.h | 19 ------------------- src/util/log.h | 42 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 54 insertions(+), 20 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 1c3e30e9807..df6946d6611 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -602,3 +602,16 @@ bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::stri m_category_log_levels[flag] = level.value(); return true; } + +bool util::log::ShouldLog(Category category, Level level) +{ + return LogInstance().WillLogCategoryLevel(static_cast(category), level); +} + +void util::log::Log(util::log::Entry entry) +{ + BCLog::Logger& logger{LogInstance()}; + if (logger.Enabled()) { + logger.LogPrintStr(std::move(entry.message), std::move(entry.source_loc), static_cast(entry.category), entry.level, entry.should_ratelimit); + } +} diff --git a/src/logging.h b/src/logging.h index d41a33bc3af..e21495015b6 100644 --- a/src/logging.h +++ b/src/logging.h @@ -9,8 +9,6 @@ #include #include // IWYU pragma: export #include -#include -#include #include #include // IWYU pragma: export #include @@ -22,11 +20,8 @@ #include #include #include -#include -#include #include #include -#include #include static const bool DEFAULT_LOGTIMEMICROS = false; @@ -304,18 +299,4 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve /** Return true if str parses as a log category and set the flag */ bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str); -template -inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags flag, BCLog::Level level, bool should_ratelimit, util::ConstevalFormatString fmt, const Args&... args) -{ - if (LogInstance().Enabled()) { - std::string log_msg; - try { - log_msg = tfm::format(fmt, args...); - } catch (tinyformat::format_error& fmterr) { - log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt; - } - LogInstance().LogPrintStr(log_msg, std::move(source_loc), flag, level, should_ratelimit); - } -} - #endif // BITCOIN_LOGGING_H diff --git a/src/util/log.h b/src/util/log.h index ef5cbf14ec5..aae23f0df1e 100644 --- a/src/util/log.h +++ b/src/util/log.h @@ -5,8 +5,13 @@ #ifndef BITCOIN_UTIL_LOG_H #define BITCOIN_UTIL_LOG_H +#include // IWYU pragma: export +#include +#include + #include #include +#include #include /// Like std::source_location, but allowing to override the function name. @@ -30,6 +35,9 @@ private: }; namespace util::log { +/** Opaque to util::log; interpreted by consumers (e.g., BCLog::LogFlags). */ +using Category = uint64_t; + enum class Level { Trace = 0, // High-volume or detailed logging for development/debugging Debug, // Reasonably noisy logging, but still usable in production @@ -37,6 +45,21 @@ enum class Level { Warning, Error, }; + +struct Entry { + Category category; + Level level; + bool should_ratelimit{false}; //!< Hint for consumers if this entry should be ratelimited + SourceLocation source_loc; + std::string message; +}; + +/** Return whether messages with specified category and level should be logged. Applications using + * the logging library need to provide this. */ +bool ShouldLog(Category category, Level level); + +/** Send message to be logged. Applications using the logging library need to provide this. */ +void Log(Entry entry); } // namespace util::log namespace BCLog { @@ -44,6 +67,23 @@ namespace BCLog { using Level = util::log::Level; } // namespace BCLog +template +inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags flag, BCLog::Level level, bool should_ratelimit, util::ConstevalFormatString fmt, const Args&... args) +{ + std::string log_msg; + try { + log_msg = tfm::format(fmt, args...); + } catch (tinyformat::format_error& fmterr) { + log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt; + } + util::log::Log(util::log::Entry{ + .category = flag, + .level = level, + .should_ratelimit = should_ratelimit, + .source_loc = std::move(source_loc), + .message = std::move(log_msg)}); +} + // Allow __func__ to be used in any context without warnings: // NOLINTNEXTLINE(bugprone-lambda-function-name) #define LogPrintLevel_(category, level, should_ratelimit, ...) LogPrintFormatInternal(SourceLocation{__func__}, category, level, should_ratelimit, __VA_ARGS__) @@ -64,7 +104,7 @@ using Level = util::log::Level; // developers or power users who are aware that -debug may cause excessive disk usage due to logging. #define detail_LogIfCategoryAndLevelEnabled(category, level, ...) \ do { \ - if (LogAcceptCategory((category), (level))) { \ + if (util::log::ShouldLog((category), (level))) { \ bool rate_limit{level >= BCLog::Level::Info}; \ Assume(!rate_limit); /*Only called with the levels below*/ \ LogPrintLevel_(category, level, rate_limit, __VA_ARGS__); \