From bb396ff0ff010c6f0f9ab255916230ddf69ba62e Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Wed, 7 Apr 2021 15:20:35 -0400
Subject: [PATCH] upnp/account: create upnp controller instance only when
needed
Create upnp controller instance only when needed. Particularly
prevents creating instances when storing account data.
Gitlab: #510
Change-Id: I99f119f5015389eec769e21e0f6ef70f1663f5da
---
src/account.cpp | 15 ++++++++++-----
src/account.h | 2 +-
src/jamidht/jamiaccount.cpp | 35 +++--------------------------------
src/jamidht/jamiaccount.h | 1 -
src/upnp/upnp_control.cpp | 20 ++++++++++----------
src/upnp/upnp_control.h | 4 ++--
6 files changed, 26 insertions(+), 51 deletions(-)
diff --git a/src/account.cpp b/src/account.cpp
index 789d4ece98..f52a336572 100644
--- a/src/account.cpp
+++ b/src/account.cpp
@@ -157,14 +157,18 @@ Account::hangupCalls()
}
void
-Account::enableUpnp(bool state)
+Account::updateUpnpController()
{
std::lock_guard<std::mutex> lk {upnp_mtx};
- if (state and !upnpCtrl_)
+ if (upnpEnabled_ and not upnpCtrl_) {
upnpCtrl_.reset(new upnp::Controller());
- else if (!state and upnpCtrl_)
+ if (not upnpCtrl_) {
+ throw std::runtime_error("Failed to create a UPNP Controller instance!");
+ }
+ } else if (not upnpEnabled_) {
upnpCtrl_.reset();
+ }
}
void
@@ -287,7 +291,7 @@ Account::unserialize(const YAML::Node& node)
}
parseValue(node, UPNP_ENABLED_KEY, upnpEnabled_);
- enableUpnp(upnpEnabled_ && isEnabled());
+ updateUpnpController();
std::string defMod;
parseValueOptional(node, DEFAULT_MODERATORS_KEY, defMod);
@@ -318,7 +322,8 @@ Account::setAccountDetails(const std::map<std::string, std::string>& details)
parseString(details, Conf::CONFIG_ACCOUNT_USERAGENT, customUserAgent_);
parseBool(details, Conf::CONFIG_UPNP_ENABLED, upnpEnabled_);
- enableUpnp(upnpEnabled_ && isEnabled());
+ updateUpnpController();
+
std::string defMod;
parseString(details, Conf::CONFIG_DEFAULT_MODERATORS, defMod);
defaultModerators_ = string_split_set(defMod);
diff --git a/src/account.h b/src/account.h
index 95058f2116..efb68b723c 100644
--- a/src/account.h
+++ b/src/account.h
@@ -373,7 +373,7 @@ private:
std::set<std::string> callIDSet_;
protected:
- virtual void enableUpnp(bool state);
+ void updateUpnpController();
static void parseString(const std::map<std::string, std::string>& details,
const char* key,
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index 24ee9c0e1a..4d01aace0c 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -2079,35 +2079,6 @@ JamiAccount::doRegister()
cacheTurnServers(); // reset cache for TURN servers
}
-void
-JamiAccount::enableUpnp(bool enable)
-{
- std::lock_guard<std::mutex> lk {upnp_mtx};
-
- if (enable) {
- auto const& addr = getPublishedIpAddress();
-
- if (upnpCtrl_ and upnpCtrl_->getExternalIP() == addr) {
- JAMI_DBG(
- "[Account %s] Already have a valid controller instance with external address [%s]",
- getAccountID().c_str(),
- upnpCtrl_->getExternalIP().toString().c_str());
- return;
- }
-
- JAMI_DBG("[Account %s] Creating a new upnp controller instance", getAccountID().c_str());
- // Need a known IPv4 address to validate the external address
- // returned by the IGD.
- if (addr and addr.getFamily() == AF_INET) {
- upnpCtrl_.reset(new upnp::Controller(addr));
- } else {
- upnpCtrl_.reset(new upnp::Controller());
- }
- } else {
- upnpCtrl_.reset();
- }
-}
-
std::vector<std::string>
JamiAccount::loadBootstrap() const
{
@@ -3741,9 +3712,9 @@ JamiAccount::storeActiveIpAddress()
getAccountID().c_str(),
publicAddr.toString().c_str());
- // Check if we need to create a new UPNP controller instance.
- if (upnpEnabled_ and (not upnpCtrl_ or not upnpCtrl_->isReady())) {
- enableUpnp(true);
+ // Set the known public address in UPNP if enabled.
+ if (upnpCtrl_) {
+ upnpCtrl_->setPublicAddress(publicAddr);
}
}
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index 4bbe027c55..103bc2e64f 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -221,7 +221,6 @@ public:
*/
void doUnregister(std::function<void(bool)> cb = {}) override;
- void enableUpnp(bool enable) override;
/**
* @return pj_str_t "From" uri based on account information.
* From RFC3261: "The To header field first and foremost specifies the desired
diff --git a/src/upnp/upnp_control.cpp b/src/upnp/upnp_control.cpp
index 0aecf23942..d6ab3badd4 100644
--- a/src/upnp/upnp_control.cpp
+++ b/src/upnp/upnp_control.cpp
@@ -39,16 +39,6 @@ Controller::Controller()
JAMI_DBG("Controller@%p: Created UPnP Controller session", this);
}
-Controller::Controller(const IpAddr& addr)
- : Controller()
-{
- assert(upnpContext_);
- assert(addr);
- assert(addr.getFamily() == AF_INET);
-
- upnpContext_->setPublicAddress(addr);
-}
-
Controller::~Controller()
{
JAMI_DBG("Controller@%p: Destroying UPnP Controller session", this);
@@ -57,6 +47,16 @@ Controller::~Controller()
upnpContext_->unregisterController(this);
}
+void
+Controller::setPublicAddress(const IpAddr& addr)
+{
+ assert(upnpContext_);
+
+ if (addr and addr.getFamily() == AF_INET) {
+ upnpContext_->setPublicAddress(addr);
+ }
+}
+
bool
Controller::isReady() const
{
diff --git a/src/upnp/upnp_control.h b/src/upnp/upnp_control.h
index d19ac8e4bd..918c255d9a 100644
--- a/src/upnp/upnp_control.h
+++ b/src/upnp/upnp_control.h
@@ -48,12 +48,12 @@ class Controller
{
public:
Controller();
- Controller(const IpAddr& addr);
~Controller();
+ // Set known public address
+ void setPublicAddress(const IpAddr& addr);
// Checks if a valid IGD is available.
bool isReady() const;
-
// Gets the external ip of the first valid IGD in the list.
IpAddr getExternalIP() const;
--
GitLab