net: Modernize logging in UPnP and nat-pmp code #29978

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2024-04-portmap-logging changing 1 files +18 −18
  1. laanwj commented at 12:33 pm on April 27, 2024: member

    i’m looking at this code anyway for #17012, so thought i’d might just as well modernize the logging:

    • Use log level and log category NET (comes closest imo-because the mapping is for P2P)
      • Port mapping errors are logged to Warning category instead of Error, because they’re not fatal, and not generally considered very serious problems.
    • Prefix UPnP and natpmp messages consistently.
  2. net: Modernize logging in UPnP and nat-pmp code 5caf7574bf
  3. laanwj added the label P2P on Apr 27, 2024
  4. DrahtBot commented at 12:33 pm on April 27, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK brunoerg, stickies-v

    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:

    • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

    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.

  5. in src/mapport.cpp:57 in 5caf7574bf
    53@@ -54,7 +54,7 @@ static bool NatpmpInit(natpmp_t* natpmp)
    54 {
    55     const int r_init = initnatpmp(natpmp, /* detect gateway automatically */ 0, /* forced gateway - NOT APPLIED*/ 0);
    56     if (r_init == 0) return true;
    57-    LogPrintf("natpmp: initnatpmp() failed with %d error.\n", r_init);
    58+    LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: initnatpmp() failed with %d error.\n", r_init);
    


    paplorinc commented at 1:03 pm on April 27, 2024:
    the message states it’s an error, but the level is warning

    laanwj commented at 1:25 pm on April 27, 2024:
    i’ve mentioned this in my post: these are intentionally logged at Warning level as port mapping failures are not generally considered very serious problems, there’s no reason to unnecessarily worry users about them
  6. in src/mapport.cpp:75 in 5caf7574bf
    71@@ -72,12 +72,12 @@ static bool NatpmpDiscover(natpmp_t* natpmp, struct in_addr& external_ipv4_addr)
    72             external_ipv4_addr = response.pnu.publicaddress.addr;
    73             return true;
    74         } else if (r_read == NATPMP_ERR_NOGATEWAYSUPPORT) {
    75-            LogPrintf("natpmp: The gateway does not support NAT-PMP.\n");
    76+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: The gateway does not support NAT-PMP.\n");
    


    paplorinc commented at 1:05 pm on April 27, 2024:
    the condition error code indicates this should also be an error
  7. in src/mapport.cpp:195 in 5caf7574bf
    191@@ -192,21 +192,21 @@ static bool ProcessUpnp()
    192 
    193             if (r != UPNPCOMMAND_SUCCESS) {
    194                 ret = false;
    195-                LogPrintf("AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", port, port, lanaddr, r, strupnperror(r));
    196+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "UPnP: AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", port, port, lanaddr, r, strupnperror(r));
    


    paplorinc commented at 1:07 pm on April 27, 2024:
    was the message changed on purpose to confirm to the rest of the logs? Shouldn’t we rather remove the prefixes now that the category is specified?

    laanwj commented at 1:27 pm on April 27, 2024:
    Yes, this was on purpose. i’ve kept the prefix because nat-pmp and UPnP messages should be distinguishable.
  8. in src/mapport.cpp:179 in 5caf7574bf
    176             } else {
    177                 if (externalIPAddress[0]) {
    178                     std::optional<CNetAddr> resolved{LookupHost(externalIPAddress, false)};
    179                     if (resolved.has_value()) {
    180-                        LogPrintf("UPnP: ExternalIPAddress = %s\n", resolved->ToStringAddr());
    181+                        LogPrintLevel(BCLog::NET, BCLog::Level::Info, "UPnP: ExternalIPAddress = %s\n", resolved->ToStringAddr());
    


    paplorinc commented at 1:12 pm on April 27, 2024:
    Some of these should probably be debugs, if they’re spammy

    laanwj commented at 1:26 pm on April 27, 2024:
    I have no reason to believe this message is spammy. Mind that it was logged as an unconditional message, and no one ever complained (this one is also reasonably important).
  9. paplorinc commented at 1:13 pm on April 27, 2024: contributor
    +1
  10. brunoerg commented at 8:45 pm on April 27, 2024: contributor
    Concept ACK
  11. stickies-v commented at 10:49 am on April 30, 2024: contributor
    Concept ACK, but it’s probably better to use the new macros introduced in #28318? Currently, the conditional macros (which this PR would need) don’t take a category parameter, but if you think that’s important perhaps it’d be good to chime in on the conversation on #29256.
  12. laanwj commented at 11:10 am on April 30, 2024: member
    i dunno, i prefer being explicit and not using macros myself TBH, what would macros add? At least more cognitive overload (“what macro to use here”). The way it’s done now, it’s clear where the messages come from and what their level is. i’m fine with changing if you really insist. But as you say, it isn’t even possible with the current ones.
  13. laanwj commented at 11:16 am on April 30, 2024: member
    Honestly, on second thought, i don’t feel like repeating this argument. i’ll do everyone a favor and close this.
  14. laanwj closed this on Apr 30, 2024

  15. stickies-v commented at 11:26 am on April 30, 2024: contributor

    i prefer being explicit and not using macros myself TBH

    LogPrintLevel is a macro too? edit: oh, I suppose you mean multiple macros

    The way it’s done now, it’s clear where the messages come from and what their level is

    What #29256 proposes is that you’d be able to do exactly that with the shorthand LogWarning(BCLog::NET, "natpmp: initnatpmp() failed with %d error.\n", r_init);, hence why I thought you’d be interested in chiming in on that pull. Apologies for derailing this PR, that was not my intent.

  16. laanwj commented at 11:46 am on April 30, 2024: member

    What #29256 proposes is that you’d be able to do exactly that with the shorthand LogWarning(BCLog::NET, “natpmp: initnatpmp() failed with %d error.\n”, r_init);, hence why I thought you’d be interested in chiming in on that pull. Apologies for derailing this PR, that was not my intent.

    Thanks, i’ll keep it in mind.

    Apologies for derailing this PR, that was not my intent.

    No need for apologies ! you’re right, i don’t know what has been discussed when i was gone, i should not be doing things like this, besides there’s more serious things to work on 😄


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-12-11 03:12 UTC

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