From 17c6ec755c5273d7f44f4b3097335d30f5c24eee Mon Sep 17 00:00:00 2001
From: Yat Ho <lagoho7@gmail.com>
Date: Sun, 26 May 2024 06:08:16 +0800
Subject: [PATCH] feat: allow port forwarding state to recover from error
 (#6718)

* feat: allow upnp to recover from errors

* feat: allow natpmp to recover from errors

* chore: housekeeping

* code review: explicitly list all states to start discovering from

* fix: recover from failed UPnP discovery

* refactor: remove `UpnpState::Failed`
---
 libtransmission/port-forwarding-natpmp.cc | 12 +++---------
 libtransmission/port-forwarding-upnp.cc   | 18 +++++++-----------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/libtransmission/port-forwarding-natpmp.cc b/libtransmission/port-forwarding-natpmp.cc
index 28ddad625..7d4d0054c 100644
--- a/libtransmission/port-forwarding-natpmp.cc
+++ b/libtransmission/port-forwarding-natpmp.cc
@@ -140,16 +140,10 @@ tr_natpmp::PulseResult tr_natpmp::pulse(tr_port local_port, bool is_enabled)
         }
     }
 
-    if (state_ == State::Idle)
+    if ((state_ == State::Idle || state_ == State::Err) &&
+        (is_mapped_ ? tr_time() >= renew_time_ : is_enabled && has_discovered_))
     {
-        if (is_enabled && !is_mapped_ && has_discovered_)
-        {
-            state_ = State::SendMap;
-        }
-        else if (is_mapped_ && tr_time() >= renew_time_)
-        {
-            state_ = State::SendMap;
-        }
+        state_ = State::SendMap;
     }
 
     if (state_ == State::SendMap && canSendCommand())
diff --git a/libtransmission/port-forwarding-upnp.cc b/libtransmission/port-forwarding-upnp.cc
index 26041b99d..15c248c5d 100644
--- a/libtransmission/port-forwarding-upnp.cc
+++ b/libtransmission/port-forwarding-upnp.cc
@@ -39,7 +39,6 @@ namespace
 enum class UpnpState : uint8_t
 {
     Idle,
-    Failed,
     WillDiscover, // next action is upnpDiscover()
     Discovering, // currently making blocking upnpDiscover() call in a worker thread
     WillMap, // next action is UPNP_AddPortMapping()
@@ -58,9 +57,7 @@ struct tr_upnp
     ~tr_upnp()
     {
         TR_ASSERT(!isMapped);
-        TR_ASSERT(
-            state == UpnpState::Idle || state == UpnpState::Failed || state == UpnpState::WillDiscover ||
-            state == UpnpState::Discovering);
+        TR_ASSERT(state == UpnpState::Idle || state == UpnpState::WillDiscover || state == UpnpState::Discovering);
 
         FreeUPNPUrls(&urls);
     }
@@ -298,7 +295,7 @@ tr_port_forwarding_state tr_upnpPulse(
         }
         else
         {
-            handle->state = UpnpState::Failed;
+            handle->state = UpnpState::WillDiscover;
             tr_logAddDebug(fmt::format("UPNP_GetValidIGD failed: {} ({})", tr_strerror(errno), errno));
             tr_logAddDebug("If your router supports UPnP, please make sure UPnP is enabled!");
         }
@@ -306,15 +303,15 @@ tr_port_forwarding_state tr_upnpPulse(
         freeUPNPDevlist(devlist);
     }
 
-    if ((handle->state == UpnpState::Idle) && (handle->isMapped) &&
+    if (handle->state == UpnpState::Idle && handle->isMapped &&
         (!is_enabled || handle->advertised_port != advertised_port || handle->local_port != local_port))
     {
         handle->state = UpnpState::WillUnmap;
     }
 
     if (is_enabled && handle->isMapped && do_port_check &&
-        ((get_specific_port_mapping_entry(handle, "TCP") != UPNPCOMMAND_SUCCESS) ||
-         (get_specific_port_mapping_entry(handle, "UDP") != UPNPCOMMAND_SUCCESS)))
+        (get_specific_port_mapping_entry(handle, "TCP") != UPNPCOMMAND_SUCCESS ||
+         get_specific_port_mapping_entry(handle, "UDP") != UPNPCOMMAND_SUCCESS))
     {
         tr_logAddInfo(fmt::format(
             _("Local port {local_port} is not forwarded to {advertised_port}"),
@@ -339,7 +336,7 @@ tr_port_forwarding_state tr_upnpPulse(
         handle->local_port = {};
     }
 
-    if ((handle->state == UpnpState::Idle) && is_enabled && !handle->isMapped)
+    if (handle->state == UpnpState::Idle && is_enabled && !handle->isMapped)
     {
         handle->state = UpnpState::WillMap;
     }
@@ -376,15 +373,14 @@ tr_port_forwarding_state tr_upnpPulse(
                 fmt::arg("advertised_port", advertised_port.host())));
             handle->advertised_port = advertised_port;
             handle->local_port = local_port;
-            handle->state = UpnpState::Idle;
         }
         else
         {
             tr_logAddInfo(_("If your router supports UPnP, please make sure UPnP is enabled!"));
             handle->advertised_port = {};
             handle->local_port = {};
-            handle->state = UpnpState::Failed;
         }
+        handle->state = UpnpState::Idle;
     }
 
     return port_fwd_state(handle->state, handle->isMapped);