net: Modernize logging in UPnP and nat-pmp code

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.

net: Modernize logging in UPnP and nat-pmp code 5caf7574bf

laanwj added the label P2P on Apr 27, 2024

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.

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

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

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.

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

paplorinc commented at 1:13 pm on April 27, 2024: contributor

+1

brunoerg commented at 8:45 pm on April 27, 2024: contributor

Concept ACK

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.

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.

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.

laanwj closed this on Apr 30, 2024

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.

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 😄

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

Code Coverage

Reviews

Conflicts