diff --git a/daemon/src/account.cpp b/daemon/src/account.cpp
index c1077b74ab6bcb778245373d4a91cf27d8ac9a35..40f83e3b9b795c29542c28f777409211743280c9 100644
--- a/daemon/src/account.cpp
+++ b/daemon/src/account.cpp
@@ -502,11 +502,11 @@ Account::getUPnPIpAddress() const
* ie: if it is able to make port mappings
*/
bool
-Account::getUPnPActive() const
+Account::getUPnPActive(std::chrono::seconds timeout) const
{
std::lock_guard<std::mutex> lk(upnp_mtx);
if (upnpEnabled_)
- return upnp_->hasValidIGD();
+ return upnp_->hasValidIGD(timeout);
return false;
}
diff --git a/daemon/src/account.h b/daemon/src/account.h
index f295bd93e34cc3ccf6a65f60cfea411b9a51a3d9..6165cc4cfcad61da655ad5d592bdaf3a3f708102 100644
--- a/daemon/src/account.h
+++ b/daemon/src/account.h
@@ -53,6 +53,7 @@
#include <stdexcept>
#include <atomic>
#include <mutex>
+#include <chrono>
namespace ring { namespace upnp {
class Controller;
@@ -255,7 +256,7 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
* returns whether or not UPnP is enabled and active
* ie: if it is able to make port mappings
*/
- bool getUPnPActive() const;
+ bool getUPnPActive(std::chrono::seconds timeout = {}) const;
/**
* Get the UPnP IP (external router) address.
diff --git a/daemon/src/upnp/upnp_context.cpp b/daemon/src/upnp/upnp_context.cpp
index c3e2eb34d375a402adf1637d92c322191ec4827d..2d72b46ec57fff1cc9ca4fbfa301f47e141af08f 100644
--- a/daemon/src/upnp/upnp_context.cpp
+++ b/daemon/src/upnp/upnp_context.cpp
@@ -40,6 +40,7 @@
#include <memory>
#include <condition_variable>
#include <random>
+#include <chrono>
#include <cstdlib> // for std::free
#if HAVE_LIBUPNP
@@ -178,39 +179,33 @@ UPnPContext::~UPnPContext()
void
UPnPContext::searchForIGD()
{
- if (clientRegistered_) {
- std::lock_guard<std::mutex> lock(igdSearchMutex_);
- /* send out search for both types, as some routers may possibly only reply to one */
- UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_ROOT_DEVICE, nullptr);
- UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_IGD_DEVICE, nullptr);
- UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_WANIP_SERVICE, nullptr);
- UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_WANPPP_SERVICE, nullptr);
-
- pendingIGDSearchRequests_ = 4;
- } else
- RING_WARN("UPnP: Control Point not registered.");
+ if (not clientRegistered_) {
+ RING_WARN("UPnP: Control Point not registered");
+ return;
+ }
+
+ /* send out search for both types, as some routers may possibly only reply to one */
+ UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_ROOT_DEVICE, nullptr);
+ UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_IGD_DEVICE, nullptr);
+ UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANIP_SERVICE, nullptr);
+ UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANPPP_SERVICE, nullptr);
}
-/**
- * returns 'true' if there is at least one valid (connected) IGD
- * note: this function will until an IGD has been found or SEARCH_TIMEOUT_SEC
- * have expired; the timeout starts when the context is created as we start
- * searchign for IGDs immediately
- */
bool
-UPnPContext::hasValidIGD()
+UPnPContext::hasValidIGD(std::chrono::seconds timeout)
{
if (not clientRegistered_) {
- RING_WARN("UPnP: Control Point not registered.");
+ RING_WARN("UPnP: Control Point not registered");
return false;
}
- {
- std::unique_lock<std::mutex> lock(igdSearchMutex_);
- igdSearchCondition_.wait(lock, [this] { return pendingIGDSearchRequests_ <= 0;});
+ std::unique_lock<std::mutex> lock(validIGDMutex_);
+ if (!validIGDCondVar_.wait_for(lock, timeout,
+ [this]{return not validIGDs_.empty();})) {
+ RING_WARN("UPnP: Valid IGD search timeout");
+ return false;
}
- std::lock_guard<std::mutex> lock(validIGDMutex_);
return not validIGDs_.empty();
}
@@ -256,7 +251,7 @@ UPnPContext::addMapping(IGD* igd,
auto iter = globalMappings->find(port_external);
if (iter != globalMappings->end()) {
/* mapping exists with same external port */
- GlobalMapping *mapping_ptr = &iter->second;
+ GlobalMapping* mapping_ptr = &iter->second;
if (*mapping_ptr == mapping) {
/* the same mapping, so nothing needs to be done */
*upnp_error = UPNP_E_SUCCESS;
@@ -337,17 +332,12 @@ UPnPContext::addAnyMapping(uint16_t port_desired,
bool use_same_port,
bool unique)
{
- if (not hasValidIGD()) {
- RING_WARN("UPnP: no valid IGD availabe");
- return {};
- }
-
/* get a lock on the igd list because we don't want the igd to be modified
* or removed from the list while using it */
std::lock_guard<std::mutex> lock(validIGDMutex_);
IGD* igd = chooseIGD_unlocked();
if (not igd) {
- RING_WARN("UPnP: no valid IGD availabe");
+ RING_WARN("UPnP: no valid IGD available");
return {};
}
@@ -402,18 +392,13 @@ UPnPContext::addAnyMapping(uint16_t port_desired,
void
UPnPContext::removeMapping(const Mapping& mapping)
{
- if (not hasValidIGD()) {
- RING_WARN("UPnP: no valid IGD availabe");
- return;
- }
-
/* get a lock on the igd list because we don't want the igd to be modified
* or removed from the list while using it */
std::lock_guard<std::mutex> lock(validIGDMutex_);
IGD* igd = chooseIGD_unlocked();
if (not igd) {
- RING_WARN("UPnP: no valid IGD availabe");
- return;;
+ RING_WARN("UPnP: no valid IGD available");
+ return;
}
/* first make sure the mapping exists in the global list of the igd */
@@ -451,17 +436,12 @@ UPnPContext::removeMapping(const Mapping& mapping)
IpAddr
UPnPContext::getExternalIP()
{
- if (not hasValidIGD()) {
- RING_WARN("UPnP: no valid IGD availabe");
- return {};
- }
-
/* get a lock on the igd list because we don't want the igd to be modified
* or removed from the list while using it */
std::lock_guard<std::mutex> lock(validIGDMutex_);
IGD* igd = chooseIGD_unlocked();
if (not igd) {
- RING_WARN("UPnP: no valid IGD availabe");
+ RING_WARN("UPnP: no valid IGD available");
return {};
}
@@ -637,15 +617,6 @@ UPnPContext::parseIGD(IXML_Document* doc, const Upnp_Discovery* d_event)
removeMappingsByLocalIPAndDescription(new_igd.get(), Mapping::UPNP_DEFAULT_MAPPING_DESCRIPTION);
validIGDs_.emplace(UDN, std::move(new_igd));
}
-
- /* check if we have pending search requests and notify anyone waiting */
- {
- std::lock_guard<std::mutex> lock(igdSearchMutex_);
- if (pendingIGDSearchRequests_ > 0) {
- pendingIGDSearchRequests_ = 0;
- }
- }
- igdSearchCondition_.notify_all();
}
}
@@ -692,7 +663,6 @@ get_first_element_item(IXML_Element* element, const char* item)
return ret;
}
-
int
cp_callback(Upnp_EventType event_type, void* event, void* /*user_data*/)
{
@@ -801,16 +771,8 @@ cp_callback(Upnp_EventType event_type, void* event, void* /*user_data*/)
break;
case UPNP_DISCOVERY_SEARCH_TIMEOUT:
- {
RING_DBG("UPnP: Control Point search timeout");
- {
- std::lock_guard<std::mutex> lock(upnpContext->igdSearchMutex_);
- if (upnpContext->pendingIGDSearchRequests_ > 0)
- --(upnpContext->pendingIGDSearchRequests_);
- }
- upnpContext->igdSearchCondition_.notify_all();
- }
- break;
+ break;
case UPNP_CONTROL_ACTION_COMPLETE:
{
diff --git a/daemon/src/upnp/upnp_context.h b/daemon/src/upnp/upnp_context.h
index cff06827cfa24c3431ea745273526a98d6ad76e8..523112af9c91c93f53fde9af5973ab3d2543affb 100644
--- a/daemon/src/upnp/upnp_context.h
+++ b/daemon/src/upnp/upnp_context.h
@@ -40,6 +40,7 @@
#include <mutex>
#include <memory>
#include <condition_variable>
+#include <chrono>
#if HAVE_LIBUPNP
#include <upnp/upnp.h>
@@ -58,22 +59,19 @@ namespace ring { namespace upnp {
class UPnPContext {
public:
+ constexpr static unsigned SEARCH_TIMEOUT {30};
-#if HAVE_LIBUPNP
-
- /* search timeout in seconds */
- constexpr static int SEARCH_TIMEOUT_SEC = 30;
+#if HAVE_LIBUPNP
UPnPContext();
~UPnPContext();
/**
- * returns 'true' if there is at least one valid (connected) IGD
- * note: this function will wait until an IGD has been found or SEARCH_TIMEOUT_SEC
- * have expired; the timeout starts when the context is created as we start
- * searching for IGDs immediately
+ * Returns 'true' if there is at least one valid (connected) IGD.
+ * @param timeout Time to wait until a valid IGD is found.
+ * If timeout is not given or 0, the function pool (non-blocking).
*/
- bool hasValidIGD();
+ bool hasValidIGD(std::chrono::seconds timeout = {});
/**
* tries to add mapping from and to the port_desired
@@ -86,8 +84,6 @@ public:
* that the external and internal ports are the same
*
* returns a valid mapping on success and an invalid mapping on failure
- *
- * note: this function will call hasValidIGD and wait upto SEARCH_TIMEOUT_SEC
*/
Mapping addAnyMapping(uint16_t port_desired,
uint16_t port_local,
@@ -97,11 +93,12 @@ public:
/**
* tries to remove the given mapping
- *
- * note: this function will call hasValidIGD and wait upto SEARCH_TIMEOUT_SEC
*/
void removeMapping(const Mapping& mapping);
+ /**
+ * tries to get the external ip of the router
+ */
IpAddr getExternalIP();
#else
@@ -162,7 +159,8 @@ private:
* the mutex is used to access these lists and IGDs in a thread-safe manner
*/
std::map<std::string, std::unique_ptr<IGD>> validIGDs_;
- std::mutex validIGDMutex_;
+ mutable std::mutex validIGDMutex_;
+ std::condition_variable validIGDCondVar_;
/**
* chooses the IGD to use (currently selects the first one in the map)
@@ -173,11 +171,6 @@ private:
/* sends out async search for IGD */
void searchForIGD();
- /* vars to sync search timeout */
- unsigned pendingIGDSearchRequests_ {0};
- std::mutex igdSearchMutex_;
- std::condition_variable igdSearchCondition_;
-
/**
* callback function for the UPnP client (control point)
* all UPnP events received by the client are processed here
@@ -233,4 +226,4 @@ std::shared_ptr<UPnPContext> getUPnPContext();
}} // namespace ring::upnp
-#endif /* UPNP_CONTEXT_H_ */
\ No newline at end of file
+#endif /* UPNP_CONTEXT_H_ */
diff --git a/daemon/src/upnp/upnp_control.cpp b/daemon/src/upnp/upnp_control.cpp
index e21d55c87603148c287fd3a9237b647ddf9b32a7..e1abef1be56c6249117ae8e635152653254288d1 100644
--- a/daemon/src/upnp/upnp_control.cpp
+++ b/daemon/src/upnp/upnp_control.cpp
@@ -56,29 +56,15 @@ Controller::~Controller()
removeMappings();
}
-/**
- * Return whether or not this controller has a valid IGD,
- * if 'flase' then all requests will fail
- */
bool
-Controller::hasValidIGD()
+Controller::hasValidIGD(std::chrono::seconds timeout)
{
#if HAVE_LIBUPNP
- return upnpContext_->hasValidIGD();
+ return upnpContext_->hasValidIGD(timeout);
#endif
return false;
}
-/**
- * tries to add mapping from and to the port_desired
- * if unique == true, makes sure the client is not using this port already
- * if the mapping fails, tries other available ports until success
- *
- * tries to use a random port between 1024 < > 65535 if desired port fails
- *
- * maps port_desired to port_local; if use_same_port == true, makes sure that
- * that the extranl and internal ports are the same
- */
bool
Controller::addAnyMapping(uint16_t port_desired,
uint16_t port_local,
@@ -104,9 +90,6 @@ Controller::addAnyMapping(uint16_t port_desired,
return false;
}
-/**
- * addAnyMapping with the local port being the same as the external port
- */
bool
Controller::addAnyMapping(uint16_t port_desired,
PortType type,
@@ -116,11 +99,6 @@ Controller::addAnyMapping(uint16_t port_desired,
addAnyMapping(port_desired, port_desired, type, true, unique, port_used);
}
-/**
- * removes mappings added by this instance of the specified port type
- * if an mapping has more than one user in the global list, it is not deleted
- * from the router, but the number of users is decremented
- */
void
Controller::removeMappings(PortType type) {
#if HAVE_LIBUPNP
@@ -132,10 +110,6 @@ Controller::removeMappings(PortType type) {
}
#endif
}
-
-/**
- * removes all mappings added by this instance
- */
void
Controller::removeMappings()
{
@@ -145,9 +119,6 @@ Controller::removeMappings()
#endif
}
-/**
- * tries to get the external ip of the router
- */
IpAddr
Controller::getExternalIP()
{
diff --git a/daemon/src/upnp/upnp_control.h b/daemon/src/upnp/upnp_control.h
index e2c1770322d2dc7b560e3be863f2a02a670af81c..86d4d2916eed23159122139897af52bf2c6934c9 100644
--- a/daemon/src/upnp/upnp_control.h
+++ b/daemon/src/upnp/upnp_control.h
@@ -36,6 +36,7 @@
#endif
#include <memory>
+#include <chrono>
#include "noncopyable.h"
#include "upnp_igd.h"
@@ -58,16 +59,11 @@ public:
~Controller();
/**
- * Return whether or not this controller has a valid IGD,
- * if 'flase' then all requests will fail
+ * Return whether or not this controller has a valid IGD.
+ * @param timeout Time to wait until a valid IGD is found.
+ * If timeout is not given or 0, the function pool (non-blocking).
*/
- bool hasValidIGD();
-
- /**
- * like hasValidIGD, but calls the given callback when the IGD is found
- * or when the search times out without finding one
- */
- // void waitForValildIGD(IGDFoundCallback cb);
+ bool hasValidIGD(std::chrono::seconds timeout = {});
/**
* tries to add mapping from and to the port_desired