Change LogAcceptCategory to use uint32_t rather than sets of strings. #9424

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:log_category_simplify changing 30 files +377 −273
  1. gmaxwell commented at 8:33 pm on December 25, 2016: contributor

    This changes the logging categories to boolean flags instead of strings.

    This simplifies the acceptance testing by avoiding accessing a scoped static thread local pointer to a thread local set of strings. It eliminates the only use of boost::thread_specific_ptr outside of lockorder debugging.

    This change allows log entries to be directed to multiple categories and makes it easy to change the logging flags at runtime (e.g. via an RPC, though that isn’t done by this commit.)

    It also eliminates the fDebug global.

    Configuration of unknown logging categories now produces a warning.

  2. gmaxwell commented at 8:37 pm on December 25, 2016: contributor
    If we ever need more than 32 categories (there are 19 now) this could be trivially changed to use a uint64_t.
  3. in src/net_processing.cpp: in 295aa46929 outdated 
    1438 
1439-        if ((fDebug && vInv.size() > 0) || (vInv.size() == 1))
1440-            LogPrint("net", "received getdata for: %s peer=%d\n", vInv[0].ToString(), pfrom->id);
1441+        if (vInv.size() > 0)
1442+            LogPrint(LOG_NET, "received getdata for: %s peer=%d\n", vInv[0].ToString(), pfrom->id);
1443
    gmaxwell commented at 8:45 pm on December 25, 2016:
    The prior code above struck me as somewhat brain-damaged (LogPrint() does nothing when fDebug isn’t true), so this chunk could use some more review attention in case I was missing something.
  4. fanquake added the label Refactoring on Dec 26, 2016
  5. fanquake added the label Utils and libraries on Dec 26, 2016
  6. in src/util.h: in e348647315 outdated 
    69@@ -69,15 +70,50 @@ inline std::string _(const char* psz)
70 void SetupEnvironment();
71 bool SetupNetworking();
72 
73+enum LogFlags : uint32_t {
    rebroad commented at 12:28 pm on December 26, 2016:
    idea: Would be easier to maintain if it was possible to define LogFlags and CLogCategoryDesc in the same section…
  7. gmaxwell commented at 6:00 pm on December 29, 2016: contributor
    One week, no substantive feedback. Closing.
  8. gmaxwell closed this on Dec 29, 2016
  9. TheBlueMatt commented at 6:03 pm on December 29, 2016: member
    Concept ACK
  10. paveljanik commented at 10:48 am on December 30, 2016: contributor

    Please reopen. Holidays…

    Concept ACK

  11. rebroad commented at 1:18 pm on December 30, 2016: contributor
    concept ACK
  12. MarcoFalke commented at 2:17 pm on December 30, 2016: member
    utACK e348647315b619e6a1d8b614554abb0747047696
  13. paveljanik commented at 7:50 pm on December 30, 2016: contributor
    Needs rebase.
  14. paveljanik commented at 6:27 pm on January 8, 2017: contributor
    @gmaxwell Please reopen this…
  15. jonasschnelli commented at 7:33 pm on March 30, 2017: contributor
    Concept ACK, seems to be a great PR with serval acks. Needs rebase and reopen.
  16. jtimon commented at 7:40 pm on March 30, 2017: contributor
    Concept ACK
  17. jnewbery commented at 7:43 pm on March 30, 2017: member
    Definite concept ACK. Makes #10123 trivial.
  18. gmaxwell reopened this on Mar 30, 2017
  19. laanwj commented at 8:19 pm on March 30, 2017: member
    Concept ACK.
  20. in src/util.h:74 in e348647315 outdated 
    69@@ -69,15 +70,50 @@ inline std::string _(const char* psz)
70 void SetupEnvironment();
71 bool SetupNetworking();
72 
73+enum LogFlags : uint32_t {
74+    LOG_NONE        = 0,
75+    LOG_NET         = (1 <<  0),
    laanwj commented at 8:20 pm on March 30, 2017:
    Instead of the LOG_ prefix, I’d propose to use a scoped enum. E.g. LogFlags::NONE LogFlags::NET etc. (defined using enum class LogFlags)
    gmaxwell commented at 1:44 am on March 31, 2017:
    Oh darn, didn’t see this until I’d already done the rebase. Uh, I’ll look into it and put it as a commit on top for squashing. (presumably it’ll be a more or less trivial change that could be reviewed separately)
    gmaxwell commented at 2:40 am on March 31, 2017:
    19:32 < gmaxwell> wumpus: uh. so boolean operations don’t appear to exist for scoped enums. so … I don’t think I can use them without defining a set of boolean operations for them. 19:32 < sipa> gmaxwell: eh? 19:33 < sipa> ah, scoped enums don’t have an implicit conversion to int 19:34 < sipa> you can still explicitly convert them, though 19:34 < sipa> but enums are generally a bad approach for bitfields, as they’re intended to be exhaustive 19:34 < gmaxwell> sipa: wumpus suggested that I change the log print thing to use a scoped enum. The original thing that inspired this change is that rebroad wanted to do some horrible hack to do the logical equivilent of LogPrint(LOG_NET | LOG_MEMPOOL, …). (and of course the matching also uses it as a bitfield.) 19:35 < sipa> right, a normal enum (which behaves much more like an int) may be more appropriate 19:35 < gmaxwell> well thats what I implemented. 19:37 < sipa> awesöme 19:37 < gmaxwell> in any case. I’m happy to change it to whatever. someone who has an opinion needs to tell me what to do. I think I only need a couple casts to make the enum class stuff go, but it would be ugly to do “LogPrint(LOG_NET | LOG_MEMPOOL,” later unless we provide | for the type.
    laanwj commented at 10:33 am on March 31, 2017:
    It’s just that the LOG_ prefix is very common, so it’s easy to get namespace collisions. Scoped enums avoid that, the only way any of their members can collide is when the name of the type conflicts, which would be an error in the first place. Then again if this introduces extra C++ casting uglyness in all usages let’s leave it like this…
    gmaxwell commented at 6:27 pm on March 31, 2017:
    I could make it BCLOG or something like that, you’re right about namespace collisions, my initial sed through to rename the LOG_ accidentally renamed some things it shouldn’t.
    sipa commented at 6:31 pm on March 31, 2017:

    Another approach would be wrapping it in a C++ namespace (for example:

    0namespace bclog {
1enum flags {
2  NET,
3  MEMPOOL,
4  ...
5};
6}

    at which point you can use bclog::NET | bclog::MEMPOOL.

  21. in src/util.h:116 in e348647315 outdated 
    114-#define LogPrintf(...) LogPrint(NULL, __VA_ARGS__)
115+#define LogPrintf(...) LogPrintStr(tfm::format(__VA_ARGS__))
116 
117 template<typename... Args>
118-static inline int LogPrint(const char* category, const char* fmt, const Args&... args)
119+static inline int LogPrint(uint32_t category, const char* fmt, const Args&... args)
    JeremyRubin commented at 8:27 pm on March 30, 2017:
    type should maybe be LogFlags for future upgrades.
    laanwj commented at 10:46 am on March 31, 2017:
    Yes, that would add some type safety. It would make it more of a hassle to submit a message to more categories at once but that isn’t supported now either.
  22. in src/util.h:57 in e348647315 outdated 
    54@@ -56,6 +55,8 @@ extern CTranslationInterface translationInterface;
55 extern const char * const BITCOIN_CONF_FILENAME;
56 extern const char * const BITCOIN_PID_FILENAME;
57 
58+extern std::atomic<uint32_t> logCategories;
    JeremyRubin commented at 8:31 pm on March 30, 2017:
    isn’t this set on init single threadedly? Maybe you can drop the atomics.
    gmaxwell commented at 2:41 am on March 31, 2017:
    I expect the very next logical change will be to add an RPC to turn on and off logging categories. Since all the reads are order relaxed I expect it to emit the same code as a non-atomic read (at least on x86).
    theuni commented at 7:15 am on March 31, 2017:
    I’ve wanted this for so long! Consider this the next/2 logical change.
    laanwj commented at 6:30 am on April 2, 2017:
    Yes being able to change the debug flags at runtime (even if not the libevent and leveldb ones which are processed at startup) would be nice.
  23. JeremyRubin commented at 2:04 am on March 31, 2017: contributor

    Concept ACK!

    I think it would be nice if there were a semi-convenient way to add new categories without triggering a big recompile though…

  24. JeremyRubin commented at 5:00 pm on March 31, 2017: contributor

    Suggestion on how to make the category more extensible without triggering big recompiles:

    0#define LogPrint(cat, ...) _LogPrint(cat, #cat, __VA_ARGS__)
1static inline int _LogPrint(uint32_t category, const char* category_str, const char* fmt, const Args&... args) {
2    // ...
3    if (category == LOG_USE_IDENT_NAME)
4        // use the macro
5    // ...
6}

    then from somefile.cpp

    0static const uint32_t LOG_SOMEFILE = LOG_USE_IDENT_NAME;
1LogPrint(LOG_SOMEFILE, "...");
  25. in src/util.h:107 in ab53beaf9c outdated 
    103+}
104+
105+/** Returns a string with the supported log categories */
106+std::string ListLogCategories();
107+
108+/** Return true of str parses as a log category and set the flags in f */
    jnewbery commented at 6:50 pm on March 31, 2017:
    s/of/if
  26. in src/util.h:108 in ab53beaf9c outdated 
    104+
105+/** Returns a string with the supported log categories */
106+std::string ListLogCategories();
107+
108+/** Return true of str parses as a log category and set the flags in f */
109+bool ParseLogCategory(uint32_t *f, const std::string *str);
    jnewbery commented at 6:56 pm on March 31, 2017:
    I prefer a name like SetLogCategory. Parse suggests no side-effects to me.
  27. in src/util.cpp:261 in ab53beaf9c outdated 
    276+    {LOG_MEMPOOLREJ, "mempoolrej"},
277+    {LOG_LIBEVENT, "libevent"},
278+    {LOG_COINDB, "coindb"},
279+    {LOG_QT, "qt"},
280+    {LOG_LEVELDB, "leveldb"},
281+    {LOG_ALL, "1"},
    jnewbery commented at 7:03 pm on March 31, 2017:
    Can you add an “all” alias here, ie -debug=all is equivalent to -debug=1
    gmaxwell commented at 0:20 am on April 1, 2017:
    Yup.
  28. in src/init.cpp:908 in ab53beaf9c outdated 
    904@@ -908,12 +905,23 @@ bool AppInitParameterInteraction()
905 
906     // ********************************************************* Step 3: parameter-to-internal-flags
907 
908-    fDebug = mapMultiArgs.count("-debug");
909+    bool fDebug = mapMultiArgs.count("-debug") > 0;
    jnewbery commented at 7:07 pm on March 31, 2017:

    Now that you’ve removed the global fDebug bool, you can go ahead and remove it entirely and test the conditions directly (rather than make the variable local), ie:

    if (mapMultiArgs.count("-debug") > 0) {

    and further below:

    if !(GetBoolArg("-nodebug", false) || find(categories.begin(), categories.end(), std::string("0")) != categories.end()) {

    gmaxwell commented at 0:11 am on April 1, 2017:
    Great point.
  29. in src/util.h:113 in ab53beaf9c outdated 
    128-#define LogPrint(category, ...) do { \
129-    if (LogAcceptCategory((category))) { \
130-        LogPrintf(__VA_ARGS__); \
131-    } \
132-} while(0)
133+#define LogPrintf(...) LogPrintStr(tfm::format(__VA_ARGS__))
    jnewbery commented at 7:15 pm on March 31, 2017:
    Why have you removed the try/except handling in this macro? (I’m not saying it’s wrong, I just don’t understand why it was necessary before but not now).
    jnewbery commented at 7:17 pm on March 31, 2017:
    EDIT: I think this is a bad rebase that’s reverted #9963
    gmaxwell commented at 0:20 am on April 1, 2017:
    Yep. Fixing.
  30. in src/tinyformat.h:170 in ab53beaf9c outdated 
    166@@ -167,7 +167,7 @@ namespace tinyformat {
167 class format_error: public std::runtime_error
168 {
169 public:
170-    format_error(const std::string &what): std::runtime_error(what) {
171+    format_error(const std::string &fmtwhat): std::runtime_error(fmtwhat) {
    jnewbery commented at 7:17 pm on March 31, 2017:
    bad rebase?
    gmaxwell commented at 0:07 am on April 1, 2017:
    No. silencing an obnoxious shadowing warning that was making it impossible to see where other issues were.
    jnewbery commented at 11:17 am on April 1, 2017:
    My only concern is that we occasionally pull updates to tinyformat.cpp from upstream (eg #8274), so any trivial changes like this add to the maintainer’s burden when merging. Not sure if @laanwj has an opinion.
    laanwj commented at 12:13 pm on April 1, 2017:
    Can we just disable the shadowing warnings please, instead of peppering unrelated variable renames in commits?
    laanwj commented at 12:21 pm on April 1, 2017:
    There you go: #10136
  31. jnewbery commented at 7:43 pm on March 31, 2017: member

    It looks to me like there are a couple of rebase errors here. Other than that a few nits but looks good. I’ve tested ab53beaf9c0d8f923f24a50b373968a8c01760d4 manually and against the extended test suite and it works.

    One thing to note about the future logging RPC work: I think libevent and leveldb logging levels are set at start up, so they won’t be modifiable through the RPC without some additional work. Not a problem for this PR, but something to note since it’d be easy to believe that just updating logCategories would be enough. Perhaps worth putting a comment next to the logCategories variable warning about this?

  32. gmaxwell commented at 7:13 am on April 1, 2017: contributor
    I think I addressed all the comments.
  33. laanwj commented at 12:47 pm on April 1, 2017: member

    Suggestion on how to make the category more extensible without triggering big recompiles:

    IMO we shouldn’t worry about recompiles when the set of categories changes. That’s a rare occurrence, and the way headers depend on each other almost every header change already results in a (near) full recompile.

  34. laanwj commented at 1:24 pm on April 1, 2017: member
    utACK https://github.com/bitcoin/bitcoin/pull/9424/commits/21d94e712ddd7859bfaf02282eaa38cb18ec7e1b apart from the parameter name change in tinyformat.h
  35. Change LogAcceptCategory to use uint32_t rather than sets of strings. 
    This changes the logging categories to boolean flags instead of strings.

This simplifies the acceptance testing by avoiding accessing a scoped
 static thread local pointer to a thread local set of strings.  It
 eliminates the only use of boost::thread_specific_ptr outside of
 lockorder debugging.

This change allows log entries to be directed to multiple categories
 and makes it easy to change the logging flags at runtime (e.g. via
 an RPC, though that isn't done by this commit.)

It also eliminates the fDebug global.

Configuration of unknown logging categories now produces a warning.
    6b3bb3d9ba
  36. gmaxwell commented at 7:00 pm on April 1, 2017: contributor
    (Trivally) rebased, squashed, and removed the tinyformat change.
  37. laanwj commented at 6:32 am on April 2, 2017: member
    Going to merge this; a change like this will need rebase after every single thing that gets merged, and @gmaxwell addressed all the nits. Further improvements can be done in later PRs.
  38. laanwj merged this on Apr 2, 2017
  39. laanwj closed this on Apr 2, 2017
  40. laanwj referenced this in commit 1a5aaabb8a on Apr 2, 2017
  41. PastaPastaPasta referenced this in commit a3cbf2c161 on May 12, 2019
  42. PastaPastaPasta referenced this in commit 6eacba4d49 on May 12, 2019
  43. UdjinM6 referenced this in commit 5273b60ba1 on May 16, 2019
  44. PastaPastaPasta referenced this in commit 938f13b89f on May 17, 2019
  45. PastaPastaPasta referenced this in commit ebe58da243 on May 21, 2019
  46. PastaPastaPasta referenced this in commit 793c6702cf on May 21, 2019
  47. PastaPastaPasta referenced this in commit 437ea8a331 on May 21, 2019
  48. PastaPastaPasta referenced this in commit 048a99a4f7 on May 22, 2019
  49. PastaPastaPasta referenced this in commit 76952e6721 on May 22, 2019
  50. UdjinM6 referenced this in commit 29194b1f5a on May 22, 2019
  51. barrystyle referenced this in commit b773a6230f on Jan 22, 2020
  52. furszy referenced this in commit 8a6f5147c9 on Apr 1, 2020
  53. DrahtBot locked this on Sep 8, 2021


gmaxwell rebroad TheBlueMatt paveljanik MarcoFalke jonasschnelli jtimon jnewbery laanwj sipa JeremyRubin theuni

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-26 16:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me