From 4c3c19d943a0a4cf191495f6ebe9b964835607a4 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 12 May 2023 00:23:21 +0200 Subject: [PATCH 1/7] init: raise on invalid debug/debugexclude config options --- src/init.cpp | 4 +++- src/init/common.cpp | 8 +++++--- src/init/common.h | 4 +++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 38e1dbb4a2c..37aaeb30298 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -77,6 +77,7 @@ #include #include #include +#include #include #include #include @@ -949,7 +950,8 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections)); // ********************************************************* Step 3: parameter-to-internal-flags - init::SetLoggingCategories(args); + auto result = init::SetLoggingCategories(args); + if (!result) return InitError(util::ErrorString(result)); init::SetLoggingLevel(args); nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT); diff --git a/src/init/common.cpp b/src/init/common.cpp index 9a52a09cea0..2c36d30fdf1 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -78,7 +79,7 @@ void SetLoggingLevel(const ArgsManager& args) } } -void SetLoggingCategories(const ArgsManager& args) +util::Result SetLoggingCategories(const ArgsManager& args) { if (args.IsArgSet("-debug")) { // Special-case: if -debug=0/-nodebug is set, turn off debugging messages @@ -88,7 +89,7 @@ void SetLoggingCategories(const ArgsManager& args) [](std::string cat){return cat == "0" || cat == "none";})) { for (const auto& cat : categories) { if (!LogInstance().EnableCategory(cat)) { - InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)); + return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)}; } } } @@ -97,9 +98,10 @@ void SetLoggingCategories(const ArgsManager& args) // Now remove the logging categories which were explicitly excluded for (const std::string& cat : args.GetArgs("-debugexclude")) { if (!LogInstance().DisableCategory(cat)) { - InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)); + return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)}; } } + return {}; } bool StartLogging(const ArgsManager& args) diff --git a/src/init/common.h b/src/init/common.h index 44c3a502eea..1c7bcf86711 100644 --- a/src/init/common.h +++ b/src/init/common.h @@ -8,12 +8,14 @@ #ifndef BITCOIN_INIT_COMMON_H #define BITCOIN_INIT_COMMON_H +#include + class ArgsManager; namespace init { void AddLoggingArgs(ArgsManager& args); void SetLoggingOptions(const ArgsManager& args); -void SetLoggingCategories(const ArgsManager& args); +[[nodiscard]] util::Result SetLoggingCategories(const ArgsManager& args); void SetLoggingLevel(const ArgsManager& args); bool StartLogging(const ArgsManager& args); void LogPackageVersion(); From b0c3995393c592fa96306e077ed64e65d5400882 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 12 May 2023 00:42:02 +0200 Subject: [PATCH 2/7] test: -debug and -debugexclude raise on invalid values --- test/functional/feature_logging.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/functional/feature_logging.py b/test/functional/feature_logging.py index fe4f02dfe62..a33aa043f8b 100755 --- a/test/functional/feature_logging.py +++ b/test/functional/feature_logging.py @@ -69,6 +69,19 @@ class LoggingTest(BitcoinTestFramework): # just sanity check no crash here self.restart_node(0, [f"-debuglogfile={os.devnull}"]) + self.log.info("Test -debug and -debugexclude raise when invalid values are passed") + self.stop_node(0) + self.nodes[0].assert_start_raises_init_error( + extra_args=["-debug=abc"], + expected_msg="Error: Unsupported logging category -debug=abc.", + match=ErrorMatch.FULL_REGEX, + ) + self.nodes[0].assert_start_raises_init_error( + extra_args=["-debugexclude=abc"], + expected_msg="Error: Unsupported logging category -debugexclude=abc.", + match=ErrorMatch.FULL_REGEX, + ) + if __name__ == '__main__': LoggingTest().main() From a9c295888b82c86ef4629aa2d9061ea152b48f20 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 12 May 2023 00:58:35 +0200 Subject: [PATCH 3/7] init: raise on invalid loglevel config option --- src/init.cpp | 3 ++- src/init/common.cpp | 7 ++++--- src/init/common.h | 2 +- src/test/logging_tests.cpp | 12 +++++++++--- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 37aaeb30298..c343a107e6d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -952,7 +952,8 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb // ********************************************************* Step 3: parameter-to-internal-flags auto result = init::SetLoggingCategories(args); if (!result) return InitError(util::ErrorString(result)); - init::SetLoggingLevel(args); + result = init::SetLoggingLevel(args); + if (!result) return InitError(util::ErrorString(result)); nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT); if (nConnectTimeout <= 0) { diff --git a/src/init/common.cpp b/src/init/common.cpp index 2c36d30fdf1..b9c19725b1a 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -59,24 +59,25 @@ void SetLoggingOptions(const ArgsManager& args) fLogIPs = args.GetBoolArg("-logips", DEFAULT_LOGIPS); } -void SetLoggingLevel(const ArgsManager& args) +util::Result SetLoggingLevel(const ArgsManager& args) { if (args.IsArgSet("-loglevel")) { for (const std::string& level_str : args.GetArgs("-loglevel")) { if (level_str.find_first_of(':', 3) == std::string::npos) { // user passed a global log level, i.e. -loglevel= if (!LogInstance().SetLogLevel(level_str)) { - InitWarning(strprintf(_("Unsupported global logging level -loglevel=%s. Valid values: %s."), level_str, LogInstance().LogLevelsString())); + return util::Error{strprintf(_("Unsupported global logging level -loglevel=%s. Valid values: %s."), level_str, LogInstance().LogLevelsString())}; } } else { // user passed a category-specific log level, i.e. -loglevel=: const auto& toks = SplitString(level_str, ':'); if (!(toks.size() == 2 && LogInstance().SetCategoryLogLevel(toks[0], toks[1]))) { - InitWarning(strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=:. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())); + return util::Error{strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=:. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())}; } } } } + return {}; } util::Result SetLoggingCategories(const ArgsManager& args) diff --git a/src/init/common.h b/src/init/common.h index 1c7bcf86711..b61a77c6d46 100644 --- a/src/init/common.h +++ b/src/init/common.h @@ -16,7 +16,7 @@ namespace init { void AddLoggingArgs(ArgsManager& args); void SetLoggingOptions(const ArgsManager& args); [[nodiscard]] util::Result SetLoggingCategories(const ArgsManager& args); -void SetLoggingLevel(const ArgsManager& args); +[[nodiscard]] util::Result SetLoggingLevel(const ArgsManager& args); bool StartLogging(const ArgsManager& args); void LogPackageVersion(); } // namespace init diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index beb9398c74d..2699d316da8 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -200,7 +200,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup) const char* argv_test[] = {"bitcoind", "-loglevel=debug"}; std::string err; BOOST_REQUIRE(args.ParseParameters(2, argv_test, err)); - init::SetLoggingLevel(args); + + auto result = init::SetLoggingLevel(args); + BOOST_REQUIRE(result); BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug); } @@ -212,7 +214,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup) const char* argv_test[] = {"bitcoind", "-loglevel=net:trace"}; std::string err; BOOST_REQUIRE(args.ParseParameters(2, argv_test, err)); - init::SetLoggingLevel(args); + + auto result = init::SetLoggingLevel(args); + BOOST_REQUIRE(result); BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::DEFAULT_LOG_LEVEL); const auto& category_levels{LogInstance().CategoryLevels()}; @@ -229,7 +233,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup) const char* argv_test[] = {"bitcoind", "-loglevel=debug", "-loglevel=net:trace", "-loglevel=http:info"}; std::string err; BOOST_REQUIRE(args.ParseParameters(4, argv_test, err)); - init::SetLoggingLevel(args); + + auto result = init::SetLoggingLevel(args); + BOOST_REQUIRE(result); BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug); const auto& category_levels{LogInstance().CategoryLevels()}; From 25478292726dd7208b22a8924c8f1fdeac5c33f5 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 12 May 2023 00:58:56 +0200 Subject: [PATCH 4/7] test: -loglevel raises on invalid values --- test/functional/feature_logging.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/functional/feature_logging.py b/test/functional/feature_logging.py index a33aa043f8b..b0788e2a2d1 100755 --- a/test/functional/feature_logging.py +++ b/test/functional/feature_logging.py @@ -82,6 +82,23 @@ class LoggingTest(BitcoinTestFramework): match=ErrorMatch.FULL_REGEX, ) + self.log.info("Test -loglevel raises when invalid values are passed") + self.nodes[0].assert_start_raises_init_error( + extra_args=["-loglevel=abc"], + expected_msg="Error: Unsupported global logging level -loglevel=abc. Valid values: info, debug, trace.", + match=ErrorMatch.FULL_REGEX, + ) + self.nodes[0].assert_start_raises_init_error( + extra_args=["-loglevel=net:abc"], + expected_msg="Error: Unsupported category-specific logging level -loglevel=net:abc.", + match=ErrorMatch.PARTIAL_REGEX, + ) + self.nodes[0].assert_start_raises_init_error( + extra_args=["-loglevel=net:info:abc"], + expected_msg="Error: Unsupported category-specific logging level -loglevel=net:info:abc.", + match=ErrorMatch.PARTIAL_REGEX, + ) + if __name__ == '__main__': LoggingTest().main() From 6cb1c66041ee14dbedad3aeeb90190ea5dddf917 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Tue, 13 Jun 2023 12:51:39 -0600 Subject: [PATCH 5/7] init: remove config option names from translated -loglevel strings --- 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 b9c19725b1a..f3f7c696c54 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -66,13 +66,13 @@ util::Result SetLoggingLevel(const ArgsManager& args) if (level_str.find_first_of(':', 3) == std::string::npos) { // user passed a global log level, i.e. -loglevel= if (!LogInstance().SetLogLevel(level_str)) { - return util::Error{strprintf(_("Unsupported global logging level -loglevel=%s. Valid values: %s."), level_str, LogInstance().LogLevelsString())}; + return util::Error{strprintf(_("Unsupported global logging level %s=%s. Valid values: %s."), "-loglevel", level_str, LogInstance().LogLevelsString())}; } } else { // user passed a category-specific log level, i.e. -loglevel=: const auto& toks = SplitString(level_str, ':'); if (!(toks.size() == 2 && LogInstance().SetCategoryLogLevel(toks[0], toks[1]))) { - return util::Error{strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=:. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())}; + return util::Error{strprintf(_("Unsupported category-specific logging level %1$s=%2$s. Expected %1$s=:. Valid categories: %3$s. Valid loglevels: %4$s."), "-loglevel", level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())}; } } } From cf622b214bfe0a97e403f1e9dc54bf5bbfc59fc3 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Tue, 13 Jun 2023 13:08:00 -0600 Subject: [PATCH 6/7] doc: release note re raising on invalid -debug/debugexclude/loglevel --- doc/release-notes-27632.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 doc/release-notes-27632.md diff --git a/doc/release-notes-27632.md b/doc/release-notes-27632.md new file mode 100644 index 00000000000..d588247c858 --- /dev/null +++ b/doc/release-notes-27632.md @@ -0,0 +1,5 @@ +Updated settings +---------------- + +- Passing an invalid `-debug`, `-debugexclude`, or `-loglevel` logging configuration + option now raises an error, rather than logging an easily missed warning. (#27632) From daa5a658c0e79172e4dea0758246f11281790d29 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 12 May 2023 01:06:58 +0200 Subject: [PATCH 7/7] refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum name is the same as its value, like the other BCLog enums. --- src/logging.cpp | 4 ++-- src/logging.h | 2 +- src/node/blockstorage.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 7725fefeb7d..a5cfb0d28e3 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -179,7 +179,7 @@ const CLogCategoryDesc LogCategories[] = {BCLog::LOCK, "lock"}, #endif {BCLog::UTIL, "util"}, - {BCLog::BLOCKSTORE, "blockstorage"}, + {BCLog::BLOCKSTORAGE, "blockstorage"}, {BCLog::TXRECONCILIATION, "txreconciliation"}, {BCLog::SCAN, "scan"}, {BCLog::ALL, "1"}, @@ -280,7 +280,7 @@ std::string LogCategoryToStr(BCLog::LogFlags category) #endif case BCLog::LogFlags::UTIL: return "util"; - case BCLog::LogFlags::BLOCKSTORE: + case BCLog::LogFlags::BLOCKSTORAGE: return "blockstorage"; case BCLog::LogFlags::TXRECONCILIATION: return "txreconciliation"; diff --git a/src/logging.h b/src/logging.h index e7c554e79f8..fc03c8eac38 100644 --- a/src/logging.h +++ b/src/logging.h @@ -65,7 +65,7 @@ namespace BCLog { LOCK = (1 << 24), #endif UTIL = (1 << 25), - BLOCKSTORE = (1 << 26), + BLOCKSTORAGE = (1 << 26), TXRECONCILIATION = (1 << 27), SCAN = (1 << 28), ALL = ~(uint32_t)0, diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 1368ae6f6d5..87cd291c7e5 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -573,7 +573,7 @@ void BlockManager::UnlinkPrunedFiles(const std::set& setFilesToPrune) const const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)}; const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)}; if (removed_blockfile || removed_undofile) { - LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it); + LogPrint(BCLog::BLOCKSTORAGE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it); } } } @@ -642,7 +642,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne if ((int)nFile != m_last_blockfile) { if (!fKnown) { - LogPrint(BCLog::BLOCKSTORE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString()); + LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString()); } FlushBlockFile(!fKnown, finalize_undo); m_last_blockfile = nFile;