Skip to content

Commit e4bbfb2

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#27632: Raise on invalid -debug and -loglevel config options
daa5a65 refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE (Jon Atack) cf622b2 doc: release note re raising on invalid -debug/debugexclude/loglevel (Jon Atack) 6cb1c66 init: remove config option names from translated -loglevel strings (Jon Atack) 2547829 test: -loglevel raises on invalid values (Jon Atack) a9c2958 init: raise on invalid loglevel config option (Jon Atack) b0c3995 test: -debug and -debugexclude raise on invalid values (Jon Atack) 4c3c19d init: raise on invalid debug/debugexclude config options (Jon Atack) Pull request description: and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums. Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458. ACKs for top commit: achow101: ACK daa5a65 ryanofsky: Code review ACK daa5a65. Just translated string template cleanup since last review pinheadmz: re-ACK daa5a65 Tree-SHA512: 4c107a93d8e8ce4e2ee81d44aec672526ca354ec390b241221067f68204beac8b4ba7a65748bcfa124ff2245c4307fa9243ec4fe0b464d0fa69c787fb322c3cc
2 parents 688c613 + daa5a65 commit e4bbfb2

File tree

9 files changed

+67
-18
lines changed

9 files changed

+67
-18
lines changed

‎doc/release-notes-27632.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Updated settings
2+
----------------
3+
4+
- Passing an invalid `-debug`, `-debugexclude`, or `-loglevel` logging configuration
5+
option now raises an error, rather than logging an easily missed warning. (#27632)

‎src/init.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@
7777
#include <util/fs.h>
7878
#include <util/fs_helpers.h>
7979
#include <util/moneystr.h>
80+
#include <util/result.h>
8081
#include <util/strencodings.h>
8182
#include <util/string.h>
8283
#include <util/syscall_sandbox.h>
@@ -951,8 +952,10 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
951952
InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
952953

953954
// ********************************************************* Step 3: parameter-to-internal-flags
954-
init::SetLoggingCategories(args);
955-
init::SetLoggingLevel(args);
955+
auto result = init::SetLoggingCategories(args);
956+
if (!result) return InitError(util::ErrorString(result));
957+
result = init::SetLoggingLevel(args);
958+
if (!result) return InitError(util::ErrorString(result));
956959

957960
nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT);
958961
if (nConnectTimeout <= 0) {

‎src/init/common.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include <tinyformat.h>
1414
#include <util/fs.h>
1515
#include <util/fs_helpers.h>
16+
#include <util/result.h>
1617
#include <util/string.h>
1718
#include <util/time.h>
1819
#include <util/translation.h>
@@ -58,27 +59,28 @@ void SetLoggingOptions(const ArgsManager& args)
5859
fLogIPs = args.GetBoolArg("-logips", DEFAULT_LOGIPS);
5960
}
6061

61-
void SetLoggingLevel(const ArgsManager& args)
62+
util::Result<void> SetLoggingLevel(const ArgsManager& args)
6263
{
6364
if (args.IsArgSet("-loglevel")) {
6465
for (const std::string& level_str : args.GetArgs("-loglevel")) {
6566
if (level_str.find_first_of(':', 3) == std::string::npos) {
6667
// user passed a global log level, i.e. -loglevel=<level>
6768
if (!LogInstance().SetLogLevel(level_str)) {
68-
InitWarning(strprintf(_("Unsupported global logging level -loglevel=%s. Valid values: %s."), level_str, LogInstance().LogLevelsString()));
69+
return util::Error{strprintf(_("Unsupported global logging level %s=%s. Valid values: %s."), "-loglevel", level_str, LogInstance().LogLevelsString())};
6970
}
7071
} else {
7172
// user passed a category-specific log level, i.e. -loglevel=<category>:<level>
7273
const auto& toks = SplitString(level_str, ':');
7374
if (!(toks.size() == 2 && LogInstance().SetCategoryLogLevel(toks[0], toks[1]))) {
74-
InitWarning(strprintf(_("Unsupported category-specific logging level -loglevel=%s. Expected -loglevel=<category>:<loglevel>. Valid categories: %s. Valid loglevels: %s."), level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString()));
75+
return util::Error{strprintf(_("Unsupported category-specific logging level %1$s=%2$s. Expected %1$s=<category>:<loglevel>. Valid categories: %3$s. Valid loglevels: %4$s."), "-loglevel", level_str, LogInstance().LogCategoriesString(), LogInstance().LogLevelsString())};
7576
}
7677
}
7778
}
7879
}
80+
return {};
7981
}
8082

81-
void SetLoggingCategories(const ArgsManager& args)
83+
util::Result<void> SetLoggingCategories(const ArgsManager& args)
8284
{
8385
if (args.IsArgSet("-debug")) {
8486
// Special-case: if -debug=0/-nodebug is set, turn off debugging messages
@@ -88,7 +90,7 @@ void SetLoggingCategories(const ArgsManager& args)
8890
[](std::string cat){return cat == "0" || cat == "none";})) {
8991
for (const auto& cat : categories) {
9092
if (!LogInstance().EnableCategory(cat)) {
91-
InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debug", cat));
93+
return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)};
9294
}
9395
}
9496
}
@@ -97,9 +99,10 @@ void SetLoggingCategories(const ArgsManager& args)
9799
// Now remove the logging categories which were explicitly excluded
98100
for (const std::string& cat : args.GetArgs("-debugexclude")) {
99101
if (!LogInstance().DisableCategory(cat)) {
100-
InitWarning(strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat));
102+
return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)};
101103
}
102104
}
105+
return {};
103106
}
104107

105108
bool StartLogging(const ArgsManager& args)

‎src/init/common.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,15 @@
88
#ifndef BITCOIN_INIT_COMMON_H
99
#define BITCOIN_INIT_COMMON_H
1010

11+
#include <util/result.h>
12+
1113
class ArgsManager;
1214

1315
namespace init {
1416
void AddLoggingArgs(ArgsManager& args);
1517
void SetLoggingOptions(const ArgsManager& args);
16-
void SetLoggingCategories(const ArgsManager& args);
17-
void SetLoggingLevel(const ArgsManager& args);
18+
[[nodiscard]] util::Result<void> SetLoggingCategories(const ArgsManager& args);
19+
[[nodiscard]] util::Result<void> SetLoggingLevel(const ArgsManager& args);
1820
bool StartLogging(const ArgsManager& args);
1921
void LogPackageVersion();
2022
} // namespace init

‎src/logging.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ const CLogCategoryDesc LogCategories[] =
179179
{BCLog::LOCK, "lock"},
180180
#endif
181181
{BCLog::UTIL, "util"},
182-
{BCLog::BLOCKSTORE, "blockstorage"},
182+
{BCLog::BLOCKSTORAGE, "blockstorage"},
183183
{BCLog::TXRECONCILIATION, "txreconciliation"},
184184
{BCLog::SCAN, "scan"},
185185
{BCLog::ALL, "1"},
@@ -280,7 +280,7 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
280280
#endif
281281
case BCLog::LogFlags::UTIL:
282282
return "util";
283-
case BCLog::LogFlags::BLOCKSTORE:
283+
case BCLog::LogFlags::BLOCKSTORAGE:
284284
return "blockstorage";
285285
case BCLog::LogFlags::TXRECONCILIATION:
286286
return "txreconciliation";

‎src/logging.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ namespace BCLog {
6565
LOCK = (1 << 24),
6666
#endif
6767
UTIL = (1 << 25),
68-
BLOCKSTORE = (1 << 26),
68+
BLOCKSTORAGE = (1 << 26),
6969
TXRECONCILIATION = (1 << 27),
7070
SCAN = (1 << 28),
7171
ALL = ~(uint32_t)0,

‎src/node/blockstorage.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ void BlockManager::UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const
573573
const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)};
574574
const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)};
575575
if (removed_blockfile || removed_undofile) {
576-
LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
576+
LogPrint(BCLog::BLOCKSTORAGE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
577577
}
578578
}
579579
}
@@ -642,7 +642,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
642642

643643
if ((int)nFile != m_last_blockfile) {
644644
if (!fKnown) {
645-
LogPrint(BCLog::BLOCKSTORE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
645+
LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
646646
}
647647
FlushBlockFile(!fKnown, finalize_undo);
648648
m_last_blockfile = nFile;

‎src/test/logging_tests.cpp

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
200200
const char* argv_test[] = {"bitcoind", "-loglevel=debug"};
201201
std::string err;
202202
BOOST_REQUIRE(args.ParseParameters(2, argv_test, err));
203-
init::SetLoggingLevel(args);
203+
204+
auto result = init::SetLoggingLevel(args);
205+
BOOST_REQUIRE(result);
204206
BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug);
205207
}
206208

@@ -212,7 +214,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
212214
const char* argv_test[] = {"bitcoind", "-loglevel=net:trace"};
213215
std::string err;
214216
BOOST_REQUIRE(args.ParseParameters(2, argv_test, err));
215-
init::SetLoggingLevel(args);
217+
218+
auto result = init::SetLoggingLevel(args);
219+
BOOST_REQUIRE(result);
216220
BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::DEFAULT_LOG_LEVEL);
217221

218222
const auto& category_levels{LogInstance().CategoryLevels()};
@@ -229,7 +233,9 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
229233
const char* argv_test[] = {"bitcoind", "-loglevel=debug", "-loglevel=net:trace", "-loglevel=http:info"};
230234
std::string err;
231235
BOOST_REQUIRE(args.ParseParameters(4, argv_test, err));
232-
init::SetLoggingLevel(args);
236+
237+
auto result = init::SetLoggingLevel(args);
238+
BOOST_REQUIRE(result);
233239
BOOST_CHECK_EQUAL(LogInstance().LogLevel(), BCLog::Level::Debug);
234240

235241
const auto& category_levels{LogInstance().CategoryLevels()};

‎test/functional/feature_logging.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,36 @@ def run_test(self):
6969
# just sanity check no crash here
7070
self.restart_node(0, [f"-debuglogfile={os.devnull}"])
7171

72+
self.log.info("Test -debug and -debugexclude raise when invalid values are passed")
73+
self.stop_node(0)
74+
self.nodes[0].assert_start_raises_init_error(
75+
extra_args=["-debug=abc"],
76+
expected_msg="Error: Unsupported logging category -debug=abc.",
77+
match=ErrorMatch.FULL_REGEX,
78+
)
79+
self.nodes[0].assert_start_raises_init_error(
80+
extra_args=["-debugexclude=abc"],
81+
expected_msg="Error: Unsupported logging category -debugexclude=abc.",
82+
match=ErrorMatch.FULL_REGEX,
83+
)
84+
85+
self.log.info("Test -loglevel raises when invalid values are passed")
86+
self.nodes[0].assert_start_raises_init_error(
87+
extra_args=["-loglevel=abc"],
88+
expected_msg="Error: Unsupported global logging level -loglevel=abc. Valid values: info, debug, trace.",
89+
match=ErrorMatch.FULL_REGEX,
90+
)
91+
self.nodes[0].assert_start_raises_init_error(
92+
extra_args=["-loglevel=net:abc"],
93+
expected_msg="Error: Unsupported category-specific logging level -loglevel=net:abc.",
94+
match=ErrorMatch.PARTIAL_REGEX,
95+
)
96+
self.nodes[0].assert_start_raises_init_error(
97+
extra_args=["-loglevel=net:info:abc"],
98+
expected_msg="Error: Unsupported category-specific logging level -loglevel=net:info:abc.",
99+
match=ErrorMatch.PARTIAL_REGEX,
100+
)
101+
72102

73103
if __name__ == '__main__':
74104
LoggingTest().main()

0 commit comments

Comments
 (0)