diff --git a/src/account.h b/src/account.h index da4c0d7064fda5580e924be724921e52f24beff2..052973192f8b6505bd9f6f4d6bddce16e8782952 100644 --- a/src/account.h +++ b/src/account.h @@ -484,8 +484,8 @@ class Account : public Serializable, public std::enable_shared_from_this<Account /** * UPnP IGD controller and the mutex to access it */ - std::unique_ptr<jami::upnp::Controller> upnp_; mutable std::mutex upnp_mtx {}; + std::unique_ptr<jami::upnp::Controller> upnp_; /** * private account codec searching functions diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp index 2027cb69604384bdd3f15556f79ef70573ac085d..c2a61b7971fd139b81ace265dfedf6e2eca3cb2e 100644 --- a/src/jamidht/jamiaccount.cpp +++ b/src/jamidht/jamiaccount.cpp @@ -1489,30 +1489,23 @@ JamiAccount::registerAsyncOps() }); if (upnp_) { - upnp_->requestMappingAdd([this, onLoad](uint16_t port_used, bool success) { + upnp_->requestMappingAdd([this, onLoad, update = std::make_shared<bool>(false)](uint16_t port_used, bool success) { auto oldPort = static_cast<in_port_t>(dhtPortUsed_); auto newPort = success ? port_used : dhtPort_; - - if (not success and not dht_->isRunning()) { - JAMI_WARN("[Account %s] Failed to open port %u: starting DHT anyways", getAccountID().c_str(), oldPort); - onLoad(); - return; - } - - if (oldPort != newPort or not dht_->isRunning()){ - dhtPortUsed_ = newPort; - if (not dht_->isRunning()) { - JAMI_WARN("[Account %s] Starting DHT on port %u", getAccountID().c_str(), newPort); - onLoad(); - } else { + if (*update) { + if (oldPort != newPort) { JAMI_WARN("[Account %s] DHT port changed to %u: restarting network", getAccountID().c_str(), newPort); dht_->connectivityChanged(); } } else { - JAMI_WARN("[Account %s] DHT port %u opened: restarting network", getAccountID().c_str(), newPort); - dht_->connectivityChanged(); + *update = true; + if (success) + JAMI_WARN("[Account %s] Starting DHT on port %u", getAccountID().c_str(), newPort); + else + JAMI_WARN("[Account %s] Failed to open port %u: starting DHT anyways", getAccountID().c_str(), oldPort); + onLoad(); } - }, dhtPort_, jami::upnp::PortType::UDP, false); + }, dhtPort_, upnp::PortType::UDP, false); } else onLoad(); } @@ -1536,10 +1529,10 @@ JamiAccount::doRegister() generateDhParams(); } + setRegistrationState(RegistrationState::TRYING); /* if UPnP is enabled, then wait for IGD to complete registration */ if (upnp_ or proxyServerCached_.empty()) { JAMI_DBG("UPnP: Attempting to map ports for Jami account"); - setRegistrationState(RegistrationState::TRYING); registerAsyncOps(); } else { doRegister_(); @@ -1658,6 +1651,11 @@ JamiAccount::doRegister_() { std::lock_guard<std::mutex> lock(configurationMutex_); + if (registrationState_ != RegistrationState::TRYING) { + JAMI_ERR("[Account %s] already registered", getAccountID().c_str()); + return; + } + try { if (not accountManager_ or not accountManager_->getInfo()) throw std::runtime_error("No identity configured for this account."); @@ -2000,6 +1998,8 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb) dht_->join(); + if (upnp_) upnp_->requestMappingRemove(static_cast<in_port_t>(dhtPortUsed_), upnp::PortType::UDP); + lock.unlock(); setRegistrationState(RegistrationState::UNREGISTERED); diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp index e80a723c06eb0127d9c3bab642ec4d1bc176638b..34967e55aa942d41b5040dd1e53707ad6801fb9e 100644 --- a/src/upnp/upnp_context.cpp +++ b/src/upnp/upnp_context.cpp @@ -379,10 +379,10 @@ UPnPContext::dispatchOnAddCallback(const Mapping& map, bool success) cb(map, success); } -void +bool UPnPContext::requestMappingRemove(const Mapping& map) { - + auto removed = false; std::lock_guard<std::mutex> lk(igdListMutex_); if (not igdList_.empty()) { for (auto const& igd : igdList_) { @@ -392,9 +392,11 @@ UPnPContext::requestMappingRemove(const Mapping& map) } else { igd.first->requestMappingRemove(map); } + removed = true; } } } + return removed; } void diff --git a/src/upnp/upnp_context.h b/src/upnp/upnp_context.h index eb4d37ef25a3cf9c7448a832c4b70392e65086a2..760dc993b69fa0e6c544d63734463ff38ba6e1d4 100644 --- a/src/upnp/upnp_context.h +++ b/src/upnp/upnp_context.h @@ -125,7 +125,7 @@ public: void unregisterAddMappingTimeout(const Mapping& map); // Sends out a request to protocol to remove a mapping. - void requestMappingRemove(const Mapping& map); + bool requestMappingRemove(const Mapping& map); // Removes mapping from corresponding IGD. void removeMappingFromIgd(IpAddr igdIp, const Mapping& map); // Callback function for when mapping is removed. diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp index 3e809099da49d4e29bf3e065719f6eaeab877863..84a11beb1e4dbceae8a8217346b8548e8d1ff5e1 100644 --- a/src/upnp/upnp_control.cpp +++ b/src/upnp/upnp_control.cpp @@ -46,7 +46,7 @@ Controller::~Controller() } bool -Controller::hasValidIGD() +Controller::hasValidIGD() const { return upnpContext_ and upnpContext_->hasValidIGD(); } @@ -99,11 +99,11 @@ Controller::requestMappingAdd(NotifyServiceCallback&& cb, uint16_t portDesired, id_, keepCb_, [cb, portDesired, this](const Mapping& map, bool success) { cb(portDesired, success); - if (success && map.isValid()) + if (map.isValid()) addLocalMap(map); }, [this](const Mapping& map, bool success) { - if (map.isValid() and success) + if (map.isValid()) removeLocalMap(map); }, [this]() { @@ -117,20 +117,31 @@ Controller::requestMappingAdd(NotifyServiceCallback&& cb, uint16_t portDesired, } bool -Controller::isLocalMapPresent(const unsigned int portExternal, PortType type) +Controller::requestMappingRemove(uint16_t portExternal, PortType type) { + if (not upnpContext_) return false; std::lock_guard<std::mutex> lk(mapListMutex_); auto& instanceMappings = type == PortType::UDP ? udpMappings_ : tcpMappings_; - auto it = instanceMappings.find(portExternal); - return it != instanceMappings.end(); + auto mapIt = instanceMappings.find(portExternal); + if (mapIt != instanceMappings.end()) { + if (!upnpContext_->requestMappingRemove(mapIt->second)) { + // No port mapped, so the callback will not be removed, + // remove it anyway + upnpContext_->unregisterCallback(mapIt->second); + } + instanceMappings.erase(mapIt); + return true; + } + return false; } -void -Controller::addLocalMap(const Mapping& map) +bool +Controller::isLocalMapPresent(uint16_t portExternal, PortType type) const { std::lock_guard<std::mutex> lk(mapListMutex_); - auto& instanceMappings = map.getType() == PortType::UDP ? udpMappings_ : tcpMappings_; - instanceMappings.emplace(map.getPortExternal(), Mapping(map)); + auto& instanceMappings = type == PortType::UDP ? udpMappings_ : tcpMappings_; + auto it = instanceMappings.find(portExternal); + return it != instanceMappings.end(); } void @@ -138,19 +149,35 @@ Controller::requestAllMappingRemove(PortType type) { if (not upnpContext_) return; std::lock_guard<std::mutex> lk(mapListMutex_); auto& instanceMappings = type == PortType::UDP ? udpMappings_ : tcpMappings_; - for (const auto& map: instanceMappings) - upnpContext_->requestMappingRemove(map.second); + for (const auto& map: instanceMappings) { + if (!upnpContext_->requestMappingRemove(map.second)) { + // No port mapped, so the callback will not be removed, + // remove it anyway + upnpContext_->unregisterCallback(map.second); + } + } } void +Controller::addLocalMap(const Mapping& map) +{ + std::lock_guard<std::mutex> lk(mapListMutex_); + auto& instanceMappings = map.getType() == PortType::UDP ? udpMappings_ : tcpMappings_; + instanceMappings.emplace(map.getPortExternal(), Mapping(map)); +} + +bool Controller::removeLocalMap(const Mapping& map) { - if (not upnpContext_) return; + if (not upnpContext_) return false; std::lock_guard<std::mutex> lk(mapListMutex_); auto& instanceMappings = map.getType() == PortType::UDP ? udpMappings_ : tcpMappings_; auto it = instanceMappings.find(map.getPortExternal()); - if (it != instanceMappings.end()) + if (it != instanceMappings.end()) { instanceMappings.erase(it); + return true; + } + return false; } }} // namespace jami::upnp diff --git a/src/upnp/upnp_control.h b/src/upnp/upnp_control.h index 4fcb894fe85833c145cf625ee5d08381411ed5de..0ae822909437188a7b324a8ef2a6ac6d79556613 100644 --- a/src/upnp/upnp_control.h +++ b/src/upnp/upnp_control.h @@ -47,11 +47,11 @@ class Controller { public: using NotifyServiceCallback = std::function<void(uint16_t, bool)>; - Controller(bool keepCb = true); + Controller(bool keepCb = false); ~Controller(); // Checks if a valid IGD is available. - bool hasValidIGD(); + bool hasValidIGD() const; // Gets the external ip of the first valid IGD in the list. IpAddr getExternalIP() const; @@ -60,26 +60,26 @@ public: IpAddr getLocalIP() const; void requestMappingAdd(NotifyServiceCallback&& cb, uint16_t portDesired, PortType type, bool unique, uint16_t portLocal = 0); + bool requestMappingRemove(uint16_t portExternal, PortType type); // Checks if the map is present locally given a port and type. - bool isLocalMapPresent(const unsigned int portExternal, PortType type); - - // Adds a mapping locally to the list. - void addLocalMap(const Mapping& map); - // Removes a mapping locally from the list. - void removeLocalMap(const Mapping& map); + bool isLocalMapPresent(uint16_t portExternal, PortType type) const; private: std::shared_ptr<UPnPContext> upnpContext_; // Context from which the controller executes the wanted commands. - std::mutex mapListMutex_; // Mutex to protect mappings list. + mutable std::mutex mapListMutex_; // Mutex to protect mappings list. PortMapLocal udpMappings_; // List of UDP mappings created by this instance. PortMapLocal tcpMappings_; // List of TCP mappings created by this instance. size_t listToken_ {0}; // IGD listener token. uint64_t id_ {0}; // Variable to store string of address to be used as the unique identifier. - bool keepCb_ {true}; // Variable that indicates if the controller wants to keep it's callbacks in the list after a connectivity change. + bool keepCb_ {false}; // Variable that indicates if the controller wants to keep it's callbacks in the list after a connectivity change. + // Adds a mapping locally to the list. + void addLocalMap(const Mapping& map); + // Removes a mapping locally from the list. + bool removeLocalMap(const Mapping& map); // Removes all mappings of the given type. void requestAllMappingRemove(PortType type); };