upnp: do not enable if not used

upnp is always activated even if no account uses it.

Here we refactoring its creation and availability checks.
This also removes an atomic_bool that gives no true conccurent
protection as it protects itself, but not multiple accesses
to upnp_ member.

Change-Id: Ibf6aaee54077867cdf3029fd51ef6cae0b7488c1

src/account.cpp

@@ -101,7 +101,6 @@ Account::Account(const std::string &accountID)
     , userAgent_(DEFAULT_USER_AGENT)
     , hasCustomUserAgent_(false)
     , mailBox_()
-    , upnp_(new upnp::Controller())
 {
     random_device rdev;
     std::seed_seq seed {rdev(), rdev()};
@@ -139,6 +138,17 @@ Account::freeAccount()
     doUnregister();
 }
 
+void
+Account::enableUpnp(bool state)
+{
+    std::lock_guard<std::mutex> lk {upnp_mtx};
+
+    if (state and !upnp_)
+        upnp_.reset(new upnp::Controller());
+    else if (!state and upnp_)
+        upnp_.reset();
+}
+
 void
 Account::setRegistrationState(RegistrationState state, unsigned detail_code, const std::string& detail_str)
 {
@@ -216,7 +226,7 @@ Account::serialize(YAML::Emitter& out)
     out << YAML::Key << USER_AGENT_KEY << YAML::Value << userAgent_;
     out << YAML::Key << DISPLAY_NAME_KEY << YAML::Value << displayName_;
     out << YAML::Key << HOSTNAME_KEY << YAML::Value << hostname_;
-    out << YAML::Key << UPNP_ENABLED_KEY << YAML::Value << upnpEnabled_;
+    out << YAML::Key << UPNP_ENABLED_KEY << YAML::Value << bool(upnp_);
 }
 
 void
@@ -246,7 +256,7 @@ Account::unserialize(const YAML::Node& node)
 
     bool enabled;
     parseValue(node, UPNP_ENABLED_KEY, enabled);
-    upnpEnabled_.store(enabled);
+    enableUpnp(enabled);
 }
 
 void
@@ -270,7 +280,7 @@ Account::setAccountDetails(const std::map<std::string, std::string> &details)
         userAgent_ = DEFAULT_USER_AGENT;
     bool enabled;
     parseBool(details, Conf::CONFIG_UPNP_ENABLED, enabled);
-    upnpEnabled_.store(enabled);
+    enableUpnp(enabled);
 }
 
 std::map<std::string, std::string>
@@ -290,7 +300,7 @@ Account::getAccountDetails() const
         {DRing::Account::ConfProperties::ACTIVE_CALL_LIMIT,   ring::to_string(activeCallLimit_)},
         {Conf::CONFIG_RINGTONE_ENABLED,     ringtoneEnabled_ ? TRUE_STR : FALSE_STR},
         {Conf::CONFIG_RINGTONE_PATH,        ringtonePath_},
-        {Conf::CONFIG_UPNP_ENABLED,         upnpEnabled_ ? TRUE_STR : FALSE_STR},
+        {Conf::CONFIG_UPNP_ENABLED,         upnp_ ? TRUE_STR : FALSE_STR},
     };
 }
 
@@ -437,7 +447,7 @@ IpAddr
 Account::getUPnPIpAddress() const
 {
     std::lock_guard<std::mutex> lk(upnp_mtx);
-    if (upnpEnabled_)
+    if (upnp_)
         return upnp_->getExternalIP();
     return {};
 }
@@ -450,7 +460,7 @@ bool
 Account::getUPnPActive(std::chrono::seconds timeout) const
 {
     std::lock_guard<std::mutex> lk(upnp_mtx);
-    if (upnpEnabled_)
+    if (upnp_)
         return upnp_->hasValidIGD(timeout);
     return false;
 }

src/account.h

@@ -323,6 +323,8 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
          */
         std::set<std::string> callIDSet_;
 
+        void enableUpnp(bool state);
+
     protected:
         static void parseString(const std::map<std::string, std::string> &details, const char *key, std::string &s);
         static void parseBool(const std::map<std::string, std::string> &details, const char *key, bool &b);
@@ -467,11 +469,6 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
         std::unique_ptr<ring::upnp::Controller> upnp_;
         mutable std::mutex upnp_mtx {};
 
-        /**
-         * flag which determines if this account is set to use UPnP.
-         */
-        std::atomic_bool upnpEnabled_ {false};
-
         /**
          * private account codec searching functions
          */

src/ringdht/ringaccount.cpp

@@ -1962,7 +1962,7 @@ RingAccount::doRegister()
     }
 
     /* if UPnP is enabled, then wait for IGD to complete registration */
-    if ( upnpEnabled_ ) {
+    if (upnp_) {
         auto shared = shared_from_this();
         RING_DBG("UPnP: waiting for IGD to register RING account");
         setRegistrationState(RegistrationState::TRYING);
@@ -3212,7 +3212,7 @@ RingAccount::igdChanged()
 {
     if (not dht_.isRunning())
         return;
-    if ( upnpEnabled_ ) {
+    if (upnp_) {
         auto shared = std::static_pointer_cast<RingAccount>(shared_from_this());
         std::thread{[shared] {
             auto& this_ = *shared.get();

src/sip/sipaccount.cpp

@@ -698,7 +698,7 @@ void SIPAccount::doRegister()
     RING_DBG("doRegister %s", hostname_.c_str());
 
     /* if UPnP is enabled, then wait for IGD to complete registration */
-    if ( upnpEnabled_ ) {
+    if (upnp_) {
         RING_DBG("UPnP: waiting for IGD to register SIP account");
         setRegistrationState(RegistrationState::TRYING);
         auto shared = shared_from_this();
@@ -842,7 +842,7 @@ void SIPAccount::doUnregister(std::function<void(bool)> released_cb)
     if (released_cb)
         released_cb(not isIP2IP());
 
-    if (upnpEnabled_) {
+    if (upnp_) {
         upnp_->setIGDListener();
         upnp_->removeMappings();
     }