Move special cases from LOG_CATEGORIES_BY_STR to GetLogCategory() (suggested here).
Remove "none" and "0" from RPC logging help because that help text was wrong. "none" resulted in an error and "0" was ignored itself (contrary to what the help text suggested).
Remove unused LOG_CATEGORIES_BY_STR[""] (suggested here).
This is a followup to #29419, addressing leftover suggestions + more.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | LarryRuane, ryanofsky |
| Concept ACK | kevkevinpal |
| Stale ACK | ajtowns |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
315@@ -317,8 +316,6 @@ namespace BCLog {
316
317 std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level level) const
318 {
319- if (category == LogFlags::NONE) category = LogFlags::ALL;
NONE does not make sense logically and no caller does it currently. Thus I removed this. But you are right that a (nonsensical?) caller could be added in the future. Would you prefer an assert like assert(category != LogFlags::NONE); here or to restore this line as it is (was)?
re: #29798 (review) In commit “logging: remove unused code” (d98ca056a82264884bc1aa2da3af540a73ee8f26)
Would you prefer an assert
I’d suggest just dropping this commit entirely. If you adopt my suggestion from the first commit the {"", BCLog::NONE} map entry already disappears so this does not provide an additional simplification to the map, and I don’t think there’s really another reason to change GetLogPrefix behavior here, at least not as part of this PR.
199- if (str.empty()) {
200+ if (str.empty() || str == "1") {
201 flag = BCLog::ALL;
202 return true;
203 }
204+ if (str == "0") {
In commit “logging: move special cases to GetLogCategory()” (9fb321ad2b0176955718317b796715250eac01f4)
Would be good if commit title had a “refactor:” prefix so it is clear this is not intended to change behavior.
Also, I think this commit should remove ALL and NONE cases from the map, so special cases are handled completely within the GetLogCategory() and LogCategoryToStr() functions, and there are no more special cases in the map and LogCategoriesList(). Would suggest:
0--- a/src/logging.cpp
1+++ b/src/logging.cpp
2@@ -142,7 +142,6 @@ bool BCLog::Logger::DefaultShrinkDebugFile() const
3 }
4
5 static const std::map<std::string, BCLog::LogFlags> LOG_CATEGORIES_BY_STR{
6- {"", BCLog::NONE},
7 {"net", BCLog::NET},
8 {"tor", BCLog::TOR},
9 {"mempool", BCLog::MEMPOOL},
10@@ -174,7 +173,6 @@ static const std::map<std::string, BCLog::LogFlags> LOG_CATEGORIES_BY_STR{
11 {"txreconciliation", BCLog::TXRECONCILIATION},
12 {"scan", BCLog::SCAN},
13 {"txpackages", BCLog::TXPACKAGES},
14- {"all", BCLog::ALL},
15 };
16
17 static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_FLAG{
18@@ -191,11 +189,10 @@ static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_
19
20 bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
21 {
22- if (str.empty() || str == "1") {
23+ if (str.empty() || str == "1" || str == "all") {
24 flag = BCLog::ALL;
25 return true;
26- }
27- if (str == "0") {
28+ } else if (str == "0") {
29 flag = BCLog::NONE;
30 return true;
31 }
32@@ -226,6 +223,11 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level)
33
34 std::string LogCategoryToStr(BCLog::LogFlags category)
35 {
36+ if (category == BCLog::NONE) {
37+ return "";
38+ } else if (category == BCLog::ALL) {
39+ return "all";
40+ }
41 auto it = LOG_CATEGORIES_BY_FLAG.find(category);
42 assert(it != LOG_CATEGORIES_BY_FLAG.end());
43 return it->second;
44@@ -252,9 +254,7 @@ std::vector<LogCategory> BCLog::Logger::LogCategoriesList() const
45 {
46 std::vector<LogCategory> ret;
47 for (const auto& [category, flag] : LOG_CATEGORIES_BY_STR) {
48- if (flag != BCLog::NONE && flag != BCLog::ALL) {
49- ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
50- }
51+ ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
52 }
53 return ret;
54 }
And would suggest a commit message like:
0logging, refactor: make category special cases explicit
1
2Make special cases explicit in GetLogCategory() and LogCategoryToStr()
3functions. Simplify the LOG_CATEGORIES_BY_STR and LOG_CATEGORIES_BY_FLAG
4mappings and LogCategoriesList() function.
103@@ -104,13 +104,15 @@ namespace BCLog {
104 std::atomic<Level> m_log_level{DEFAULT_LOG_LEVEL};
105
106 /** Log categories bitfield. */
107- std::atomic<uint32_t> m_categories{0};
108+ std::atomic<uint32_t> m_categories{BCLog::NONE};
In commit “logging: minor encapsulation improvement and use BCLog::NONE instead of 0” (1fae74149507e5914a7bb7986e4906053b8dbbbb)
Would be good to add “refactor:” prefix to commit message so it is obvious this is not supposed to change behavior.
194@@ -195,10 +195,6 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
195 flag = BCLog::ALL;
196 return true;
197 }
198- if (str == "0") {
In commit “rpc: remove misleading text from logging help” (e78866a10973c64a59e8a71eed737328b6bc0ca3)
Right now this returns NONE/true when the empty string "" is passed. Should return false in that case too and never return NONE?
Also, this is a nice change, but I found description confusing because the whole thing sounds it this like a help text change, but then the last sentence mentions it also adds a new error. Would suggest a description more like:
0rpc: make logging method reject "0" category
1
2Current logging RPC method documentation claims to accept "0" and "none"
3categories, but the "none" argument is actually rejected and the "0" argument is
4ignored. Update the implementation to refuse both categories, and remove the
5help text claiming to support them.
0 value as the first commit so we’re not adding code and removing within two commits
Reworded the commit message as suggested, but I didn’t get this:
Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in master and in this PR - there is an if (str.empty() ... a few lines above.
Should return false in that case too
You mean to return false when empty string is passed? That would be some further change that maybe makes sense, but is not included in this PR.
and never return NONE?
It never returns NONE (in this PR).
"0" from LOG_CATEGORIES_BY_STR which is used outside of GetLogCategory() and then it becomes difficult to asses the change. So I left it as it is.
Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in
masterand in this PR - there is anif (str.empty() ...a few lines above.
Right, I think I was confused when I wrote that comment. Makes sense to keep "" behavior unchanged, too.
Code review ACK d98ca056a82264884bc1aa2da3af540a73ee8f26
Nice cleanups! Main feedback I have is to more clearly indicate which commits are refactoring, and which have behavior changes. Would suggest putting “logging, refactor:” in refactoring commit titles and “logging:” in commits that change behavior. Also I would consider dropping any behavior changes that aren’t obvious improvments and doing them in separate PRs so they have more visibility and are not lost in “logging cleanup.”
Concept ACK
just a question is there any reason to keep BCLog::NONE in logging.h and not completely delete it I did
grep --exclude-dir=".deps" -nri "BCLog::NONE" ./src/ --binary-files=without-match
and
grep --exclude-dir=".deps" -nri "LogFlags::NONE" ./src/ --binary-files=without-match
and only see it being used in ./src/logging.{h,cpp} and ./src//qt/transactiondesc.cpp
d98ca056a8...bee22409ea: rebase and address suggestions
Edit: @kevkevinpal, I am not sure about dropping BCLog::NONE on the technical side, but what is sure is that it would further increase the scope of this PR, so I am leaving it as it is :)
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
bee22409ea...df2422c5f9: fix CI
this compiles locally but not on the CI:
0ret.emplace_back(category, WillLogCategory(flag));
so instead use:
0ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});
Consider also updating the help text for -debug and -debugexclude:
0--- a/src/init/common.cpp
1+++ b/src/init/common.cpp
2@@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman)
3 {
4 argsman.AddArg("-debuglogfile=<file>", 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);
5 argsman.AddArg("-debug=<category>", "Output debug and trace logging (default: -nodebug, supplying <category> is optional). "
6- "If <category> is not supplied or if <category> = 1, output all debug and trace logging. <category> can be: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
7+ "If <category> is not supplied or if <category> is 1 or \"all\", output all debug logging. If <category> is 0 or \"none\", any other categories are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
8 ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
9- argsman.AddArg("-debugexclude=<category>", "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);
10+ argsman.AddArg("-debugexclude=<category>", "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);
217@@ -225,8 +218,14 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level)
218 assert(false);
219 }
220
221-std::string LogCategoryToStr(BCLog::LogFlags category)
222+static std::string LogCategoryToStr(BCLog::LogFlags category)
223 {
224+ if (category == BCLog::NONE) {
225+ return "";
226+ }
nit, in commit “logging, refactor: make category special cases explicit” (eacf21f7cb3d4122fd196a2e49b2d7857a611bcb)
I don’t think it’s possible for the argument to be NONE, so perhaps:
0 assert(category != BCLog::NONE)'
(or maybe add a comment that this should never happen)
LogCategoryToStr() has checked explicitly that it is not NONE. This was suggested here: #29798 (review), @ryanofsky, what do you think?
re: #29798 (review)
Good catch. Since this case can’t happen and the function is already written to assert on other invalid categories, agree it’s better to remove this unused special case. I think it might be overkill to add an assert for this since there is an assert right below that will catch it, but no real opinion on that.
if, no need for two asserts.
224+ if (category == BCLog::NONE) {
225+ return "";
226+ }
227+ if (category == BCLog::ALL) {
228+ return "all";
229+ }
very nit
0 if (category == BCLog::ALL) return "all";
{ as it makes debugging easier (set a breakpoint on the line with the return statement) and also if a line happens to be added in the body of the if, then the diff is smaller.
178 };
179
180 static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_FLAG{
181 // Swap keys and values from LOG_CATEGORIES_BY_STR.
182 [](const std::map<std::string, BCLog::LogFlags>& in) {
183 std::unordered_map<BCLog::LogFlags, std::string> out;
In commit “logging, refactor: make category special cases explicit” (eacf21f7cb3d4122fd196a2e49b2d7857a611bcb) Although you’re not touching this line, perhaps as long as you’re in the vicinity, add:
0 out.reserve(in.size());
df2422c5f9...397211b573: rebase due to conflicts
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).
* 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()`).
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.