upnp: restart upnp on connectivity change

When receiving a 'connectivity change', UPNP search must be restarted
if the host address changes and also if there is no valid IGD.
Also reduce the number of retries on failure.

Gitlab: #615

Change-Id: I80e6d61ab79ef91f480bb0073b9cec8e7e67e559

src/upnp/protocol/natpmp/nat_pmp.cpp

@@ -552,7 +552,7 @@ NatPmp::getIgdPublicAddress()
     err = readResponse(natpmpHdl_, response);
 
     if (err < 0) {
-        JAMI_ERR("NAT-PMP: read response on IGD %s failed with error %s",
+        JAMI_WARN("NAT-PMP: Read response on IGD %s failed - %s",
                   igd_->toString().c_str(),
                   getNatPmpErrorStr(err));
         return;

src/upnp/protocol/natpmp/nat_pmp.h

@@ -51,13 +51,13 @@ namespace upnp {
 // Requested lifetime in seconds. The actual lifetime might be different.
 constexpr static unsigned int MAPPING_ALLOCATION_LIFETIME {60 * 60};
 // Max number of IGD search attempts before failure.
-constexpr static unsigned int MAX_RESTART_SEARCH_RETRIES {5};
+constexpr static unsigned int MAX_RESTART_SEARCH_RETRIES {3};
 // Time-out between two successive read response.
 constexpr static auto TIMEOUT_BEFORE_READ_RETRY {std::chrono::milliseconds(300)};
 // Max number of read attempts before failure.
-constexpr static unsigned int MAX_READ_RETRIES {5};
+constexpr static unsigned int MAX_READ_RETRIES {3};
 // Base unit for the timeout between two successive IGD search.
-constexpr static auto NATPMP_SEARCH_RETRY_UNIT {std::chrono::seconds(15)};
+constexpr static auto NATPMP_SEARCH_RETRY_UNIT {std::chrono::seconds(10)};
 
 class NatPmp : public UPnPProtocol
 {

src/upnp/protocol/pupnp/pupnp.cpp

@@ -36,6 +36,17 @@ constexpr static const char* ACTION_GET_GENERIC_PORT_MAPPING_ENTRY {"GetGenericP
 constexpr static const char* ACTION_GET_STATUS_INFO {"GetStatusInfo"};
 constexpr static const char* ACTION_GET_EXTERNAL_IP_ADDRESS {"GetExternalIPAddress"};
 
+// Error codes returned by router when trying to remove ports.
+constexpr static int ARRAY_IDX_INVALID = 713;
+constexpr static int CONFLICT_IN_MAPPING = 718;
+
+// Max number of IGD search attempts before failure.
+constexpr static unsigned int PUPNP_MAX_RESTART_SEARCH_RETRIES {3};
+// IGD search timeout (in seconds).
+constexpr static unsigned int SEARCH_TIMEOUT {60};
+// Base unit for the timeout between two successive IGD search.
+constexpr static auto PUPNP_SEARCH_RETRY_UNIT {std::chrono::seconds(10)};
+
 // Helper functions for xml parsing.
 static std::string
 getElementText(IXML_Node* node)

src/upnp/protocol/pupnp/pupnp.h

@@ -64,17 +64,6 @@ class IpAddr;
 namespace jami {
 namespace upnp {
 
-// Error codes returned by router when trying to remove ports.
-constexpr static int ARRAY_IDX_INVALID = 713;
-constexpr static int CONFLICT_IN_MAPPING = 718;
-
-// IGD search timeout (in seconds).
-constexpr static unsigned int SEARCH_TIMEOUT {60};
-// Max number of IGD search attempts before failure.
-constexpr static unsigned int PUPNP_MAX_RESTART_SEARCH_RETRIES {5};
-// Base unit for the timeout between two successive IGD search.
-constexpr static auto PUPNP_SEARCH_RETRY_UNIT {std::chrono::seconds(15)};
-
 class PUPnP : public UPnPProtocol
 {
 public:

src/upnp/upnp_context.cpp

@@ -224,8 +224,15 @@ UPnPContext::connectivityChanged()
 
     JAMI_DBG("Connectivity change check: host address %s", hostAddr.toString().c_str());
 
-    auto addrChanged = false;
+    auto restartUpnp = false;
 
+    // On reception of "connectivity change" notification, the UPNP search
+    // will be restarted if either there is no valid IGD, or the IGD address
+    // changed.
+
+    if (not isReady()) {
+        restartUpnp = true;
+    } else {
         // Check if the host address changed.
         for (auto const& [_, protocol] : protocolList_) {
             if (protocol->isReady() and hostAddr != protocol->getHostAddress()) {
@@ -233,16 +240,19 @@ UPnPContext::connectivityChanged()
                           protocol->getHostAddress().toString().c_str(),
                           hostAddr.toString().c_str());
                 protocol->clearIgds();
-            addrChanged = true;
+                restartUpnp = true;
+                break;
+            }
         }
     }
 
-    // Address did not change, nothing to do.
-    if (not addrChanged) {
+    // We have at least one valid IGD and the host address did
+    // not change, so no need to restart.
+    if (not restartUpnp) {
         return;
     }
 
-    // No registered controller. New search will be performed when
+    // No registered controller. A new search will be performed when
     // a controller is registered.
     if (controllerList_.empty())
         return;