diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 50b35e0fc8cb2a950f769c8fc4a4ab6be257a94c..d1b45bb5cee1f08ebf14fc70766eff16241d7bbb 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -788,7 +788,7 @@ IceTransport::Impl::requestUpnpMappings()
// Request upnp mapping for each component.
for (unsigned compId = 1; compId <= component_count_; compId++) {
// Set port number to 0 to get any available port.
- Mapping requestedMap {0, 0, portType};
+ Mapping requestedMap(portType);
// Request the mapping
Mapping::sharedPtr_t mapPtr = upnp_->reserveMapping(requestedMap);
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index 316dd6aea51571e947c3aba71a9292ea3704f523..008938f92a882339e84b259d91b71b969095b7d9 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -1786,8 +1786,6 @@ JamiAccount::registerAsyncOps()
upnpCtrl_->releaseMapping(dhtUpnpMapping_);
}
- dhtUpnpMapping_ = upnp::Mapping {0, 0, upnp::PortType::UDP};
-
dhtUpnpMapping_.enableAutoUpdate(true);
// Set the notify callback.
@@ -1805,7 +1803,8 @@ JamiAccount::registerAsyncOps()
if (*update) {
// Check if we need to update the mapping and the registration.
- if (dhtMap != *mapRes or dhtMap.getState() != mapRes->getState()) {
+ if (dhtMap.getMapKey() != mapRes->getMapKey()
+ or dhtMap.getState() != mapRes->getState()) {
// The connectivity must be restarted, if either:
// - the state changed to "OPEN",
// - the state changed to "FAILED" and the mapping was in use.
@@ -1813,7 +1812,7 @@ JamiAccount::registerAsyncOps()
or (mapRes->getState() == upnp::MappingState::FAILED
and dhtMap.getState() == upnp::MappingState::OPEN)) {
// Update the mapping and restart the registration.
- dhtMap = upnp::Mapping {*mapRes};
+ dhtMap.updateFrom(mapRes);
JAMI_WARN("[Account %s] Allocated port changed to %u. Restarting the "
"registration",
@@ -1824,14 +1823,14 @@ JamiAccount::registerAsyncOps()
} else {
// Only update the mapping.
- dhtMap = upnp::Mapping {*mapRes};
+ dhtMap.updateFrom(mapRes);
}
}
} else {
*update = true;
// Set connection info and load the account.
if (mapRes->getState() == upnp::MappingState::OPEN) {
- dhtMap = upnp::Mapping {*mapRes};
+ dhtMap.updateFrom(mapRes);
JAMI_DBG("[Account %s] Mapping %s successfully allocated: starting the DHT",
accId.c_str(),
dhtMap.toString().c_str());
@@ -2566,7 +2565,6 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb)
// Release current upnp mapping if any.
if (upnpCtrl_ and dhtUpnpMapping_.isValid()) {
upnpCtrl_->releaseMapping(dhtUpnpMapping_);
- dhtUpnpMapping_ = upnp::Mapping {};
}
lock.unlock();
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index 376534f49be6cd4d3b5bc8efb4b6a05232ae0fa0..fde37c8f33dc407a0f3f9462759ffe3a45d534dc 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -638,7 +638,7 @@ private:
}
/* Current UPNP mapping */
- upnp::Mapping dhtUpnpMapping_ {};
+ upnp::Mapping dhtUpnpMapping_ {upnp::PortType::UDP};
bool dhtPeerDiscovery_ {false};
diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp
index 1375ccd441461376c97ebbd54abc5b7ad5606755..c0653039f4ed407e676de946d56ac5cf104608c6 100644
--- a/src/sip/sipaccount.cpp
+++ b/src/sip/sipaccount.cpp
@@ -738,7 +738,7 @@ SIPAccount::getVolatileAccountDetails() const
bool
SIPAccount::mapPortUPnP()
{
- upnp::Mapping map {publishedPort_, localPort_, upnp::PortType::UDP};
+ upnp::Mapping map(upnp::PortType::UDP, publishedPort_, localPort_);
map.setNotifyCallback([w = weak()](upnp::Mapping::sharedPtr_t mapRes) {
if (auto accPtr = w.lock()) {
auto oldPort = static_cast<in_port_t>(accPtr->publishedPortUsed_);
diff --git a/src/upnp/protocol/igd.cpp b/src/upnp/protocol/igd.cpp
index 4c625cbe920e07822279dc52f368052fb63af05c..8159a89367bea4d1a8ea72d67a5d3ab5df3785b8 100644
--- a/src/upnp/protocol/igd.cpp
+++ b/src/upnp/protocol/igd.cpp
@@ -27,7 +27,6 @@ namespace upnp {
IGD::IGD(NatProtocolType proto)
: protocol_(proto)
- , valid_(true)
{}
bool
@@ -66,5 +65,11 @@ IGD::incrementErrorsCounter()
return true;
}
+int
+IGD::getErrorsCount() const
+{
+ return errorsCounter_.load();
+}
+
} // namespace upnp
} // namespace jami
\ No newline at end of file
diff --git a/src/upnp/protocol/igd.h b/src/upnp/protocol/igd.h
index e52a1d283771af34491c59a51ee03c6b655f782c..ceb523bcbe6b2f8caf5baa231eed69660fbeac8d 100644
--- a/src/upnp/protocol/igd.h
+++ b/src/upnp/protocol/igd.h
@@ -44,7 +44,7 @@ class IGD
{
public:
// Max error before moving the IGD to invalid state.
- constexpr static int MAX_ERRORS_COUNT = 20;
+ constexpr static int MAX_ERRORS_COUNT = 10;
IGD(NatProtocolType prot);
virtual ~IGD() = default;
@@ -93,10 +93,11 @@ public:
void setValid(bool valid);
bool isValid() const { return valid_; }
bool incrementErrorsCounter();
+ int getErrorsCount() const;
protected:
const NatProtocolType protocol_ {NatProtocolType::UNKNOWN};
- std::atomic_bool valid_ {true};
+ std::atomic_bool valid_ {false};
std::atomic<int> errorsCounter_ {0};
mutable std::mutex mutex_;
diff --git a/src/upnp/protocol/mapping.cpp b/src/upnp/protocol/mapping.cpp
index 0ede1470729c7df0354ff0e0d1a7307822fd51db..58a5a367f5594b7f894d9a7d44993d1c0caada20 100644
--- a/src/upnp/protocol/mapping.cpp
+++ b/src/upnp/protocol/mapping.cpp
@@ -25,16 +25,16 @@
namespace jami {
namespace upnp {
-Mapping::Mapping(uint16_t portExternal, uint16_t portInternal, PortType type, bool available)
- : internalAddr_()
- , internalPort_(portInternal)
+Mapping::Mapping(PortType type, uint16_t portExternal, uint16_t portInternal, bool available)
+ : type_(type)
, externalPort_(portExternal)
- , type_(type)
+ , internalPort_(portInternal)
+ , internalAddr_()
, igd_()
, available_(available)
, state_(MappingState::PENDING)
, notifyCb_(nullptr)
- , timeoutTimer_(nullptr)
+ , timeoutTimer_()
, autoUpdate_(false)
#if HAVE_LIBNATPMP
, renewalTime_(sys_clock::now())
@@ -53,75 +53,28 @@ Mapping::Mapping(const Mapping& other)
available_ = other.available_;
state_ = other.state_;
notifyCb_ = other.notifyCb_;
- timeoutTimer_ = other.timeoutTimer_;
autoUpdate_ = other.autoUpdate_;
#if HAVE_LIBNATPMP
renewalTime_ = other.renewalTime_;
#endif
}
-Mapping&
-Mapping::operator=(Mapping&& other) noexcept
-{
- std::lock_guard<std::mutex> lock(mutex_);
-
- if (this != &other) {
- internalAddr_ = other.internalAddr_;
- internalPort_ = other.internalPort_;
- externalPort_ = other.externalPort_;
- type_ = other.type_;
- igd_ = other.igd_;
- other.igd_.reset();
- available_ = other.available_;
- other.available_ = false;
- state_ = other.state_;
- other.state_ = MappingState::PENDING;
- notifyCb_ = std::move(other.notifyCb_);
- other.notifyCb_ = nullptr;
- timeoutTimer_ = std::move(other.timeoutTimer_);
- other.timeoutTimer_ = nullptr;
- autoUpdate_ = other.autoUpdate_;
- other.autoUpdate_ = false;
-#if HAVE_LIBNATPMP
- renewalTime_ = other.renewalTime_;
-#endif
- }
- return *this;
-}
-
-bool
-Mapping::operator==(const Mapping& other) const noexcept
-{
- std::lock_guard<std::mutex> lock(mutex_);
-
- if (internalAddr_ != other.internalAddr_)
- return false;
- if (internalPort_ != other.internalPort_)
- return false;
- if (externalPort_ != other.externalPort_)
- return false;
- if (type_ != other.type_)
- return false;
- if (igd_ != other.igd_)
- return false;
- return true;
-}
-
-bool
-Mapping::operator!=(const Mapping& other) const noexcept
+void
+Mapping::updateFrom(const Mapping::sharedPtr_t& other)
{
- return not(*this == other);
+ updateFrom(*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);
+ if (type_ != other.type_) {
+ JAMI_ERR("The source and destination types must match");
+ return;
}
internalAddr_ = std::move(other.internalAddr_);
+ internalPort_ = other.internalPort_;
externalPort_ = other.externalPort_;
igd_ = other.igd_;
state_ = other.state_;
@@ -165,15 +118,17 @@ Mapping::getStateStr() const
}
std::string
-Mapping::toString(bool addState) const
+Mapping::toString(bool extraInfo) const
{
std::lock_guard<std::mutex> lock(mutex_);
std::ostringstream descr;
descr << UPNP_MAPPING_DESCRIPTION_PREFIX << "-" << getTypeStr(type_);
descr << ":" << std::to_string(internalPort_);
- if (addState)
- descr << " (state=" << getStateStr(state_) << ")";
+ if (extraInfo) {
+ descr << " (state=" << getStateStr(state_)
+ << ", auto-update=" << (autoUpdate_ ? "YES" : "NO") << ")";
+ }
return descr.str();
}
@@ -231,9 +186,9 @@ Mapping::cancelTimeoutTimer()
{
std::lock_guard<std::mutex> lock(mutex_);
- if (timeoutTimer_ != nullptr) {
+ if (timeoutTimer_) {
timeoutTimer_->cancel();
- timeoutTimer_ = nullptr;
+ timeoutTimer_.reset();
}
}
diff --git a/src/upnp/protocol/mapping.h b/src/upnp/protocol/mapping.h
index 694784e5b2221c22a19192e0e66314c0c4861334..43885917609960a3d39348d72bee2f021a21e5e9 100644
--- a/src/upnp/protocol/mapping.h
+++ b/src/upnp/protocol/mapping.h
@@ -61,19 +61,18 @@ public:
static constexpr char const* MAPPING_STATE_STR[4] {"PENDING", "IN_PROGRESS", "FAILED", "OPEN"};
static constexpr char const* UPNP_MAPPING_DESCRIPTION_PREFIX {"JAMI"};
- Mapping(uint16_t portExternal = 0,
+ Mapping(PortType type,
+ uint16_t portExternal = 0,
uint16_t portInternal = 0,
- PortType type = PortType::UDP,
bool available = true);
Mapping(const Mapping& other);
Mapping(Mapping&& other) = delete;
~Mapping() = default;
- Mapping& operator=(Mapping&& other) noexcept;
- bool operator==(const Mapping& other) const noexcept;
- bool operator!=(const Mapping& other) const noexcept;
-
// Delete operators with confusing semantic.
+ Mapping& operator=(Mapping&& other) = delete;
+ bool operator==(const Mapping& other) = delete;
+ bool operator!=(const Mapping& other) = delete;
bool operator<(const Mapping& other) = delete;
bool operator>(const Mapping& other) = delete;
bool operator<=(const Mapping& other) = delete;
@@ -82,6 +81,7 @@ public:
inline explicit operator bool() const { return isValid(); }
void updateFrom(const Mapping& other);
+ void updateFrom(const Mapping::sharedPtr_t& other);
std::string getExternalAddress() const;
uint16_t getExternalPort() const;
std::string getExternalPortStr() const;
@@ -101,7 +101,7 @@ public:
{
return MAPPING_STATE_STR[static_cast<int>(state)];
}
- std::string toString(bool addState = false) const;
+ std::string toString(bool extraInfo = false) const;
bool isValid() const;
bool hasValidHostAddress() const;
bool hasPublicAddress() const;
@@ -131,10 +131,10 @@ private:
#endif
mutable std::mutex mutex_;
- std::string internalAddr_;
- uint16_t internalPort_ {0};
+ PortType type_ {PortType::UDP};
uint16_t externalPort_ {0};
- PortType type_;
+ uint16_t internalPort_ {0};
+ std::string internalAddr_;
// Protocol and
std::shared_ptr<IGD> igd_;
// Track if the mapping is available to use.
@@ -142,7 +142,7 @@ private:
// Track the state of the mapping
MappingState state_;
NotifyCallback notifyCb_;
- std::shared_ptr<Task> timeoutTimer_;
+ std::shared_ptr<Task> timeoutTimer_ {};
// If true, a new mapping will be requested on behave of the mapping
// owner when the mapping state changes from "OPEN" to "FAILED".
bool autoUpdate_;
diff --git a/src/upnp/protocol/natpmp/nat_pmp.cpp b/src/upnp/protocol/natpmp/nat_pmp.cpp
index 6a58e7af12ff69ee6452875f0bdf4372dfac09a2..eab6049fe32e650ce29a3dc209b0b41fbf12df99 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.cpp
+++ b/src/upnp/protocol/natpmp/nat_pmp.cpp
@@ -225,10 +225,14 @@ NatPmp::requestMappingAdd(const std::shared_ptr<IGD>& igd, const Mapping& mappin
void
NatPmp::requestMappingRenew(const Mapping& mapping)
{
- assert(mapping.getIgd());
-
// Process on nat-pmp thread.
getNatpmpScheduler()->run([this, mapping] {
+ if (not mapping.getIgd() or not mapping.getIgd()->isValid()) {
+ JAMI_WARN("NAT-PMP: Mapping %s has an invalid IGD. Ignoring.",
+ mapping.toString().c_str());
+ return;
+ }
+
JAMI_DBG("NAT-PMP: Renew mapping %s on %s",
mapping.toString().c_str(),
mapping.getIgd()->getLocalIp().toString().c_str());
@@ -335,9 +339,7 @@ NatPmp::addPortMapping(const std::shared_ptr<IGD>& igdIn, Mapping& mapping, bool
return;
}
- Mapping mapToAdd(mapping.getExternalPort(),
- mapping.getInternalPort(),
- mapping.getType() == PortType::UDP ? upnp::PortType::UDP : upnp::PortType::TCP);
+ Mapping mapToAdd(mapping);
mapToAdd.setInternalAddress(getHostAddress().toString());
mapToAdd.setIgd(igd_);
@@ -345,9 +347,9 @@ NatPmp::addPortMapping(const std::shared_ptr<IGD>& igdIn, Mapping& mapping, bool
int err = sendMappingRequest(mapping, lifetime);
if (err < 0) {
- JAMI_ERR("NAT-PMP: Add mapping request failed with error %s %i",
- getNatPmpErrorStr(err),
- errno);
+ JAMI_WARN("NAT-PMP: Add mapping request failed with error %s %i",
+ getNatPmpErrorStr(err),
+ errno);
if (isErrorFatal(err)) {
// Fatal error, increment the counter.
@@ -400,10 +402,7 @@ NatPmp::removePortMapping(Mapping& mapping)
return;
}
- Mapping mapToRemove(mapping.getExternalPort(),
- mapping.getInternalPort(),
- mapping.getType() == PortType::UDP ? upnp::PortType::UDP
- : upnp::PortType::TCP);
+ Mapping mapToRemove(mapping);
uint32_t lifetime = 0;
int err = sendMappingRequest(mapping, lifetime);
diff --git a/src/upnp/protocol/pupnp/pupnp.cpp b/src/upnp/protocol/pupnp/pupnp.cpp
index cf6334101cca85ec35c5515381d4c3d3e044fa0c..0b81a8c0992f3a1ad565501cb274d7d146d5b550 100644
--- a/src/upnp/protocol/pupnp/pupnp.cpp
+++ b/src/upnp/protocol/pupnp/pupnp.cpp
@@ -163,15 +163,10 @@ PUPnP::PUPnP()
PUPnP::~PUPnP()
{
- JAMI_DBG("PUPnP: Destroying instance %p", this);
// Clear all the lists.
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
std::lock_guard<std::mutex> lk(ctrlptMutex_);
- for (auto const& igd : validIgdList_) {
- if (igd->isValid())
- deleteMappingsByDescription(igd, Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
- }
validIgdList_.clear();
clientRegistered_ = false;
UpnpUnRegisterClient(ctrlptHandle_);
@@ -191,6 +186,8 @@ PUPnP::~PUPnP()
pupnpCv_.notify_all();
if (pupnpThread_.joinable())
pupnpThread_.join();
+
+ JAMI_DBG("PUPnP: Instance [%p] destroyed", this);
}
bool
@@ -467,6 +464,9 @@ PUPnP::validateIgd(const IGDInfo& info)
}
}
+ // We have a valid IGD
+ igd_candidate->setValid(true);
+
JAMI_DBG("PUPnP: Added a new IGD [%s] to the list of valid IGDs",
igd_candidate->getUID().c_str());
@@ -583,7 +583,7 @@ PUPnP::processAddMapAction(const std::string& ctrlURL,
uint16_t iPort,
PortType portType)
{
- Mapping mapToAdd(ePort, iPort, portType);
+ Mapping mapToAdd(portType, ePort, iPort);
JAMI_DBG("PUPnP: Opened port %s", mapToAdd.toString().c_str());
@@ -615,7 +615,7 @@ PUPnP::processRemoveMapAction(const std::string& ctrlURL,
uint16_t iPort,
PortType portType)
{
- Mapping mapToRemove {ePort, iPort, portType};
+ Mapping mapToRemove(portType, ePort, iPort);
{
std::lock_guard<std::mutex> lock(validIgdListMutex_);
for (auto const& it : validIgdList_) {
@@ -1324,11 +1324,11 @@ PUPnP::getMappingsListByDescr(const std::shared_ptr<IGD>& igd, const std::string
}
std::transform(transport.begin(), transport.end(), transport.begin(), ::toupper);
+ PortType type = transport.find("TCP") != std::string::npos ? PortType::TCP : PortType::UDP;
uint16_t ePort = static_cast<uint16_t>(std::stoi(port_external));
uint16_t iPort = static_cast<uint16_t>(std::stoi(port_internal));
- PortType type = transport.find("TCP") != std::string::npos ? PortType::TCP : PortType::UDP;
- auto map = Mapping(ePort, iPort, type);
+ Mapping map(type, ePort, iPort);
map.setIgd(igd);
mapList.emplace(map.getMapKey(), std::move(map));
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index 6973540f59cc14521c8d10aaa7391de306c70a3a..e52f86bc1024df8b837c090da3519fd16d077f0c 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -24,6 +24,17 @@
namespace jami {
namespace upnp {
+constexpr static auto NAT_MAP_REQUEST_TIMEOUT_UNIT = std::chrono::seconds(1);
+constexpr static auto PUPNP_MAP_REQUEST_TIMEOUT_UNIT = std::chrono::seconds(5);
+constexpr static auto MAP_UPDATE_INTERVAL = std::chrono::seconds(30);
+constexpr static int MAX_REQUEST_RETRIES = 20;
+constexpr static int MAX_REQUEST_REMOVE_COUNT = 5;
+
+constexpr static uint16_t UPNP_TCP_PORT_MIN {10000};
+constexpr static uint16_t UPNP_TCP_PORT_MAX {UPNP_TCP_PORT_MIN + 5000};
+constexpr static uint16_t UPNP_UDP_PORT_MIN {20000};
+constexpr static uint16_t UPNP_UDP_PORT_MAX {UPNP_UDP_PORT_MIN + 5000};
+
std::shared_ptr<UPnPContext>
UPnPContext::getUPnPContext()
{
@@ -34,6 +45,8 @@ UPnPContext::getUPnPContext()
UPnPContext::UPnPContext()
{
+ JAMI_DBG("Creating UPnPContext instance [%p]", this);
+
// Set port ranges
portRange_.emplace(PortType::TCP, std::make_pair(UPNP_TCP_PORT_MIN, UPNP_TCP_PORT_MAX));
portRange_.emplace(PortType::UDP, std::make_pair(UPNP_UDP_PORT_MIN, UPNP_UDP_PORT_MAX));
@@ -46,10 +59,14 @@ UPnPContext::UPnPContext()
UPnPContext::~UPnPContext()
{
+ stopUpnp(true);
+
+ std::lock_guard<std::mutex> lock(mappingMutex_);
+ mappingList_->clear();
mappingListUpdateTimer_->cancel();
+ controllerList_.clear();
- deleteAllMappings(PortType::UDP);
- deleteAllMappings(PortType::TCP);
+ JAMI_DBG("UPnPContext instance [%p] destroyed", this);
}
void
@@ -72,7 +89,7 @@ UPnPContext::init()
}
void
-UPnPContext::StartUpnp()
+UPnPContext::startUpnp()
{
assert(not controllerList_.empty());
@@ -89,14 +106,19 @@ UPnPContext::StartUpnp()
}
void
-UPnPContext::StopUpnp()
+UPnPContext::stopUpnp(bool forceRelease)
{
- CHECK_VALID_THREAD();
+ if (not isValidThread()) {
+ runOnUpnpContextThread([this] { stopUpnp(); });
+ return;
+ }
JAMI_DBG("Stoping UPNP context");
- // Use temporary list to avoid holding the lock while
- // processing the mapping list.
+ // Clear all current mappings if any.
+
+ // Use a temporary list to avoid processing the mapping
+ // list while holding the lock.
std::list<Mapping::sharedPtr_t> toRemoveList;
{
std::lock_guard<std::mutex> lock(mappingMutex_);
@@ -113,11 +135,17 @@ UPnPContext::StopUpnp()
}
for (auto const& map : toRemoveList) {
- map->cancelTimeoutTimer();
+ requestRemoveMapping(map);
updateMappingState(map, MappingState::FAILED);
- unregisterMapping(map);
+ // We dont remove mappings with auto-update enabled,
+ // unless forceRelease is true.
+ if (not map->getAutoUpdate() or forceRelease) {
+ map->enableAutoUpdate(false);
+ unregisterMapping(map);
+ }
}
+ // Clear all current IGDs.
for (auto const& [_, protocol] : protocolList_) {
protocol->clearIgds();
}
@@ -126,24 +154,27 @@ UPnPContext::StopUpnp()
}
uint16_t
-UPnPContext::generateRandomPort(uint16_t min, uint16_t max, bool mustBeEven)
+UPnPContext::generateRandomPort(PortType type, bool mustBeEven)
{
- if (min >= max) {
- JAMI_ERR("Max port number (%i) must be greater than min port number (%i)", max, min);
+ auto minPort = type == PortType::TCP ? UPNP_TCP_PORT_MIN : UPNP_UDP_PORT_MIN;
+ auto maxPort = type == PortType::TCP ? UPNP_TCP_PORT_MAX : UPNP_UDP_PORT_MAX;
+
+ if (minPort >= maxPort) {
+ JAMI_ERR("Max port number (%i) must be greater than min port number (%i)", maxPort, minPort);
// Must be called with valid range.
assert(false);
}
int fact = mustBeEven ? 2 : 1;
if (mustBeEven) {
- min /= fact;
- max /= fact;
+ minPort /= fact;
+ maxPort /= fact;
}
// Seed the generator.
static std::mt19937 gen(dht::crypto::getSeededRandomEngine());
// Define the range.
- std::uniform_int_distribution<uint16_t> dist(min, max);
+ std::uniform_int_distribution<uint16_t> dist(minPort, maxPort);
return dist(gen) * fact;
}
@@ -162,8 +193,8 @@ UPnPContext::connectivityChanged()
JAMI_DBG("Connectivity changed. Reset IGDs and restart.");
- StopUpnp();
- StartUpnp();
+ stopUpnp();
+ startUpnp();
processMappingWithAutoUpdate();
}
@@ -271,7 +302,7 @@ UPnPContext::releaseMapping(const Mapping& map)
auto mapPtr = getMappingWithKey(map.getMapKey());
if (not mapPtr) {
- // Might happen if the mapping failed
+ // Might happen if the mapping failed or was never granted.
JAMI_DBG("Mapping %s does not exist or was already removed", map.toString().c_str());
return;
}
@@ -282,7 +313,7 @@ UPnPContext::releaseMapping(const Mapping& map)
}
// Remove it.
- deleteMapping(mapPtr);
+ requestRemoveMapping(mapPtr);
unregisterMapping(mapPtr);
}
@@ -302,7 +333,7 @@ UPnPContext::registerController(void* controller)
JAMI_DBG("Successfully registered controller %p", this);
if (not started_)
- StartUpnp();
+ startUpnp();
}
void
@@ -320,31 +351,21 @@ UPnPContext::unregisterController(void* controller)
}
if (controllerList_.empty())
- StopUpnp();
+ stopUpnp();
}
uint16_t
-UPnPContext::getAvailablePortNumber(PortType type, uint16_t minPort, uint16_t maxPort)
+UPnPContext::getAvailablePortNumber(PortType type)
{
// Only return an availalable random port. No actual
// reservation is made here.
- if (minPort > maxPort) {
- JAMI_ERR("Min port %u can not be greater than max port %u", minPort, maxPort);
- return 0;
- }
-
- if (minPort == 0)
- minPort = portRange_[type].first;
- if (maxPort == 0)
- maxPort = portRange_[type].second;
-
std::lock_guard<std::mutex> lock(mappingMutex_);
auto& mappingList = getMappingList(type);
int tryCount = 0;
while (tryCount++ < MAX_REQUEST_RETRIES) {
- uint16_t port = generateRandomPort(minPort, maxPort);
- Mapping map {port, port, type};
+ uint16_t port = generateRandomPort(type);
+ Mapping map(type, port, port);
if (mappingList.find(map.getMapKey()) == mappingList.end())
return port;
}
@@ -355,17 +376,29 @@ UPnPContext::getAvailablePortNumber(PortType type, uint16_t minPort, uint16_t ma
}
void
-UPnPContext::requestMapping(const std::shared_ptr<IGD>& igd, const Mapping::sharedPtr_t& map)
+UPnPContext::requestMapping(const Mapping::sharedPtr_t& map)
{
assert(map);
if (not isValidThread()) {
- runOnUpnpContextThread([this, igd, map] { requestMapping(igd, map); });
+ runOnUpnpContextThread([this, map] { requestMapping(map); });
return;
}
CHECK_VALID_THREAD();
+ auto const& igd = getPreferredIgd();
+ // We must have at least a valid IGD pointer if we get here.
+ // Not this method is called only if there were a valid IGD, however,
+ // because the processing is asynchronous, it's possible that the IGD
+ // was invalidated when the this code executed.
+ if (not igd) {
+ JAMI_DBG("No valid IGDs available");
+ return;
+ }
+
+ map->setIgd(igd);
+
JAMI_DBG("Request mapping %s using protocol [%s] IGD [%s %s]",
map->toString().c_str(),
igd->getProtocolName(),
@@ -373,61 +406,32 @@ UPnPContext::requestMapping(const std::shared_ptr<IGD>& igd, const Mapping::shar
igd->getLocalIp().toString().c_str());
// Register mapping timeout callback and update the state if needed.
- registerAddMappingTimeout(igd, map);
+ registerAddMappingTimeout(map);
if (map->getState() != MappingState::IN_PROGRESS)
updateMappingState(map, MappingState::IN_PROGRESS);
// Request the mapping.
- auto const& protocol = protocolList_.at(igd->getProtocol());
- protocol->requestMappingAdd(igd, *map);
-}
-
-void
-UPnPContext::requestMappingOnValidIgds(const Mapping::sharedPtr_t& map)
-{
- if (not isValidThread()) {
- runOnUpnpContextThread([this, map] { requestMappingOnValidIgds(map); });
- return;
- }
-
- CHECK_VALID_THREAD();
-
- if (not isReady()) {
- JAMI_WARN("Can not request mapping, no valid IGD available!");
+ if (not igd) {
+ JAMI_ERR("No valid IGD!");
return;
}
-
- // Set to PUPNP by default.
- auto proto = protocolList_.at(NatProtocolType::PUPNP);
-
-#if HAVE_LIBNATPMP
- // Prefere NAT-PMP if available.
- if (protocolList_.at(NatProtocolType::NAT_PMP)->isReady())
- proto = protocolList_.at(NatProtocolType::NAT_PMP);
-#endif
-
- // Request the mappings
- if (proto->isReady()) {
- std::list<std::shared_ptr<IGD>> igdList;
- proto->getIgdList(igdList);
- for (auto const& igd : igdList)
- requestMapping(igd, map);
- }
+ auto const& protocol = protocolList_.at(igd->getProtocol());
+ protocol->requestMappingAdd(igd, *map);
}
bool
-UPnPContext::provisionNewMappings(PortType type, int portCount, uint16_t minPort, uint16_t maxPort)
+UPnPContext::provisionNewMappings(PortType type, int portCount)
{
JAMI_DBG("Provision %i new mappings of type [%s]", portCount, Mapping::getTypeStr(type));
assert(portCount > 0);
while (portCount > 0) {
- auto port = getAvailablePortNumber(type, minPort, maxPort);
+ auto port = getAvailablePortNumber(type);
if (port > 0) {
// Found an available port number
portCount--;
- Mapping map {port, port, type, true};
+ Mapping map(type, port, port, true);
registerMapping(map);
} else {
// Very unlikely to get here!
@@ -462,7 +466,7 @@ UPnPContext::deleteUnneededMappings(PortType type, int portCount)
if (map->getState() == MappingState::OPEN and portCount > 0) {
// Close portCount mappings in "OPEN" state.
- deleteMapping(map);
+ requestRemoveMapping(map);
it = unregisterMapping(it);
portCount--;
} else if (map->getState() != MappingState::OPEN) {
@@ -479,6 +483,29 @@ UPnPContext::deleteUnneededMappings(PortType type, int portCount)
return true;
}
+std::shared_ptr<IGD>
+UPnPContext::getPreferredIgd() const
+{
+ CHECK_VALID_THREAD();
+
+ std::shared_ptr<IGD> prefIgd;
+ for (auto const& [_, protocol] : protocolList_) {
+ if (protocol->isReady()) {
+ std::list<std::shared_ptr<IGD>> igdList;
+ protocol->getIgdList(igdList);
+ assert(not igdList.empty());
+ auto const& igd = igdList.front();
+
+ // Perefer IGD with the lowest error count, if equal, prefer NAT-PMP.
+ if (not prefIgd or igd->getErrorsCount() < prefIgd->getErrorsCount()
+ or protocol->getProtocol() == NatProtocolType::NAT_PMP) {
+ prefIgd = igd;
+ }
+ }
+ }
+ return prefIgd;
+}
+
void
UPnPContext::updateMappingList(bool async)
{
@@ -499,19 +526,8 @@ UPnPContext::updateMappingList(bool async)
mappingListUpdateTimer_->cancel();
mappingListUpdateTimer_ = getScheduler()->scheduleIn([this] { updateMappingList(false); },
MAP_UPDATE_INTERVAL);
- std::shared_ptr<IGD> prefIgd;
- for (auto const& [_, protocol] : protocolList_) {
- 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;
- }
- }
+ std::shared_ptr<IGD> prefIgd = getPreferredIgd();
if (not prefIgd) {
JAMI_DBG("UPNP/NAT-PMP enabled, but no valid IGDs available");
@@ -605,25 +621,24 @@ UPnPContext::pruneMappingList()
MappingStatus status;
getMappingStatus(status);
- // Prune the list if there are pending/in-progress requests.
- if (status.inProgressCount_ == 0 and status.pendingCount_ == 0) {
- std::list<std::shared_ptr<IGD>> igdsList;
-
- // Performed only with PUPNP protocol.
- auto protocol = protocolList_.at(NatProtocolType::PUPNP);
- protocol->getIgdList(igdsList);
+ // Do not prune the list if there are pending/in-progress requests.
+ if (status.inProgressCount_ != 0 or status.pendingCount_ != 0) {
+ return;
+ }
- for (auto const& igd : igdsList) {
- auto remoteMapList
- = protocol->getMappingsListByDescr(igd, Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
- if (remoteMapList.empty())
- continue;
+ auto const& igd = getPreferredIgd();
+ if (not igd or igd->getProtocol() != NatProtocolType::PUPNP) {
+ return;
+ }
+ auto protocol = protocolList_.at(NatProtocolType::PUPNP);
- pruneUnMatchedMappings(igd, remoteMapList);
+ auto remoteMapList = protocol->getMappingsListByDescr(igd,
+ Mapping::UPNP_MAPPING_DESCRIPTION_PREFIX);
+ if (remoteMapList.empty())
+ return;
- pruneUnTrackedMappings(igd, remoteMapList);
- }
- }
+ pruneUnMatchedMappings(igd, remoteMapList);
+ pruneUnTrackedMappings(igd, remoteMapList);
}
void
@@ -677,7 +692,8 @@ UPnPContext::pruneUnTrackedMappings(const std::shared_ptr<IGD>& igd,
std::lock_guard<std::mutex> lock(mappingMutex_);
for (auto const& [_, map] : remoteMapList) {
- // Must be UPNP protocol.
+ // Must has valid IGD pointer and use UPNP protocol.
+ assert(map.getIgd());
assert(map.getIgd()->getProtocol() == NatProtocolType::PUPNP);
auto& mappingList = getMappingList(map.getType());
auto it = mappingList.find(map.getMapKey());
@@ -705,6 +721,8 @@ UPnPContext::pruneUnTrackedMappings(const std::shared_ptr<IGD>& igd,
void
UPnPContext::pruneMappingsWithInvalidIgds(const std::shared_ptr<IGD>& igd)
{
+ CHECK_VALID_THREAD();
+
// Use temporary list to avoid holding the lock while
// processing the mapping list.
std::list<Mapping::sharedPtr_t> toRemoveList;
@@ -747,7 +765,7 @@ UPnPContext::processPendingRequests(const std::shared_ptr<IGD>& igd)
for (auto type : typeArray) {
auto& mappingList = getMappingList(type);
- for (auto const& [_, map] : mappingList) {
+ for (auto& [_, map] : mappingList) {
if (map->getState() == MappingState::PENDING) {
JAMI_DBG("Send request for pending mapping %s to IGD %s",
map->toString().c_str(),
@@ -760,7 +778,7 @@ UPnPContext::processPendingRequests(const std::shared_ptr<IGD>& igd)
// Process the pending requests.
for (auto const& map : requestsList) {
- requestMapping(igd, map);
+ requestMapping(map);
}
}
@@ -787,21 +805,24 @@ UPnPContext::processMappingWithAutoUpdate()
}
}
- for (auto const& map : requestsList) {
+ for (auto const& oldMap : requestsList) {
// Request a new mapping if auto-update is enabled.
JAMI_DBG("Mapping %s has auto-update enabled, a new mapping will be requested",
- map->toString().c_str());
+ oldMap->toString().c_str());
// Reserve a new mapping.
- Mapping newMapping(*map);
- newMapping.setExternalPort(0);
- newMapping.setInternalPort(0);
- registerMapping(newMapping);
+ Mapping newMapping(oldMap->getType());
+ auto const& mapPtr = reserveMapping(newMapping);
+ assert(mapPtr);
+ mapPtr->setAvailable(false);
+ mapPtr->enableAutoUpdate(true);
+ mapPtr->setNotifyCallback(oldMap->getNotifyCallback());
+
// Release the old one.
- map->setAvailable(true);
- map->enableAutoUpdate(false);
- map->setNotifyCallback(nullptr);
- unregisterMapping(map);
+ oldMap->setAvailable(true);
+ oldMap->enableAutoUpdate(false);
+ oldMap->setNotifyCallback(nullptr);
+ unregisterMapping(oldMap);
}
}
@@ -817,9 +838,15 @@ UPnPContext::onIgdUpdated(const std::shared_ptr<IGD>& igd, UpnpIgdEvent event)
CHECK_VALID_THREAD();
+ char const* IgdState = event == UpnpIgdEvent::ADDED
+ ? "ADDED"
+ : event == UpnpIgdEvent::REMOVED ? "REMOVED" : "INVALID";
+
auto const& igdLocalAddr = igd->getLocalIp();
auto protocolName = igd->getProtocolName();
+ JAMI_DBG("New event for IGD [%s] [%s]: [%s]", igd->getUID().c_str(), protocolName, IgdState);
+
// Check if IGD has a valid addresses.
if (not igdLocalAddr) {
JAMI_WARN("[%s] IGD has an invalid local address", protocolName);
@@ -839,18 +866,14 @@ 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";
-
- JAMI_WARN("State of IGD %s %s [%s] changed to [%s]",
- igd->getUID().c_str(),
- igdLocalAddr.toString(true, true).c_str(),
- protocolName,
- IgdState);
-
// The IGD was removed or is invalid.
if (event == UpnpIgdEvent::REMOVED or event == UpnpIgdEvent::INVALID_STATE) {
+ JAMI_WARN("State of IGD [%s %s] [%s] changed to [%s]. Pruning the mapping list",
+ igd->getUID().c_str(),
+ igdLocalAddr.toString(true, true).c_str(),
+ protocolName,
+ IgdState);
+
pruneMappingsWithInvalidIgds(igd);
std::lock_guard<std::mutex> lock(mappingMutex_);
@@ -902,13 +925,17 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
map->cancelTimeoutTimer();
// Update the mapping.
- map->updateFrom(mapRes);
+ map->setState(mapRes.getState());
+ map->setIgd(mapRes.getIgd());
+ map->setInternalAddress(mapRes.getInternalAddress());
+ map->setExternalPort(mapRes.getExternalPort());
// Clean-up if not valid.
if (not map->isValid()) {
- JAMI_WARN("Mapping request %s on IGD %s failed or returned an invalid mapping!",
+ JAMI_WARN("Mapping request %s on IGD %s [%s] failed!",
map->toString(true).c_str(),
- igd->getLocalIp().toString().c_str());
+ igd->getLocalIp().toString().c_str(),
+ igd->getProtocolName());
updateMappingState(map, MappingState::FAILED);
unregisterMapping(map);
return;
@@ -916,6 +943,7 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
// Ignore if already open.
if (map->getState() == MappingState::OPEN) {
+ assert(map->getIgd());
JAMI_DBG("Mapping %s is already open on IGD %s [%s]",
map->toString().c_str(),
map->getIgd()->getLocalIp().toString().c_str(),
@@ -964,17 +992,22 @@ UPnPContext::onMappingRenewed(const std::shared_ptr<IGD>& igd, const Mapping& ma
#endif
void
-UPnPContext::deleteMapping(const Mapping::sharedPtr_t& map)
+UPnPContext::requestRemoveMapping(const Mapping::sharedPtr_t& map)
{
CHECK_VALID_THREAD();
if (not map) {
- JAMI_ERR("Invalid mapping shared pointer");
+ JAMI_ERR("Mapping shared pointer is null!");
return;
}
- if (not map->getIgd() or not map->getIgd()->isValid()) {
- JAMI_WARN("Invalid mapping shared pointer");
+ if (not map->isValid()) {
+ JAMI_WARN("Mapping [%s] is invalid! Ignore.", map->toString().c_str());
+ return;
+ }
+
+ if (not map->getIgd()->isValid()) {
+ JAMI_WARN("Mapping [%s] has an invalid IGD! Ignore.", map->toString().c_str());
return;
}
@@ -995,15 +1028,9 @@ UPnPContext::deleteAllMappings(PortType type)
std::lock_guard<std::mutex> lock(mappingMutex_);
auto& mappingList = getMappingList(type);
- for (auto it = mappingList.begin(); it != mappingList.end();) {
- auto map = it->second;
- it = unregisterMapping(it);
- auto protocol = protocolList_.at(map->getIgd()->getProtocol());
- protocol->requestMappingRemove(*map);
+ for (auto const& [_, map] : mappingList) {
+ requestRemoveMapping(map);
}
-
- getMappingList(PortType::TCP).clear();
- getMappingList(PortType::UDP).clear();
}
void
@@ -1043,6 +1070,7 @@ UPnPContext::registerMapping(Mapping& map)
{
std::lock_guard<std::mutex> lock(mappingMutex_);
auto& mappingList = getMappingList(map.getType());
+
auto ret = mappingList.emplace(map.getMapKey(), std::make_shared<Mapping>(map));
if (not ret.second) {
JAMI_WARN("Mapping request for %s already added!", map.toString().c_str());
@@ -1057,7 +1085,7 @@ UPnPContext::registerMapping(Mapping& map)
if (not isReady()) {
JAMI_WARN("No IGD available. Mapping will be requested when an IGD becomes available");
} else {
- requestMappingOnValidIgds(mapPtr);
+ requestMapping(mapPtr);
}
return mapPtr;
@@ -1072,11 +1100,6 @@ UPnPContext::unregisterMapping(std::map<Mapping::key_t, Mapping::sharedPtr_t>::i
auto descr = it->second->toString();
auto& mappingList = getMappingList(it->second->getType());
auto ret = mappingList.erase(it);
- if (ret != mappingList.end()) {
- JAMI_DBG("Unregister mapping %s succeeded", descr.c_str());
- } else {
- JAMI_ERR("Failed to unregister mapping %s", descr.c_str());
- }
return ret;
}
@@ -1084,6 +1107,15 @@ UPnPContext::unregisterMapping(std::map<Mapping::key_t, Mapping::sharedPtr_t>::i
void
UPnPContext::unregisterMapping(const Mapping::sharedPtr_t& map)
{
+ CHECK_VALID_THREAD();
+
+ if (not map) {
+ JAMI_ERR("Mapping pointer is null");
+ return;
+ }
+
+ map->cancelTimeoutTimer();
+
if (map->getAutoUpdate()) {
// Dont unregister mappings with auto-update enabled.
return;
@@ -1158,14 +1190,19 @@ UPnPContext::getMappingStatus(MappingStatus& status)
}
void
-UPnPContext::registerAddMappingTimeout(const std::shared_ptr<IGD>& igd,
- const Mapping::sharedPtr_t& map)
+UPnPContext::registerAddMappingTimeout(const Mapping::sharedPtr_t& map)
{
if (not map) {
JAMI_ERR("Invalid mapping pointer");
return;
}
+ auto const& igd = map->getIgd();
+ if (not igd) {
+ JAMI_ERR("Invalid igd pointer");
+ return;
+ }
+
MappingStatus status;
getMappingStatus(status);
@@ -1177,17 +1214,31 @@ UPnPContext::registerAddMappingTimeout(const std::shared_ptr<IGD>& igd,
// 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_ + 3)));
+ Manager::instance()
+ .scheduler()
+ .scheduleIn([this, key = map->getMapKey()] { onRequestTimeOut(key); },
+ timeout * (status.inProgressCount_ + 5)));
}
void
-UPnPContext::onRequestTimeOut(const std::shared_ptr<IGD>& igd, const Mapping::sharedPtr_t& map)
+UPnPContext::onRequestTimeOut(Mapping::key_t key)
{
CHECK_VALID_THREAD();
+ auto const& map = getMappingWithKey(key);
if (not map) {
- JAMI_ERR("Invalid mapping pointer");
+ JAMI_ERR("Mapping pointer is null");
+ return;
+ }
+
+ auto igd = map->getIgd();
+ if (not igd) {
+ JAMI_ERR("IGD pointer is null");
+ return;
+ }
+
+ if (not getMappingWithKey(map->getMapKey())) {
+ JAMI_ERR("Mapping [%s] does not exist", map->toString().c_str());
return;
}
diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h
index 6b6e0feb5d1bc039564eb537c964a15216028cd4..d6879ff8c9930fd56a147cc315fa7b1737127bdb 100644
--- a/src/upnp/upnp_context.h
+++ b/src/upnp/upnp_context.h
@@ -66,19 +66,7 @@ namespace upnp {
class UPnPContext : public UpnpMappingObserver, protected UpnpThreadUtil
{
-public:
- constexpr static uint16_t UPNP_TCP_PORT_MIN = 10000;
- constexpr static uint16_t UPNP_TCP_PORT_MAX = UPNP_TCP_PORT_MIN + 5000;
- constexpr static uint16_t UPNP_UDP_PORT_MIN = 20000;
- constexpr static uint16_t UPNP_UDP_PORT_MAX = UPNP_UDP_PORT_MIN + 5000;
-
private:
- constexpr static auto NAT_MAP_REQUEST_TIMEOUT_UNIT = std::chrono::seconds(1);
- constexpr static auto PUPNP_MAP_REQUEST_TIMEOUT_UNIT = std::chrono::seconds(5);
- constexpr static auto MAP_UPDATE_INTERVAL = std::chrono::seconds(30);
- constexpr static int MAX_REQUEST_RETRIES = 20;
- constexpr static int MAX_REQUEST_REMOVE_COUNT = 5;
-
struct MappingStatus
{
int openCount_ {0};
@@ -129,15 +117,27 @@ public:
void unregisterController(void* controller);
// Generate random port numbers
- static uint16_t generateRandomPort(uint16_t min, uint16_t max, bool mustBeEven = false);
+ static uint16_t generateRandomPort(PortType type, bool mustBeEven = false);
private:
// Initialization
void init();
- // Start/Stop
- void StartUpnp();
- void StopUpnp();
+ /**
+ * @brief start the search for IGDs activate the mapping
+ * list update.
+ *
+ */
+ void startUpnp();
+
+ /**
+ * @brief Clear all IGDs and release/delete current mappings
+ *
+ * @param forceRelease If true, also delete mappings with enabled
+ * auto-update feature.
+ *
+ */
+ void stopUpnp(bool forceRelease = false);
// Create and register a new mapping.
Mapping::sharedPtr_t registerMapping(Mapping& map);
@@ -148,22 +148,19 @@ private:
void unregisterMapping(const Mapping::sharedPtr_t& map);
// Perform the request on the provided IGD.
- void requestMapping(const std::shared_ptr<IGD>& igd, const Mapping::sharedPtr_t& map);
-
- // Perform the request on all available IGDs
- void requestMappingOnValidIgds(const Mapping::sharedPtr_t& map);
+ void requestMapping(const Mapping::sharedPtr_t& map);
- // Delete mapping from the list and and send remove request.
- void deleteMapping(const Mapping::sharedPtr_t& map);
+ // Request a mapping remove from the IGD.
+ void requestRemoveMapping(const Mapping::sharedPtr_t& map);
// Remove all mappings of the given type.
void deleteAllMappings(PortType type);
// Schedule a time-out timer for a in-progress request.
- void registerAddMappingTimeout(const std::shared_ptr<IGD>& igd, const Mapping::sharedPtr_t& map);
+ void registerAddMappingTimeout(const Mapping::sharedPtr_t& map);
// Callback invoked when a request times-out
- void onRequestTimeOut(const std::shared_ptr<IGD>& igd, const Mapping::sharedPtr_t& map);
+ void onRequestTimeOut(Mapping::key_t key);
// Update the state and notify the listener
void updateMappingState(const Mapping::sharedPtr_t& map,
@@ -171,16 +168,16 @@ private:
bool notify = true);
// Provision ports.
- uint16_t getAvailablePortNumber(PortType type, uint16_t minPort = 0, uint16_t maxPort = 0);
+ uint16_t getAvailablePortNumber(PortType type);
+
+ // Get preferred IGD
+ std::shared_ptr<IGD> getPreferredIgd() const;
// Check and prune the mapping list. Called periodically.
void updateMappingList(bool async);
// Provision (pre-allocate) the requested number of mappings.
- bool provisionNewMappings(PortType type,
- int portCount,
- uint16_t minPort = 0,
- uint16_t maxPort = 0);
+ bool provisionNewMappings(PortType type, int portCount);
// Close unused mappings.
bool deleteUnneededMappings(PortType type, int portCount);
@@ -211,8 +208,15 @@ private:
void pruneMappingsWithInvalidIgds(const std::shared_ptr<IGD>& igd);
- // Get the mapping list
+ /**
+ * @brief Get the mapping list
+ *
+ * @param type transport type (TCP/UDP)
+ * @return a reference on the map
+ * @warning concurrency protection done by the caller
+ */
std::map<Mapping::key_t, Mapping::sharedPtr_t>& getMappingList(PortType type);
+
// Get the mapping from the key.
Mapping::sharedPtr_t getMappingWithKey(Mapping::key_t key);
diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp
index 5de901b3a0604a9dafe1c87b55750d873ff2cb78..702da5ff0fe8e6b1a9b4a56fa9be8ed4a9fefd25 100644
--- a/src/upnp/upnp_control.cpp
+++ b/src/upnp/upnp_control.cpp
@@ -76,7 +76,7 @@ Controller::getExternalIP() const
Mapping::sharedPtr_t
Controller::reserveMapping(uint16_t port, PortType type)
{
- Mapping map {port, port, type};
+ Mapping map(type, port, port);
return reserveMapping(map);
}
@@ -142,12 +142,7 @@ Controller::removeLocalMap(const Mapping& map)
uint16_t
Controller::generateRandomPort(PortType type)
{
- if (type == PortType::TCP)
- return UPnPContext::generateRandomPort(UPnPContext::UPNP_TCP_PORT_MIN,
- UPnPContext::UPNP_TCP_PORT_MAX);
- else
- return UPnPContext::generateRandomPort(UPnPContext::UPNP_UDP_PORT_MIN,
- UPnPContext::UPNP_UDP_PORT_MAX);
+ return UPnPContext::generateRandomPort(type);
}
} // namespace upnp
diff --git a/src/upnp/upnp_thread_util.h b/src/upnp/upnp_thread_util.h
index c1f66fc1953d597b5fef15c6d6c4cdaaec132cea..1fcc3c9ceee5f261bffd321c7821490bcddb9018 100644
--- a/src/upnp/upnp_thread_util.h
+++ b/src/upnp/upnp_thread_util.h
@@ -10,7 +10,7 @@
#define CHECK_VALID_THREAD() \
if (not isValidThread()) \
JAMI_ERR() << "The calling thread " << getCurrentThread() \
- << "is not the expected thread: " << threadId_;
+ << " is not the expected thread: " << threadId_;
namespace jami {
namespace upnp {