logging: Ensure -debug=0/none behaves consistently with -nodebug #31767

pull danielabrozzoni wants to merge 1 commits into bitcoin:master from danielabrozzoni:debug_flag_consistency changing 3 files +28 −7
  1. danielabrozzoni commented at 5:04 pm on January 30, 2025: contributor

    Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied. However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.

    This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.

    See #30529 (review)

  2. DrahtBot commented at 5:04 pm on January 30, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31767.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30529 (Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)

    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.

  3. DrahtBot added the label RPC/REST/ZMQ on Jan 30, 2025
  4. danielabrozzoni force-pushed on Jan 30, 2025
  5. danielabrozzoni force-pushed on Jan 30, 2025
  6. in doc/release-notes-31767.md:1 in 55c752a6d8 outdated
    0@@ -0,0 +1,3 @@
    1+RPC
    


    maflcko commented at 1:28 pm on February 4, 2025:
    What is the connection to RPC? I was under the impression that -debug is a setting related to logging.

    danielabrozzoni commented at 2:58 pm on February 4, 2025:
    You’re right, thanks! Updated with “Logging” as it feels more appropriate to me too

    luke-jr commented at 4:07 am on February 5, 2025:
    PR title and commit message too, please
  7. danielabrozzoni force-pushed on Feb 4, 2025
  8. DrahtBot added the label CI failed on Feb 4, 2025
  9. DrahtBot removed the label CI failed on Feb 4, 2025
  10. logging: Ensure -debug=0/none behaves consistently with -nodebug
    Previously, -nodebug cleared all prior -debug configurations in the
    command line while allowing subsequent debug options to be applied.
    However, -debug=0 and -debug=none completely disabled debugging,
    even for categories specified afterward.
    
    This commit ensures consistency by making -debug=0 and -debug=none
    behave like -nodebug: they now clear previously set debug configurations
    but do not disable debugging for categories specified later.
    
    Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
    1f4aff75c3
  11. danielabrozzoni renamed this:
    rpc: Ensure -debug=0/none behaves consistently with -nodebug
    logging: Ensure -debug=0/none behaves consistently with -nodebug
    on Feb 5, 2025
  12. danielabrozzoni force-pushed on Feb 5, 2025
  13. DrahtBot added the label CI failed on Feb 5, 2025
  14. DrahtBot removed the label CI failed on Feb 5, 2025
  15. maflcko removed the label RPC/REST/ZMQ on Feb 5, 2025
  16. in src/init/common.cpp:91 in 1f4aff75c3
    93+        auto last_negated = categories.rbegin();
    94+        for (; last_negated != categories.rend(); ++last_negated) {
    95+            if (*last_negated == "0" || *last_negated == "none") {
    96+                break;
    97+            }
    98+        }
    


    hodlinator commented at 8:17 pm on February 6, 2025:

    nit: I tried to replicate what ChatGPT suggested to ryanofsky, but std::views wouldn’t compile (didn’t look into modifying my setup). Was able to combine it with your version though into:

    0        auto last_negated = std::find_if(categories.rbegin(), categories.rend(),
    1            [](const std::string& cat) { return cat == "0" || cat == "none"; });
    
  17. in src/init/common.cpp:97 in 1f4aff75c3
     99+
    100+        auto range_to_process = (last_negated == categories.rend()) ? categories : std::ranges::subrange(last_negated.base(), categories.end());
    101+
    102+        for (const auto& cat : range_to_process) {
    103+            if (!LogInstance().EnableCategory(cat)) {
    104+                return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)};
    


    hodlinator commented at 8:19 pm on February 6, 2025:

    mega-nit: I find this marginally nicer, but still not great:

    0        for (auto it = last_negated == categories.rend() ? categories.begin() : last_negated.base();
    1            it != categories.end(); ++it) {
    2            if (!LogInstance().EnableCategory(*it)) {
    3                return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", *it)};
    
  18. in test/functional/feature_logging.py:106 in 1f4aff75c3
     98@@ -99,6 +99,18 @@ def run_test(self):
     99             match=ErrorMatch.PARTIAL_REGEX,
    100         )
    101 
    102+        self.log.info("Test that -nodebug,-debug=0,-debug=none clear previously specified debug options")
    103+        disable_debug_options = [
    104+            '-debug=0',
    105+            '-debug=none',
    106+            '-nodebug'
    


    hodlinator commented at 8:21 pm on February 6, 2025:
    nit: I tested breaking out an initial commit which only adds the test for -nodebug to prove existing behavior, before the commit performing the C++ change and adding 0 + none tests (it passed). You might want to rewrite the commit history to do that as well, but I’m okay with the current approach.
  19. in test/functional/feature_logging.py:112 in 1f4aff75c3
    107+        ]
    108+
    109+        for debug_opt in disable_debug_options:
    110+            self.restart_node(0, ['-debug=http', debug_opt, '-debug=rpc'])
    111+            logging = self.nodes[0].logging()
    112+            assert logging['rpc']
    


    hodlinator commented at 8:23 pm on February 6, 2025:

    nit: Could add invalid abc category prior to disabling, and verify that multiple categories after the disabling work.

    0            self.restart_node(0, ['-debug=http', '-debug=abc', debug_opt, '-debug=rpc', '-debug=net'])
    1            logging = self.nodes[0].logging()
    2            assert logging['rpc']
    3            assert logging['net']
    
  20. hodlinator approved
  21. hodlinator commented at 8:50 pm on February 6, 2025: contributor

    ACK 1f4aff75c308226d58a45460982f75287e15cb81

    Concept

    Having -debug=0 and -debug=none perform the same effect as -nodebug in discarding all preceding categories makes more sense, instead of disabling all categories including the ones following them.

    Testing

    As well as running the new test as-is, I also verified stated behavior of -nodebug prior to the C++ change by modifying included test.

    Suggestions

    Only found nits, the last_negated calculation in particular feels unnecessarily verbose & mutation heavy.


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: 2025-02-07 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me