Revamp implementation of port forwarder

src/base/bittorrent/portforwarderimpl.cpp

@@ -1,6 +1,6 @@
 /*
  * Bittorrent Client using Qt and libtorrent.
- * Copyright (C) 2019  Vladimir Golovnev <glassez@yandex.ru>
+ * Copyright (C) 2019-2022  Vladimir Golovnev <glassez@yandex.ru>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -28,13 +28,12 @@
 
 #include "portforwarderimpl.h"
 
-#include <libtorrent/session.hpp>
+#include <utility>
 
-#include "base/algorithm.h"
+#include "base/bittorrent/sessionimpl.h"
-#include "base/logger.h"
 
-PortForwarderImpl::PortForwarderImpl(lt::session *provider, QObject *parent)
+PortForwarderImpl::PortForwarderImpl(BitTorrent::SessionImpl *provider, QObject *parent)
-    : Net::PortForwarder {parent}
+    : Net::PortForwarder(parent)
     , m_storeActive {u"Network/PortForwardingEnabled"_qs, true}
     , m_provider {provider}
 {
@@ -66,38 +65,13 @@ void PortForwarderImpl::setEnabled(const bool enabled)
 
 void PortForwarderImpl::setPorts(const QString &profile, QSet<quint16> ports)
 {
-    PortMapping &portMapping = m_portProfiles[profile];
+    const QSet<quint16> oldForwardedPorts = std::accumulate(m_portProfiles.cbegin(), m_portProfiles.cend(), QSet<quint16>());
-    Algorithm::removeIf(portMapping, [this, &ports](const quint16 port, const std::vector<lt::port_mapping_t> &handles)
-    {
-        // keep existing forwardings
-        const bool isAlreadyMapped = ports.remove(port);
-        if (isAlreadyMapped)
-            return false;
 
-        // remove outdated forwardings
+    m_portProfiles[profile] = ports;
-        for (const lt::port_mapping_t &handle : handles)
+    const QSet<quint16> newForwardedPorts = std::accumulate(m_portProfiles.cbegin(), m_portProfiles.cend(), QSet<quint16>());
-            m_provider->delete_port_mapping(handle);
-        m_forwardedPorts.remove(port);
 
-        return true;
+    m_provider->removeMappedPorts(oldForwardedPorts - newForwardedPorts);
-    });
+    m_provider->addMappedPorts(newForwardedPorts - oldForwardedPorts);
-
-    // add new forwardings
-    for (const quint16 port : ports)
-    {
-        // port already forwarded/taken by other profile, don't do anything
-        if (m_forwardedPorts.contains(port))
-            continue;
-
-        if (isEnabled())
-            portMapping.insert(port, m_provider->add_port_mapping(lt::session::tcp, port, port));
-        else
-            portMapping.insert(port, {});
-        m_forwardedPorts.insert(port);
-    }
-
-    if (portMapping.isEmpty())
-        m_portProfiles.remove(profile);
 }
 
 void PortForwarderImpl::removePorts(const QString &profile)
@@ -107,40 +81,12 @@ void PortForwarderImpl::removePorts(const QString &profile)
 
 void PortForwarderImpl::start()
 {
-    lt::settings_pack settingsPack;
+    m_provider->enablePortMapping();
-    settingsPack.set_bool(lt::settings_pack::enable_upnp, true);
+    for (const QSet<quint16> &ports : asConst(m_portProfiles))
-    settingsPack.set_bool(lt::settings_pack::enable_natpmp, true);
+        m_provider->addMappedPorts(ports);
-    m_provider->apply_settings(std::move(settingsPack));
-
-    for (auto profileIter = m_portProfiles.begin(); profileIter != m_portProfiles.end(); ++profileIter)
-    {
-        PortMapping &portMapping = profileIter.value();
-        for (auto iter = portMapping.begin(); iter != portMapping.end(); ++iter)
-        {
-            Q_ASSERT(iter.value().empty());
-
-            const quint16 port = iter.key();
-            iter.value() = m_provider->add_port_mapping(lt::session::tcp, port, port);
-        }
-    }
-
-    LogMsg(tr("UPnP/NAT-PMP support: ON"), Log::INFO);
 }
 
 void PortForwarderImpl::stop()
 {
-    lt::settings_pack settingsPack;
+    m_provider->disablePortMapping();
-    settingsPack.set_bool(lt::settings_pack::enable_upnp, false);
-    settingsPack.set_bool(lt::settings_pack::enable_natpmp, false);
-    m_provider->apply_settings(std::move(settingsPack));
-
-    // don't clear m_portProfiles so a later `start()` call can restore the port forwardings
-    for (auto profileIter = m_portProfiles.begin(); profileIter != m_portProfiles.end(); ++profileIter)
-    {
-        PortMapping &portMapping = profileIter.value();
-        for (auto iter = portMapping.begin(); iter != portMapping.end(); ++iter)
-            iter.value().clear();
-    }
-
-    LogMsg(tr("UPnP/NAT-PMP support: OFF"), Log::INFO);
 }

src/base/bittorrent/portforwarderimpl.h

@@ -1,6 +1,6 @@
 /*
  * Bittorrent Client using Qt and libtorrent.
- * Copyright (C) 2019  Vladimir Golovnev <glassez@yandex.ru>
+ * Copyright (C) 2019-2022  Vladimir Golovnev <glassez@yandex.ru>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -28,24 +28,24 @@
 
 #pragma once
 
-#include <vector>
-
-#include <libtorrent/fwd.hpp>
-#include <libtorrent/portmap.hpp>
-
 #include <QHash>
 #include <QSet>
 
 #include "base/net/portforwarder.h"
 #include "base/settingvalue.h"
 
+namespace BitTorrent
+{
+    class SessionImpl;
+}
+
 class PortForwarderImpl final : public Net::PortForwarder
 {
     Q_OBJECT
     Q_DISABLE_COPY_MOVE(PortForwarderImpl)
 
 public:
-    explicit PortForwarderImpl(lt::session *provider, QObject *parent = nullptr);
+    explicit PortForwarderImpl(BitTorrent::SessionImpl *provider, QObject *parent = nullptr);
     ~PortForwarderImpl() override;
 
     bool isEnabled() const override;
@@ -59,9 +59,7 @@ private:
     void stop();
 
     CachedSettingValue<bool> m_storeActive;
-    lt::session *const m_provider = nullptr;
 
-    using PortMapping = QHash<quint16, std::vector<lt::port_mapping_t>>;  // <port, handles>
+    BitTorrent::SessionImpl *const m_provider = nullptr;
-    QHash<QString, PortMapping> m_portProfiles;
+    QHash<QString, QSet<quint16>> m_portProfiles;
-    QSet<quint16> m_forwardedPorts;
 };

src/base/bittorrent/sessionimpl.cpp

@@ -577,7 +577,7 @@ SessionImpl::SessionImpl(QObject *parent)
     loadStatistics();
 
     // initialize PortForwarder instance
-    new PortForwarderImpl(m_nativeSession);
+    new PortForwarderImpl(this);
 
     // start embedded tracker
     enableTracker(isTrackerEnabled());
@@ -2822,6 +2822,78 @@ void SessionImpl::findIncompleteFiles(const TorrentInfo &torrentInfo, const Path
     });
 }
 
+void SessionImpl::enablePortMapping()
+{
+    invokeAsync([this]
+    {
+        if (m_isPortMappingEnabled)
+            return;
+
+        lt::settings_pack settingsPack;
+        settingsPack.set_bool(lt::settings_pack::enable_upnp, true);
+        settingsPack.set_bool(lt::settings_pack::enable_natpmp, true);
+        m_nativeSession->apply_settings(std::move(settingsPack));
+
+        m_isPortMappingEnabled = true;
+
+        LogMsg(tr("UPnP/NAT-PMP support: ON"), Log::INFO);
+    });
+}
+
+void SessionImpl::disablePortMapping()
+{
+    invokeAsync([this]
+    {
+        if (!m_isPortMappingEnabled)
+            return;
+
+        lt::settings_pack settingsPack;
+        settingsPack.set_bool(lt::settings_pack::enable_upnp, false);
+        settingsPack.set_bool(lt::settings_pack::enable_natpmp, false);
+        m_nativeSession->apply_settings(std::move(settingsPack));
+
+        m_mappedPorts.clear();
+        m_isPortMappingEnabled = false;
+
+        LogMsg(tr("UPnP/NAT-PMP support: OFF"), Log::INFO);
+    });
+}
+
+void SessionImpl::addMappedPorts(const QSet<quint16> &ports)
+{
+    invokeAsync([this, ports]
+    {
+        if (!m_isPortMappingEnabled)
+            return;
+
+        for (const quint16 port : ports)
+        {
+            if (!m_mappedPorts.contains(port))
+                m_mappedPorts.insert(port, m_nativeSession->add_port_mapping(lt::session::tcp, port, port));
+        }
+    });
+}
+
+void SessionImpl::removeMappedPorts(const QSet<quint16> &ports)
+{
+    invokeAsync([this, ports]
+    {
+        if (!m_isPortMappingEnabled)
+            return;
+
+        Algorithm::removeIf(m_mappedPorts, [this, ports](const quint16 port, const std::vector<lt::port_mapping_t> &handles)
+        {
+            if (!ports.contains(port))
+                return false;
+
+            for (const lt::port_mapping_t &handle : handles)
+                m_nativeSession->delete_port_mapping(handle);
+
+            return true;
+        });
+    });
+}
+
 void SessionImpl::invokeAsync(std::function<void ()> func)
 {
     m_asyncWorker->start(std::move(func));

src/base/bittorrent/sessionimpl.h

@@ -33,6 +33,7 @@
 #include <vector>
 
 #include <libtorrent/fwd.hpp>
+#include <libtorrent/portmap.hpp>
 #include <libtorrent/torrent_handle.hpp>
 
 #include <QElapsedTimer>
@@ -432,6 +433,11 @@ namespace BitTorrent
         void findIncompleteFiles(const TorrentInfo &torrentInfo, const Path &savePath
                                  , const Path &downloadPath, const PathList &filePaths = {}) const;
 
+        void enablePortMapping();
+        void disablePortMapping();
+        void addMappedPorts(const QSet<quint16> &ports);
+        void removeMappedPorts(const QSet<quint16> &ports);
+
         void invokeAsync(std::function<void ()> func);
 
     private slots:
@@ -734,6 +740,12 @@ namespace BitTorrent
 
         bool m_needUpgradeDownloadPath = false;
 
+        // All port mapping related routines are invoked from working thread
+        // so there are no synchronization used. If multithreaded access is
+        // ever required, synchronization should also be provided.
+        bool m_isPortMappingEnabled = false;
+        QHash<quint16, std::vector<lt::port_mapping_t>> m_mappedPorts;
+
         friend void Session::initInstance();
         friend void Session::freeInstance();
         friend Session *Session::instance();