In commit “init: Handle dropped UPnP support more gracefully” (bd4679a608b2cbc206fa60a180b0fc4ce8a39ec4)
I think I might not understanding what this code is trying to do exactly, but it seems overcomplicated and maybe a little off. Like the check for GetSettingPath seems unnecessary, the LogWarning seems vague, the GetPersistentSetting calls not really checking for the right thing as mentioned in an earlier comment. Also the check for isTrue seems overly narrow and the UniValue(true) cast should be unnecessary. I would suggest following implementation:
0 // If settings.json contains a "upnp" option, migrate it to use "natpmp" instead
1 bool settings_changed{false}; // Whether settings.json file needs to be rewritten
2 args.LockSettings([&](common::Settings& settings) {
3 if (auto* upnp{FindKey(settings.rw_settings, "upnp")}) {
4 if (!FindKey(settings.rw_settings, "natpmp")) {
5 LogWarning(R"(Adding "natpmp": %s to settings.json to replace obsolete "upnp" setting)", upnp->write());
6 settings.rw_settings["natpmp"] = *upnp;
7 }
8 LogWarning(R"(Removing obsolete "upnp" setting from settings.json)");
9 settings.rw_settings.erase("upnp");
10 settings_changed = true;
11 }
12 });
13 if (settings_changed) args.WriteSettingsFile();
with using common::FindKey at top of the file. This seems like the most simple and direct implementation and I think comments and log messages should be a little better too.
