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

laanwj · 2024-04-27T12:33:11Z

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.

DrahtBot · 2024-04-27T12:33:13Z

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.

paplorinc

+1

paplorinc · 2024-04-27T13:03:48Z

src/mapport.cpp

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


the message states it's an error, but the level is warning

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

paplorinc · 2024-04-27T13:05:02Z

src/mapport.cpp

@@ -72,12 +72,12 @@ static bool NatpmpDiscover(natpmp_t* natpmp, struct in_addr& external_ipv4_addr)
            external_ipv4_addr = response.pnu.publicaddress.addr;
            return true;
        } else if (r_read == NATPMP_ERR_NOGATEWAYSUPPORT) {
-            LogPrintf("natpmp: The gateway does not support NAT-PMP.\n");
+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: The gateway does not support NAT-PMP.\n");


the condition error code indicates this should also be an error

paplorinc · 2024-04-27T13:07:52Z

src/mapport.cpp

@@ -192,21 +192,21 @@ static bool ProcessUpnp()

            if (r != UPNPCOMMAND_SUCCESS) {
                ret = false;
-                LogPrintf("AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", port, port, lanaddr, r, strupnperror(r));
+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "UPnP: AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", port, port, lanaddr, r, strupnperror(r));


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?

Yes, this was on purpose. i've kept the prefix because nat-pmp and UPnP messages should be distinguishable.

paplorinc · 2024-04-27T13:12:21Z

src/mapport.cpp

            } else {
                if (externalIPAddress[0]) {
                    std::optional<CNetAddr> resolved{LookupHost(externalIPAddress, false)};
                    if (resolved.has_value()) {
-                        LogPrintf("UPnP: ExternalIPAddress = %s\n", resolved->ToStringAddr());
+                        LogPrintLevel(BCLog::NET, BCLog::Level::Info, "UPnP: ExternalIPAddress = %s\n", resolved->ToStringAddr());


Some of these should probably be debugs, if they're spammy

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).

brunoerg · 2024-04-27T20:45:02Z

Concept ACK

stickies-v

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.

laanwj · 2024-04-30T11:10:13Z

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.

laanwj · 2024-04-30T11:16:54Z

Honestly, on second thought, i don't feel like repeating this argument. i'll do everyone a favor and close this.

stickies-v · 2024-04-30T11:26:51Z

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.

laanwj · 2024-04-30T11:46:14Z

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 😄

net: Modernize logging in UPnP and nat-pmp code

5caf757

laanwj added the P2P label Apr 27, 2024

paplorinc reviewed Apr 27, 2024

View reviewed changes

DrahtBot mentioned this pull request Apr 28, 2024

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

stickies-v reviewed Apr 30, 2024

View reviewed changes

laanwj closed this Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laanwj commented Apr 27, 2024

DrahtBot commented Apr 27, 2024 •

edited

paplorinc left a comment

paplorinc Apr 27, 2024

laanwj Apr 27, 2024

paplorinc Apr 27, 2024

paplorinc Apr 27, 2024

laanwj Apr 27, 2024

paplorinc Apr 27, 2024

laanwj Apr 27, 2024

brunoerg commented Apr 27, 2024

stickies-v left a comment

laanwj commented Apr 30, 2024 •

edited

laanwj commented Apr 30, 2024 •

edited

stickies-v commented Apr 30, 2024 •

edited

laanwj commented Apr 30, 2024

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

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

Conversation

laanwj commented Apr 27, 2024

DrahtBot commented Apr 27, 2024 • edited

Code Coverage

Reviews

Conflicts

paplorinc left a comment

Choose a reason for hiding this comment

paplorinc Apr 27, 2024

Choose a reason for hiding this comment

laanwj Apr 27, 2024

Choose a reason for hiding this comment

paplorinc Apr 27, 2024

Choose a reason for hiding this comment

paplorinc Apr 27, 2024

Choose a reason for hiding this comment

laanwj Apr 27, 2024

Choose a reason for hiding this comment

paplorinc Apr 27, 2024

Choose a reason for hiding this comment

laanwj Apr 27, 2024

Choose a reason for hiding this comment

brunoerg commented Apr 27, 2024

stickies-v left a comment

Choose a reason for hiding this comment

laanwj commented Apr 30, 2024 • edited

laanwj commented Apr 30, 2024 • edited

stickies-v commented Apr 30, 2024 • edited

laanwj commented Apr 30, 2024

DrahtBot commented Apr 27, 2024 •

edited

laanwj commented Apr 30, 2024 •

edited

laanwj commented Apr 30, 2024 •

edited

stickies-v commented Apr 30, 2024 •

edited