From 13a365cbd30e88d1a0503358e026fa6c3e68c273 Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Fri, 12 Feb 2021 13:28:34 -0500
Subject: [PATCH] upnp: improve IGD search
This patch introduces the following modifcations:
1- The search for IGD and all requests will be performed only
if the local address is valid.
2- A new search retry mechanism was introduced (similar to nat-pmp)
to improve resilience against transient failures, in particular
during a connectivity change. The search is perfomed a pre-defined
number of times before it is definitely set as failed.
3- The device UID was used to identify the IGD. However, some
devices may have more than one IGD with the same device ID, which
caused some of IGDs to wrongly ignored. This is problematic if the
first IGD with the same device ID that was added does not pass
the validation or does not respond successfully to mapping
requests. Now, we use both the deviceId and the URL as identifier
for the discoverd IGDs.
4- Mapping address management was simplified.
5- ICE will only considere mappings that have valid local address.
Gitlab: #420
Change-Id: I7eed337e9c1a190f6c42ebb1381957660f604410
---
src/account.cpp | 2 +-
src/ice_transport.cpp | 11 +-
src/jamidht/jamiaccount.cpp | 2 +-
src/upnp/protocol/mapping.cpp | 121 ++++++++++--------
src/upnp/protocol/mapping.h | 39 ++----
src/upnp/protocol/natpmp/nat_pmp.cpp | 33 +++--
src/upnp/protocol/natpmp/nat_pmp.h | 2 +-
src/upnp/protocol/pupnp/pupnp.cpp | 138 ++++++++++++++------
src/upnp/protocol/pupnp/pupnp.h | 20 ++-
src/upnp/protocol/upnp_protocol.h | 8 +-
src/upnp/upnp_context.cpp | 184 ++++++++++++---------------
src/upnp/upnp_context.h | 2 +-
src/upnp/upnp_control.cpp | 6 +-
src/upnp/upnp_control.h | 2 +-
14 files changed, 318 insertions(+), 252 deletions(-)
diff --git a/src/account.cpp b/src/account.cpp
index b9a70ae8dd..a391b395f5 100644
--- a/src/account.cpp
+++ b/src/account.cpp
@@ -543,7 +543,7 @@ Account::getUPnPActive() const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
if (upnpCtrl_)
- return upnpCtrl_->hasValidIGD();
+ return upnpCtrl_->isReady();
return false;
}
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 146eaceae8..cabb535ab0 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -552,9 +552,9 @@ IceTransport::Impl::handleEvents(unsigned max_msec)
void
IceTransport::Impl::onComplete(pj_ice_strans* ice_st, pj_ice_strans_op op, pj_status_t status)
{
- const char* opname = op == PJ_ICE_STRANS_OP_INIT ? "initialization"
- : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation"
- : "unknown_op";
+ const char* opname = op == PJ_ICE_STRANS_OP_INIT
+ ? "initialization"
+ : op == PJ_ICE_STRANS_OP_NEGOTIATION ? "negotiation" : "unknown_op";
const bool done = status == PJ_SUCCESS;
if (done) {
@@ -793,8 +793,9 @@ IceTransport::Impl::requestUpnpMappings()
// Request the mapping
Mapping::sharedPtr_t mapPtr = upnp_->reserveMapping(requestedMap);
- // To use a mapping, it must be valid and open.
- if (mapPtr and mapPtr->getMapKey() and (mapPtr->getState() == MappingState::OPEN)) {
+ // To use a mapping, it must be valid, open and has valid host address.
+ if (mapPtr and mapPtr->getMapKey() and (mapPtr->getState() == MappingState::OPEN)
+ and mapPtr->hasValidHostAddress()) {
std::lock_guard<std::mutex> lock(upnpMappingsMutex_);
auto ret = upnpMappings_.emplace(mapPtr->getMapKey(), *mapPtr);
if (ret.second) {
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index a111671c3f..721838f0bc 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -3376,7 +3376,7 @@ JamiAccount::storeActiveIpAddress()
publicAddr.toString().c_str());
// Check if we need to create a new UPNP controller instance.
- if (upnpEnabled_ and (not upnpCtrl_ or not upnpCtrl_->hasValidIGD())) {
+ if (upnpEnabled_ and (not upnpCtrl_ or not upnpCtrl_->isReady())) {
enableUpnp(true);
}
}
diff --git a/src/upnp/protocol/mapping.cpp b/src/upnp/protocol/mapping.cpp
index 51fa3522a1..0ede147072 100644
--- a/src/upnp/protocol/mapping.cpp
+++ b/src/upnp/protocol/mapping.cpp
@@ -26,8 +26,9 @@ namespace jami {
namespace upnp {
Mapping::Mapping(uint16_t portExternal, uint16_t portInternal, PortType type, bool available)
- : externalAddr_()
- , internalAddr_()
+ : internalAddr_()
+ , internalPort_(portInternal)
+ , externalPort_(portExternal)
, type_(type)
, igd_()
, available_(available)
@@ -38,32 +39,15 @@ Mapping::Mapping(uint16_t portExternal, uint16_t portInternal, PortType type, bo
#if HAVE_LIBNATPMP
, renewalTime_(sys_clock::now())
#endif
-{
- externalAddr_.port_ = portExternal;
- internalAddr_.port_ = portInternal;
-}
-
-Mapping::Mapping(Mapping&& other) noexcept
- : externalAddr_(std::move(other.externalAddr_))
- , internalAddr_(std::move(other.internalAddr_))
- , type_(other.type_)
- , igd_(other.igd_)
- , available_(other.available_)
- , state_(other.state_)
- , notifyCb_(std::move(other.notifyCb_))
- , timeoutTimer_(std::move(other.timeoutTimer_))
- , autoUpdate_(other.autoUpdate_)
-#if HAVE_LIBNATPMP
- , renewalTime_(other.renewalTime_)
-#endif
{}
Mapping::Mapping(const Mapping& other)
{
std::lock_guard<std::mutex> lock(mutex_);
- externalAddr_ = other.externalAddr_;
internalAddr_ = other.internalAddr_;
+ internalPort_ = other.internalPort_;
+ externalPort_ = other.externalPort_;
type_ = other.type_;
igd_ = other.igd_;
available_ = other.available_;
@@ -82,8 +66,9 @@ Mapping::operator=(Mapping&& other) noexcept
std::lock_guard<std::mutex> lock(mutex_);
if (this != &other) {
- externalAddr_ = other.externalAddr_;
internalAddr_ = other.internalAddr_;
+ internalPort_ = other.internalPort_;
+ externalPort_ = other.externalPort_;
type_ = other.type_;
igd_ = other.igd_;
other.igd_.reset();
@@ -109,16 +94,16 @@ Mapping::operator==(const Mapping& other) const noexcept
{
std::lock_guard<std::mutex> lock(mutex_);
- if (externalAddr_.addr_ != other.externalAddr_.addr_)
- return false;
- if (externalAddr_.port_ != other.externalAddr_.port_)
+ if (internalAddr_ != other.internalAddr_)
return false;
- if (internalAddr_.addr_ != other.internalAddr_.addr_)
+ if (internalPort_ != other.internalPort_)
return false;
- if (internalAddr_.port_ != other.internalAddr_.port_)
+ if (externalPort_ != other.externalPort_)
return false;
if (type_ != other.type_)
return false;
+ if (igd_ != other.igd_)
+ return false;
return true;
}
@@ -128,6 +113,20 @@ Mapping::operator!=(const Mapping& other) const noexcept
return not(*this == other);
}
+void
+Mapping::updateFrom(const Mapping& other)
+{
+ if (internalPort_ != other.internalPort_ or type_ != other.type_) {
+ JAMI_ERR("The source and destination mappings must match (same type and port)");
+ assert(false);
+ }
+
+ internalAddr_ = std::move(other.internalAddr_);
+ externalPort_ = other.externalPort_;
+ igd_ = other.igd_;
+ state_ = other.state_;
+}
+
void
Mapping::setAvailable(bool val)
{
@@ -171,7 +170,7 @@ Mapping::toString(bool addState) const
std::lock_guard<std::mutex> lock(mutex_);
std::ostringstream descr;
descr << UPNP_MAPPING_DESCRIPTION_PREFIX << "-" << getTypeStr(type_);
- descr << ":" << std::to_string(externalAddr_.port_);
+ descr << ":" << std::to_string(internalPort_);
if (addState)
descr << " (state=" << getStateStr(state_) << ")";
@@ -182,16 +181,41 @@ Mapping::toString(bool addState) const
bool
Mapping::isValid() const
{
- std::lock_guard<std::mutex> lock(mutex_);
- if (externalAddr_.port_ == 0)
- return false;
- if (internalAddr_.port_ == 0)
- return false;
- if (state_ == MappingState::FAILED)
+ {
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (state_ == MappingState::FAILED)
+ return false;
+ if (internalPort_ == 0)
+ return false;
+ if (externalPort_ == 0)
+ return false;
+ if (not igd_ or not igd_->isValid())
+ return false;
+ }
+
+ if (not hasValidHostAddress())
return false;
+
return true;
}
+bool
+Mapping::hasValidHostAddress() const
+{
+ std::lock_guard<std::mutex> lock(mutex_);
+
+ IpAddr intAddr(internalAddr_);
+ return intAddr and not intAddr.isLoopback();
+}
+
+bool
+Mapping::hasPublicAddress() const
+{
+ std::lock_guard<std::mutex> lock(mutex_);
+
+ return igd_ and igd_->getPublicIp() and not igd_->getPublicIp().isPrivate();
+}
+
void
Mapping::setTimeoutTimer(std::shared_ptr<Task> timer)
{
@@ -218,7 +242,7 @@ Mapping::getMapKey() const
{
std::lock_guard<std::mutex> lock(mutex_);
- key_t mapKey = externalAddr_.port_;
+ key_t mapKey = internalPort_;
if (type_ == PortType::UDP)
mapKey |= 1 << (sizeof(uint16_t) * 8);
return mapKey;
@@ -230,74 +254,69 @@ Mapping::getTypeFromMapKey(key_t key)
return (key >> (sizeof(uint16_t) * 8)) ? PortType::UDP : PortType::TCP;
}
-void
-Mapping::setExternalAddress(const std::string& addr)
-{
- std::lock_guard<std::mutex> lock(mutex_);
- externalAddr_.addr_ = addr;
-}
-
std::string
Mapping::getExternalAddress() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return externalAddr_.addr_;
+ if (igd_)
+ return igd_->getPublicIp().toString();
+ return {};
}
void
Mapping::setExternalPort(uint16_t port)
{
std::lock_guard<std::mutex> lock(mutex_);
- externalAddr_.port_ = port;
+ externalPort_ = port;
}
uint16_t
Mapping::getExternalPort() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return externalAddr_.port_;
+ return externalPort_;
}
std::string
Mapping::getExternalPortStr() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return std::to_string(externalAddr_.port_);
+ return std::to_string(externalPort_);
}
void
Mapping::setInternalAddress(const std::string& addr)
{
std::lock_guard<std::mutex> lock(mutex_);
- internalAddr_.addr_ = addr;
+ internalAddr_ = addr;
}
std::string
Mapping::getInternalAddress() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return internalAddr_.addr_;
+ return internalAddr_;
}
void
Mapping::setInternalPort(uint16_t port)
{
std::lock_guard<std::mutex> lock(mutex_);
- internalAddr_.port_ = port;
+ internalPort_ = port;
}
uint16_t
Mapping::getInternalPort() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return internalAddr_.port_;
+ return internalPort_;
}
std::string
Mapping::getInternalPortStr() const
{
std::lock_guard<std::mutex> lock(mutex_);
- return std::to_string(internalAddr_.port_);
+ return std::to_string(internalPort_);
}
PortType
diff --git a/src/upnp/protocol/mapping.h b/src/upnp/protocol/mapping.h
index 25bbfe3f49..694784e5b2 100644
--- a/src/upnp/protocol/mapping.h
+++ b/src/upnp/protocol/mapping.h
@@ -58,31 +58,6 @@ public:
using sharedPtr_t = std::shared_ptr<Mapping>;
using NotifyCallback = std::function<void(sharedPtr_t)>;
- struct MappingAddr
- {
- std::string addr_ {};
- uint16_t port_ {0};
- MappingAddr()
- : addr_()
- , port_(0)
- {}
-
- MappingAddr(const MappingAddr& other)
- {
- addr_ = other.addr_;
- port_ = other.port_;
- }
-
- MappingAddr& operator=(const MappingAddr& other)
- {
- if (this != &other) {
- addr_ = other.addr_;
- port_ = other.port_;
- }
- return *this;
- }
- };
-
static constexpr char const* MAPPING_STATE_STR[4] {"PENDING", "IN_PROGRESS", "FAILED", "OPEN"};
static constexpr char const* UPNP_MAPPING_DESCRIPTION_PREFIX {"JAMI"};
@@ -90,8 +65,8 @@ public:
uint16_t portInternal = 0,
PortType type = PortType::UDP,
bool available = true);
- Mapping(Mapping&& other) noexcept;
Mapping(const Mapping& other);
+ Mapping(Mapping&& other) = delete;
~Mapping() = default;
Mapping& operator=(Mapping&& other) noexcept;
@@ -106,6 +81,7 @@ public:
inline explicit operator bool() const { return isValid(); }
+ void updateFrom(const Mapping& other);
std::string getExternalAddress() const;
uint16_t getExternalPort() const;
std::string getExternalPortStr() const;
@@ -127,6 +103,8 @@ public:
}
std::string toString(bool addState = false) const;
bool isValid() const;
+ bool hasValidHostAddress() const;
+ bool hasPublicAddress() const;
void setNotifyCallback(NotifyCallback cb);
void enableAutoUpdate(bool enable);
bool getAutoUpdate() const;
@@ -138,10 +116,8 @@ public:
private:
NotifyCallback getNotifyCallback() const;
- // TODO_MC. Remove SetXXadress. already have IGD. Set ports instead.
- void setExternalAddress(const std::string& addr);
- void setExternalPort(uint16_t port);
void setInternalAddress(const std::string& addr);
+ void setExternalPort(uint16_t port);
void setInternalPort(uint16_t port);
void setIgd(const std::shared_ptr<IGD>& igd);
@@ -155,8 +131,9 @@ private:
#endif
mutable std::mutex mutex_;
- MappingAddr externalAddr_;
- MappingAddr internalAddr_;
+ std::string internalAddr_;
+ uint16_t internalPort_ {0};
+ uint16_t externalPort_ {0};
PortType type_;
// Protocol and
std::shared_ptr<IGD> igd_;
diff --git a/src/upnp/protocol/natpmp/nat_pmp.cpp b/src/upnp/protocol/natpmp/nat_pmp.cpp
index 37ea9fa4ab..6a58e7af12 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.cpp
+++ b/src/upnp/protocol/natpmp/nat_pmp.cpp
@@ -20,6 +20,8 @@
#include "nat_pmp.h"
+#if HAVE_LIBNATPMP
+
namespace jami {
namespace upnp {
@@ -46,10 +48,16 @@ NatPmp::initNatPmp()
CHECK_VALID_THREAD();
- userLocalIp_ = ip_utils::getLocalAddr(pj_AF_INET());
initialized_ = false;
- assert(igd_);
+ hostAddress_ = ip_utils::getLocalAddr(pj_AF_INET());
+
+ // Local address must be valid.
+ if (not getHostAddress() or getHostAddress().isLoopback()) {
+ JAMI_WARN("NAT-PMP: Does not have a valid local address!");
+ return;
+ }
+ assert(igd_);
if (igd_->isValid()) {
igd_->setValid(false);
processIgdUpdate(UpnpIgdEvent::REMOVED);
@@ -91,6 +99,9 @@ NatPmp::initNatPmp()
// Set the local (gateway) address.
igd_->setLocalIp(igdAddr);
+ // NAT-PMP protocol does not have UID, but we will set generic
+ // one debugging purposes.
+ igd_->setUID("NAT-PMP Gateway");
// Search and set the public address.
getIgdPublicAddress();
@@ -143,14 +154,6 @@ NatPmp::searchForIgd()
initNatPmp();
}
- if (initialized_ and not hasValidIgd()) {
- // Log current IGD.
- JAMI_DBG("NAT-PMP: Current top IGD [%s:%s] valid %s",
- igd_->getLocalIp().toString().c_str(),
- igd_->getLocalIp().toString().c_str(),
- igd_->isValid() ? "YES" : "NO");
- }
-
// Schedule a retry in case init failed.
if (not initialized_) {
if (igdSearchCounter_++ < MAX_RESTART_SEARCH_RETRIES) {
@@ -175,8 +178,11 @@ NatPmp::getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const
}
bool
-NatPmp::hasValidIgd() const
+NatPmp::isReady() const
{
+ // Must at least have a valid local address.
+ if (not getHostAddress() or getHostAddress().isLoopback())
+ return false;
return igd_ and igd_->isValid();
}
@@ -332,8 +338,7 @@ NatPmp::addPortMapping(const std::shared_ptr<IGD>& igdIn, Mapping& mapping, bool
Mapping mapToAdd(mapping.getExternalPort(),
mapping.getInternalPort(),
mapping.getType() == PortType::UDP ? upnp::PortType::UDP : upnp::PortType::TCP);
- mapToAdd.setExternalAddress(igd_->getPublicIp().toString());
- mapToAdd.setInternalAddress(getUserLocalIp().toString());
+ mapToAdd.setInternalAddress(getHostAddress().toString());
mapToAdd.setIgd(igd_);
uint32_t lifetime = MAPPING_ALLOCATION_LIFETIME;
@@ -639,3 +644,5 @@ NatPmp::processMappingRemoved(const Mapping& map)
} // namespace upnp
} // namespace jami
+
+#endif //-- #if HAVE_LIBNATPMP
diff --git a/src/upnp/protocol/natpmp/nat_pmp.h b/src/upnp/protocol/natpmp/nat_pmp.h
index 2df50e4a69..ddf0a7ea8b 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.h
+++ b/src/upnp/protocol/natpmp/nat_pmp.h
@@ -83,7 +83,7 @@ public:
void getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const override;
// Return true if it has at least one valid IGD.
- bool hasValidIgd() const override;
+ bool isReady() const override;
// Increment errors counter.
void incrementErrorsCounter(const std::shared_ptr<IGD>& igd) override;
diff --git a/src/upnp/protocol/pupnp/pupnp.cpp b/src/upnp/protocol/pupnp/pupnp.cpp
index 2bedf3ad26..cf6334101c 100644
--- a/src/upnp/protocol/pupnp/pupnp.cpp
+++ b/src/upnp/protocol/pupnp/pupnp.cpp
@@ -94,11 +94,12 @@ errorOnResponse(IXML_Document* doc)
return false;
}
-
// UPNP class implementation
PUPnP::PUPnP()
{
+ threadId_ = getCurrentThread();
+
if (not initUpnpLib())
return;
if (not registerClient())
@@ -249,10 +250,12 @@ PUPnP::setObserver(UpnpMappingObserver* obs)
void
PUPnP::searchForDevices()
{
- userLocalIp_ = ip_utils::getLocalAddr(pj_AF_INET());
+ std::unique_lock<std::mutex> lk(ctrlptMutex_);
+
+ JAMI_DBG("PUPnP: Send IGD search request");
+
// Send out search for multiple types of devices, as some routers may possibly
// only reply to one.
- std::unique_lock<std::mutex> lk(ctrlptMutex_);
UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_ROOT_DEVICE, this);
UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_IGD_DEVICE, this);
UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANIP_SERVICE, this);
@@ -264,6 +267,15 @@ PUPnP::clearIgds()
{
JAMI_DBG("PUPnP: clearing IGDs and devices lists");
+ CHECK_VALID_THREAD();
+
+ hostAddress_ = {};
+
+ if (searchForIgdTimer_)
+ searchForIgdTimer_->cancel();
+
+ igdSearchCounter_ = 0;
+
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
validIgdList_.clear();
@@ -281,9 +293,30 @@ PUPnP::clearIgds()
void
PUPnP::searchForIgd()
{
- // Notify thread to execute in non-blocking fashion.
- searchForIgd_ = true;
- pupnpCv_.notify_one();
+ CHECK_VALID_THREAD();
+
+ // Update local address before searching.
+ hostAddress_ = ip_utils::getLocalAddr(pj_AF_INET());
+
+ if (not isReady()) {
+ if (igdSearchCounter_++ < PUPNP_MAX_RESTART_SEARCH_RETRIES) {
+ JAMI_DBG("PUPnP: Start search for IGD: attempt %u", igdSearchCounter_);
+
+ searchForIgd_ = true;
+ pupnpCv_.notify_one();
+
+ // Cancel the current timer (if any) and re-schedule.
+ if (searchForIgdTimer_)
+ searchForIgdTimer_->cancel();
+
+ searchForIgdTimer_ = getUpnContextScheduler()
+ ->scheduleIn([this] { searchForIgd(); },
+ PUPNP_TIMEOUT_BEFORE_IGD_SEARCH_RETRY);
+ } else {
+ JAMI_WARN("PUPnP: Setup failed after %u trials. PUPnP will be disabled!",
+ PUPNP_MAX_RESTART_SEARCH_RETRIES);
+ }
+ }
}
void
@@ -298,6 +331,16 @@ PUPnP::getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const
}
}
+bool
+PUPnP::isReady() const
+{
+ // Must at least have a valid local address.
+ if (not getHostAddress() or getHostAddress().isLoopback())
+ return false;
+
+ return hasValidIgd();
+}
+
bool
PUPnP::hasValidIgd() const
{
@@ -309,6 +352,17 @@ PUPnP::hasValidIgd() const
return false;
}
+bool
+PUPnP::updateAndCheckHostAddress()
+{
+ if (hostAddress_ and not hostAddress_.isLoopback())
+ return true;
+
+ hostAddress_ = ip_utils::getLocalAddr(pj_AF_INET());
+
+ return hostAddress_ and not hostAddress_.isLoopback();
+}
+
void
PUPnP::incrementErrorsCounter(const std::shared_ptr<IGD>& igd)
{
@@ -344,8 +398,7 @@ PUPnP::validateIgd(const IGDInfo& info)
return false;
}
- JAMI_DBG("PUPnP: Validating IGD candidate\n"
- " UDN : %s\n"
+ JAMI_DBG("PUPnP: Validating the IGD candidate [UDN: %s]\n"
" Name : %s\n"
" Service Type : %s\n"
" Service ID : %s\n"
@@ -404,7 +457,8 @@ PUPnP::validateIgd(const IGDInfo& info)
// Must not be a null pointer
assert(igd.get() != nullptr);
if (*igd == *igd_candidate) {
- JAMI_DBG("PUPnP: Device [%s] with int/ext addresses [%s:%s] is already in the list",
+ JAMI_DBG("PUPnP: Device [%s] with int/ext addresses [%s:%s] is already in the list "
+ "of valid IGDs",
igd_candidate->getUID().c_str(),
igd_candidate->getLocalIp().toString().c_str(),
igd_candidate->getPublicIp().toString().c_str());
@@ -413,8 +467,10 @@ PUPnP::validateIgd(const IGDInfo& info)
}
}
- JAMI_DBG("PUPnP: Found new valid IGD [%s] with int/ext addresses [%s:%s]",
- igd_candidate->getUID().c_str(),
+ JAMI_DBG("PUPnP: Added a new IGD [%s] to the list of valid IGDs",
+ igd_candidate->getUID().c_str());
+
+ JAMI_DBG("PUPnP: New IGD addresses [int: %s - ext: %s]",
igd_candidate->getLocalIp().toString().c_str(),
igd_candidate->getPublicIp().toString().c_str());
@@ -448,7 +504,6 @@ void
PUPnP::requestMappingAdd(const std::shared_ptr<IGD>& igd, const Mapping& mapping)
{
if (auto pupnp_igd = std::dynamic_pointer_cast<const UPnPIGD>(igd)) {
- JAMI_DBG("PUPnP: Attempting to open port %s", mapping.toString().c_str());
actionAddPortMappingAsync(*pupnp_igd, mapping);
}
}
@@ -486,7 +541,7 @@ PUPnP::actionAddPortMappingAsync(const UPnPIGD& igd, const Mapping& mapping)
ACTION_ADD_PORT_MAPPING,
igd.getServiceType().c_str(),
"NewInternalClient",
- getUserLocalIp().toString().c_str());
+ getHostAddress().toString().c_str());
UpnpAddToAction(&action_container_ptr,
ACTION_ADD_PORT_MAPPING,
igd.getServiceType().c_str(),
@@ -537,8 +592,7 @@ PUPnP::processAddMapAction(const std::string& ctrlURL,
for (auto const& it : validIgdList_) {
if (auto igd = std::dynamic_pointer_cast<UPnPIGD>(it)) {
if (igd->getControlURL() == ctrlURL) {
- mapToAdd.setExternalAddress(igd->getPublicIp().toString());
- mapToAdd.setInternalAddress(getUserLocalIp().toString());
+ mapToAdd.setInternalAddress(getHostAddress().toString());
mapToAdd.setIgd(igd);
break;
}
@@ -590,9 +644,6 @@ PUPnP::requestMappingRemove(const Mapping& igdMapping)
// Iterate over all IGDs in internal list and try to remove selected mapping.
for (auto const& it : validIgdList_) {
if (auto igd = std::dynamic_pointer_cast<UPnPIGD>(it)) {
- JAMI_DBG("PUPnP: Attempting to close port %s %s",
- igdMapping.getExternalPortStr().c_str(),
- igdMapping.getTypeStr());
std::lock_guard<std::mutex> lk2(ctrlptMutex_);
actionDeletePortMappingAsync(*igd,
igdMapping.getExternalPortStr(),
@@ -676,16 +727,25 @@ PUPnP::processDiscoverySearchResult(const std::string& cpDeviceId,
const std::string& igdLocationUrl,
const IpAddr& dstAddr)
{
+ // Must first have a valid local address.
+ if (not updateAndCheckHostAddress()) {
+ JAMI_WARN("PUPnP: Local address is invalid. Ignore search result for now!");
+ return;
+ }
+
dht::http::Url url(igdLocationUrl);
std::lock_guard<std::mutex> lk(igdListMutex_);
- if (not discoveredIgdList_.emplace(cpDeviceId).second)
+ // Use the device ID and the URL as ID. This is necessary as some
+ // IGDs may have the same device ID but different URLs.
+
+ auto igdId = cpDeviceId + " url: " + igdLocationUrl;
+
+ if (not discoveredIgdList_.emplace(igdId).second)
return;
- JAMI_DBG("PUPnP: Added IGD %s [%s] to known IGD list",
- cpDeviceId.c_str(),
- IpAddr(url.host).toString(true, true).c_str());
+ JAMI_DBG("PUPnP: Discovered a new IGD [%s]", igdId.c_str());
// NOTE: here, we check if the location given is related to the source address.
// If it's not the case, it's certainly a router plugged in the network, but not
@@ -698,8 +758,6 @@ PUPnP::processDiscoverySearchResult(const std::string& cpDeviceId,
IpAddr(url.host).toString(true, true).c_str(),
dstAddr.toString(true, true).c_str());
return;
- } else {
- JAMI_DBG("PUPnP: Returned IGD location %s", IpAddr(url.host).toString(true, true).c_str());
}
dwnldlXmlList_.emplace_back(
@@ -834,11 +892,14 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
break;
}
case UPNP_DISCOVERY_SEARCH_TIMEOUT: {
- // Nothing to do here.
+ // Even if the discovery search is successful, it's normal to receive
+ // time-out events. This because we send search requests using various
+ // device types, which some of them may not return a response.
+ JAMI_DBG("PUPnP: Received discovery search time-out");
break;
}
case UPNP_EVENT_RECEIVED: {
- // TODO: Handle event by updating any changed state variables */
+ // Nothing to do.
break;
}
case UPNP_EVENT_AUTORENEWAL_FAILED: // Fall through. Treat failed autorenewal like an expired
@@ -933,18 +994,24 @@ PUPnP::subEventCallback(Upnp_EventType event_type, const void* event, void* user
}
int
-PUPnP::handleSubscriptionUPnPEvent(Upnp_EventType /*event_type */, const void* event)
+PUPnP::handleSubscriptionUPnPEvent(Upnp_EventType /*unused*/, const void* event)
{
+ if (event == nullptr) {
+ JAMI_ERR("PUPnP: Unexpected null pointer!");
+ return UPNP_E_INVALID_ARGUMENT;
+ }
+
const UpnpEventSubscribe* es_event = (const UpnpEventSubscribe*) event;
+ std::string publisherUrl(UpnpEventSubscribe_get_PublisherUrl_cstr(es_event));
+
int upnp_err = UpnpEventSubscribe_get_ErrCode(es_event);
if (upnp_err != UPNP_E_SUCCESS) {
- JAMI_WARN("PUPnP: Error when trying to handle subscription callback -> %s",
- UpnpGetErrorMessage(upnp_err));
+ JAMI_WARN("PUPnP: Subscription error %s from %s",
+ UpnpGetErrorMessage(upnp_err),
+ publisherUrl.c_str());
return upnp_err;
}
- // TODO: Handle subscription event.
-
return UPNP_E_SUCCESS;
}
@@ -1238,7 +1305,7 @@ PUPnP::getMappingsListByDescr(const std::shared_ptr<IGD>& igd, const std::string
std::string desc_actual = getFirstDocItem(response.get(), "NewPortMappingDescription");
std::string client_ip = getFirstDocItem(response.get(), "NewInternalClient");
- if (client_ip != getUserLocalIp().toString()) {
+ if (client_ip != getHostAddress().toString()) {
// Silently ignore un-matching addresses.
continue;
}
@@ -1284,7 +1351,7 @@ PUPnP::deleteMappingsByDescription(const std::shared_ptr<IGD>& igd, const std::s
igd->getLocalIp().toString().c_str(),
Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
- auto mapList = getMappingsListByDescr(igd, Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
+ auto mapList = getMappingsListByDescr(igd, description);
for (auto const& [_, map] : mapList) {
requestMappingRemove(map);
@@ -1305,7 +1372,6 @@ PUPnP::actionDeletePortMapping(const UPnPIGD& igd,
std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>
response(nullptr, ixmlDocument_free); // Response pointer.
IXML_Document* action_container_ptr = nullptr;
- IXML_Document* response_container_ptr = nullptr;
// Set action name.
std::string action_name {"DeletePortMapping"};
@@ -1345,7 +1411,7 @@ PUPnP::actionDeletePortMapping(const UPnPIGD& igd,
return false;
}
- JAMI_DBG("PUPnP: Successfully sent %s request from %s",
+ JAMI_DBG("PUPnP: Successfully sent %s request to %s",
action_name.c_str(),
igd.getServiceType().c_str());
@@ -1396,7 +1462,7 @@ PUPnP::actionAddPortMapping(const UPnPIGD& igd,
action_name.c_str(),
igd.getServiceType().c_str(),
"NewInternalClient",
- getUserLocalIp().toString().c_str());
+ getHostAddress().toString().c_str());
UpnpAddToAction(&action_container_ptr,
action_name.c_str(),
igd.getServiceType().c_str(),
diff --git a/src/upnp/protocol/pupnp/pupnp.h b/src/upnp/protocol/pupnp/pupnp.h
index b431b01c03..3290bab6ae 100644
--- a/src/upnp/protocol/pupnp/pupnp.h
+++ b/src/upnp/protocol/pupnp/pupnp.h
@@ -70,6 +70,10 @@ constexpr static int CONFLICT_IN_MAPPING = 718;
// Timeout values (in seconds).
constexpr static unsigned int SEARCH_TIMEOUT {60};
constexpr static unsigned int SUBSCRIBE_TIMEOUT {300};
+// Max number of IGD search attempts before failure.
+constexpr static unsigned int PUPNP_MAX_RESTART_SEARCH_RETRIES {5};
+// Time-out between two successive IGD search.
+constexpr static auto PUPNP_TIMEOUT_BEFORE_IGD_SEARCH_RETRY {std::chrono::seconds(60)};
class PUPnP : public UPnPProtocol
{
@@ -118,8 +122,8 @@ public:
// Get the IGD list.
void getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const override;
- // Return true if it has at least one valid IGD.
- bool hasValidIgd() const override;
+ // Return true if the it's fully setup.
+ bool isReady() const override;
// Increment IGD errors counter.
void incrementErrorsCounter(const std::shared_ptr<IGD>& igd) override;
@@ -139,6 +143,14 @@ public:
void requestMappingRemove(const Mapping& igdMapping) override;
private:
+ ScheduledExecutor* getUpnContextScheduler() { return UpnpThreadUtil::getScheduler(); }
+
+ // Return true if it has at least one valid IGD.
+ bool hasValidIgd() const;
+
+ // Update and check the host (local) address. Returns true
+ // if the address is valid.
+ bool updateAndCheckHostAddress();
// Delete mappings matching the description
void deleteMappingsByDescription(const std::shared_ptr<IGD>& igd,
@@ -190,7 +202,7 @@ private:
#endif
// Callback subscription event function for handling subscription request.
- int handleSubscriptionUPnPEvent(Upnp_EventType event_type, const void* event);
+ int handleSubscriptionUPnPEvent(Upnp_EventType /*unused*/, const void* event);
// Parses the IGD candidate.
std::unique_ptr<UPnPIGD> parseIgd(IXML_Document* doc, std::string locationUrl);
@@ -225,6 +237,8 @@ private:
std::condition_variable pupnpCv_ {}; // Condition variable for thread-safe signaling.
std::atomic_bool pupnpRun_ {true}; // Variable to allow the thread to run.
std::thread pupnpThread_ {}; // PUPnP thread for non-blocking client registration.
+ std::shared_ptr<Task> searchForIgdTimer_ {};
+ unsigned int igdSearchCounter_ {0};
mutable std::mutex validIgdListMutex_;
std::list<std::shared_ptr<IGD>> validIgdList_; // List of valid IGDs.
diff --git a/src/upnp/protocol/upnp_protocol.h b/src/upnp/protocol/upnp_protocol.h
index 0b3e6bd181..eed6cf00f6 100644
--- a/src/upnp/protocol/upnp_protocol.h
+++ b/src/upnp/protocol/upnp_protocol.h
@@ -97,7 +97,7 @@ public:
virtual void getIgdList(std::list<std::shared_ptr<IGD>>& igdList) const = 0;
// Return true if it has at least one valid IGD.
- virtual bool hasValidIgd() const = 0;
+ virtual bool isReady() const = 0;
// Increment IGD errors counter.
virtual void incrementErrorsCounter(const std::shared_ptr<IGD>& igd) = 0;
@@ -121,10 +121,12 @@ public:
// Set the user callbacks.
virtual void setObserver(UpnpMappingObserver* obs) = 0;
- const IpAddr& getUserLocalIp() const { return userLocalIp_; }
+ // Get the host (local) address.
+ const IpAddr& getHostAddress() const { return hostAddress_; }
protected:
- IpAddr userLocalIp_ {};
+ // The host (local) address. Must be fully set before making any request.
+ IpAddr hostAddress_ {};
UpnpMappingObserver* observer_;
};
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index 09ade02c30..772ddc6bfb 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -181,7 +181,7 @@ UPnPContext::setPublicAddress(const IpAddr& addr)
}
bool
-UPnPContext::hasValidIGD() const
+UPnPContext::isReady() const
{
std::lock_guard<std::mutex> lock(mappingMutex_);
return not validIgdList_.empty();
@@ -365,9 +365,11 @@ UPnPContext::requestMapping(const std::shared_ptr<IGD>& igd, const Mapping::shar
CHECK_VALID_THREAD();
- JAMI_DBG("Request mapping %s using protocol [%s]",
+ JAMI_DBG("Request mapping %s using protocol [%s] IGD [%s %s]",
map->toString().c_str(),
- igd->getProtocolName());
+ igd->getProtocolName(),
+ igd->getUID().c_str(),
+ igd->getLocalIp().toString().c_str());
// Register mapping timeout callback and update the state if needed.
registerAddMappingTimeout(igd, map);
@@ -389,7 +391,7 @@ UPnPContext::requestMappingOnValidIgds(const Mapping::sharedPtr_t& map)
CHECK_VALID_THREAD();
- if (not hasValidIGD()) {
+ if (not isReady()) {
JAMI_WARN("Can not request mapping, no valid IGD available!");
return;
}
@@ -399,12 +401,12 @@ UPnPContext::requestMappingOnValidIgds(const Mapping::sharedPtr_t& map)
#if HAVE_LIBNATPMP
// Prefere NAT-PMP if available.
- if (protocolList_.at(NatProtocolType::NAT_PMP)->hasValidIgd())
+ if (protocolList_.at(NatProtocolType::NAT_PMP)->isReady())
proto = protocolList_.at(NatProtocolType::NAT_PMP);
#endif
// Request the mappings
- if (proto->hasValidIgd()) {
+ if (proto->isReady()) {
std::list<std::shared_ptr<IGD>> igdList;
proto->getIgdList(igdList);
for (auto const& igd : igdList)
@@ -496,19 +498,21 @@ UPnPContext::updateMappingList(bool async)
mappingListUpdateTimer_->cancel();
mappingListUpdateTimer_ = getScheduler()->scheduleIn([this] { updateMappingList(false); },
MAP_UPDATE_INTERVAL);
-
- bool hasValidIgd = false;
+ std::shared_ptr<IGD> prefIgd;
for (auto const& [_, protocol] : protocolList_) {
- auto valid = protocol->hasValidIgd();
- if (valid) {
- JAMI_DBG("Protocol status [%s] has valid IGD(s) %s",
- protocol->getProtocolName(),
- valid ? "YES" : "NO");
- hasValidIgd = true;
+ if (protocol->isReady()) {
+ std::list<std::shared_ptr<IGD>> igdList;
+ protocol->getIgdList(igdList);
+ assert(not igdList.empty());
+ auto const& igd = igdList.front();
+
+ // Perefer NAT-PMP if available.
+ if (not prefIgd or protocol->getProtocol() == NatProtocolType::NAT_PMP)
+ prefIgd = igd;
}
}
- if (not hasValidIgd) {
+ if (not prefIgd) {
JAMI_DBG("UPNP/NAT-PMP enabled, but no valid IGDs available");
std::lock_guard<std::mutex> lock(mappingMutex_);
// Invalidate the current IGDs.
@@ -517,6 +521,14 @@ UPnPContext::updateMappingList(bool async)
return;
}
+ JAMI_DBG("Current preferred protocol [%s] IGD [%s %s] ",
+ prefIgd->getProtocolName(),
+ prefIgd->getUID().c_str(),
+ prefIgd->getLocalIp().toString().c_str());
+
+ // Process pending requests if any.
+ processPendingRequests(prefIgd);
+
// Make new requests for mappings that failed and have
// the auto-update option enabled.
processMappingWithAutoUpdate();
@@ -567,10 +579,10 @@ UPnPContext::updateMappingList(bool async)
}
// Prune the mapping list if needed
- if (protocolList_.at(NatProtocolType::PUPNP)->hasValidIgd()) {
+ if (protocolList_.at(NatProtocolType::PUPNP)->isReady()) {
#if HAVE_LIBNATPMP
// Dont perform if NAT-PMP is valid.
- if (not protocolList_.at(NatProtocolType::NAT_PMP)->hasValidIgd())
+ if (not protocolList_.at(NatProtocolType::NAT_PMP)->isReady())
#endif
{
pruneMappingList();
@@ -579,7 +591,7 @@ UPnPContext::updateMappingList(bool async)
#if HAVE_LIBNATPMP
// Renew nat-pmp allocations
- if (protocolList_.at(NatProtocolType::NAT_PMP)->hasValidIgd())
+ if (protocolList_.at(NatProtocolType::NAT_PMP)->isReady())
renewAllocations();
#endif
}
@@ -809,12 +821,12 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
// Check if IGD has a valid addresses.
if (not igdLocalAddr) {
- JAMI_ERR("[%s] IGD has an invalid local address", protocolName);
+ JAMI_WARN("[%s] IGD has an invalid local address", protocolName);
return;
}
if (not igd->getPublicIp()) {
- JAMI_ERR("[%s] IGD has an invalid public address", protocolName);
+ JAMI_WARN("[%s] IGD has an invalid public address", protocolName);
return;
}
@@ -826,11 +838,12 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
knownPublicAddress_.toString().c_str());
}
- char const* IgdState = event == UpnpIgdEvent::ADDED ? "ADDED"
- : event == UpnpIgdEvent::REMOVED ? "REMOVED"
- : "INVALID";
+ char const* IgdState = event == UpnpIgdEvent::ADDED
+ ? "ADDED"
+ : event == UpnpIgdEvent::REMOVED ? "REMOVED" : "INVALID";
- JAMI_WARN("State of IGD %s [%s] changed to [%s]",
+ JAMI_WARN("State of IGD %s %s [%s] changed to [%s]",
+ igd->getUID().c_str(),
igdLocalAddr.toString(true, true).c_str(),
protocolName,
IgdState);
@@ -861,19 +874,6 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
}
}
- // Prune the IGD before making new requests.
- if (igd->getProtocol() == NatProtocolType::PUPNP) {
- auto protocol = protocolList_.at(igd->getProtocol());
- auto remoteMapList
- = protocol->getMappingsListByDescr(igd, Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
- if (not remoteMapList.empty()) {
- pruneUnMatchedMappings(igd, remoteMapList);
- }
- }
-
- // Process pending requests if any.
- processPendingRequests(igd);
-
// Update.
updateMappingList(false);
}
@@ -883,79 +883,56 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
{
CHECK_VALID_THREAD();
- if (not mapRes.isValid() or mapRes.getState() == MappingState::FAILED) {
- JAMI_WARN("Mapping request %s failed on IGD %s [%s]",
- mapRes.toString().c_str(),
- igd->getLocalIp().toString().c_str(),
- mapRes.getProtocolName());
+ // Check if we have a pending request for this response.
+ auto map = getMappingWithKey(mapRes.getMapKey());
+ if (not map) {
+ // We may receive a response for a canceled request. Just ignore it.
+ JAMI_DBG("Response for mapping %s [IGD %s] [%s] does not have a local match",
+ mapRes.toString().c_str(),
+ igd->getLocalIp().toString().c_str(),
+ mapRes.getProtocolName());
return;
}
- Mapping::key_t key = 0;
-
- {
- std::lock_guard<std::mutex> lock(mappingMutex_);
- auto& mappingList = getMappingList(mapRes.getType());
- auto it = mappingList.find(mapRes.getMapKey());
- if (it == mappingList.end()) {
- // We may receive a response for a canceled request. Just
- // ignore it.
- JAMI_DBG("Mapping request %s from IGD %s [%s] does not match local request",
- mapRes.toString().c_str(),
- igd->getLocalIp().toString().c_str(),
- mapRes.getProtocolName());
- return;
- }
- key = it->second->getMapKey();
- assert(key);
- }
-
- auto map = getMappingWithKey(key);
// The mapping pointer must be valid at his point.
assert(map);
- // Update the state.
- if (map->isValid()) {
- // If both protocols grant the mapping, we prefere using NAT-PMP.
- if (map->getState() == MappingState::OPEN
- and map->getProtocol() == NatProtocolType::NAT_PMP) {
- JAMI_DBG("Mapping %s is already open on IGD %s [NAT-PMP]",
- map->toString().c_str(),
- igd->getLocalIp().toString().c_str());
- } else {
- // Set the attributes
- map->setExternalAddress(mapRes.getExternalAddress());
- map->setInternalAddress(mapRes.getInternalAddress());
- map->setIgd(igd);
-#if HAVE_LIBNATPMP
- if (mapRes.getProtocol() == NatProtocolType::NAT_PMP) {
- map->setRenewalTime(mapRes.getRenewalTime());
- }
-#endif
- updateMappingState(map, MappingState::OPEN);
+ // We have a response, so cancel the timer.
+ map->cancelTimeoutTimer();
- JAMI_DBG("Mapping %s (on IGD %s [%s]) successfully performed",
- map->toString().c_str(),
- igd->getLocalIp().toString().c_str(),
- map->getProtocolName());
+ // Update the mapping.
+ map->updateFrom(mapRes);
- if (not igd->isValid()) {
- std::lock_guard<std::mutex> lock(mappingMutex_);
- validIgdList_.emplace(igd);
- }
- // Call setValid() to reset the errors counter.
- igd->setValid(true);
- }
- } else {
- JAMI_ERR("Failed to perform mapping %s (on IGD %s)",
- map->toString().c_str(),
- igd->getLocalIp().toString().c_str());
+ // Clean-up if not valid.
+ if (not map->isValid()) {
+ JAMI_WARN("Mapping request %s on IGD %s failed or returned an invalid mapping!",
+ map->toString(true).c_str(),
+ igd->getLocalIp().toString().c_str());
updateMappingState(map, MappingState::FAILED);
unregisterMapping(map);
+ return;
}
- // We have a response, so cancel the time-out.
- map->cancelTimeoutTimer();
+ // Ignore if already open.
+ if (map->getState() == MappingState::OPEN) {
+ JAMI_DBG("Mapping %s is already open on IGD %s [%s]",
+ map->toString().c_str(),
+ map->getIgd()->getLocalIp().toString().c_str(),
+ map->getIgd()->getProtocolName());
+ return;
+ }
+
+ // The mapping request is new and successful. Update.
+ updateMappingState(map, MappingState::OPEN);
+
+ JAMI_DBG("Mapping %s (on IGD %s [%s]) successfully performed",
+ map->toString().c_str(),
+ igd->getLocalIp().toString().c_str(),
+ map->getProtocolName());
+
+ // Call setValid() to reset the errors counter. We need
+ // to reset the counter on each successful response.
+ igd->setValid(true);
}
#if HAVE_LIBNATPMP
@@ -1072,7 +1049,7 @@ UPnPContext::registerMapping(Mapping& map)
// No available IGD. The pending mapping requests will be processed
// when a IGD becomes available (in onIgdAdded() method).
- if (not hasValidIGD()) {
+ if (not isReady()) {
JAMI_WARN("No IGD available. Mapping will be requested when an IGD becomes available");
} else {
requestMappingOnValidIgds(mapPtr);
@@ -1191,9 +1168,12 @@ UPnPContext::registerAddMappingTimeout(const std::shared_ptr<IGD>& igd,
auto timeout = igd->getProtocol() == NatProtocolType::NAT_PMP ? NAT_MAP_REQUEST_TIMEOUT_UNIT
: PUPNP_MAP_REQUEST_TIMEOUT_UNIT;
map->setTimeoutTimer(
- // The time-out is set proportional to the number of "in-progress" requests.
+ // The time-out is set proportional to the number of "in-progress"
+ // requests plus a bias.
+ // This rule is empirical and based on experimenting with few
+ // implementations.
Manager::instance().scheduler().scheduleIn([this, igd, map] { onRequestTimeOut(igd, map); },
- timeout * (status.inProgressCount_ + 1)));
+ timeout * (status.inProgressCount_ + 3)));
}
void
@@ -1237,7 +1217,7 @@ UPnPContext::updateMappingState(const Mapping::sharedPtr_t& map, MappingState ne
// Ignore if the state did not change.
if (newState == map->getState()) {
- JAMI_WARN("Mapping %s already in state %s", map->toString().c_str(), map->getStateStr());
+ JAMI_DBG("Mapping %s already in state %s", map->toString().c_str(), map->getStateStr());
return;
}
diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h
index 566e87b61d..080dc83cd3 100644
--- a/src/upnp/upnp_context.h
+++ b/src/upnp/upnp_context.h
@@ -109,7 +109,7 @@ public:
void setPublicAddress(const IpAddr& addr);
// Check if there is a valid IGD in the IGD list.
- bool hasValidIGD() const;
+ bool isReady() const;
// Get external Ip of a chosen IGD.
IpAddr getExternalIP() const;
diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp
index 72048aa23d..652efe4486 100644
--- a/src/upnp/upnp_control.cpp
+++ b/src/upnp/upnp_control.cpp
@@ -57,17 +57,17 @@ Controller::~Controller()
}
bool
-Controller::hasValidIGD() const
+Controller::isReady() const
{
assert(upnpContext_);
- return upnpContext_->hasValidIGD();
+ return upnpContext_->isReady();
}
IpAddr
Controller::getExternalIP() const
{
assert(upnpContext_);
- if (upnpContext_->hasValidIGD()) {
+ if (upnpContext_->isReady()) {
return upnpContext_->getExternalIP();
}
return {};
diff --git a/src/upnp/upnp_control.h b/src/upnp/upnp_control.h
index 87714f12e8..67218d37a2 100644
--- a/src/upnp/upnp_control.h
+++ b/src/upnp/upnp_control.h
@@ -51,7 +51,7 @@ public:
~Controller();
// Checks if a valid IGD is available.
- bool hasValidIGD() const;
+ bool isReady() const;
// Gets the external ip of the first valid IGD in the list.
IpAddr getExternalIP() const;
--
GitLab