Keep track of used subnets

If we don't keep track of them, inserting two devices at the same time
can lead to both picking the same candidate. This happens if the host
does not get an IP in time and thus the network is still free from host
perspective as the second call looks for the subnet to use.

The tracking is done by UsbDevice and DeviceInformation keeping a
SubnetReservation. The SubnetReservation is gotten from
findUnusedSubnet(), which uses a SubnetPool singleton to keep track of
them. When the SubnetReservations are destructed, they call SubnetPool
to free the subnet the reserved.

Task-number: QTBUG-59449
Change-Id: I4b28ade4fa7a5660bd699882398facafafc9d795
Reviewed-by: Samuli Piippo <samuli.piippo@qt.io>

8 files changed, 209 insertions, 32 deletions

diff --git a/qdb/server/devicemanager.cpp b/qdb/server/devicemanager.cpp
index b468383..56ab7df 100644
--- a/qdb/server/devicemanager.cpp
+++ b/qdb/server/devicemanager.cpp
@@ -77,7 +77,8 @@ void DeviceManager::handleDeviceInformation(UsbDevice device, DeviceInformationF
                              });
     if (iter == m_deviceInfos.end()) {
         qCDebug(devicesC) << "Added new info for" << info.serial;
-        DeviceInformation newInfo{info.serial, info.hostMac, info.ipAddress, device.address};
+        DeviceInformation newInfo{info.serial, info.hostMac, info.ipAddress, device.address,
+                                  device.reservation};
         m_deviceInfos.push_back(newInfo);
         emit newDeviceInfo(newInfo);
     } else if (iter->hostMac != info.hostMac || iter->ipAddress != info.ipAddress) {
diff --git a/qdb/server/devicemanager.h b/qdb/server/devicemanager.h
index e5f52b2..00b6239 100644
--- a/qdb/server/devicemanager.h
+++ b/qdb/server/devicemanager.h
@@ -34,6 +34,7 @@ struct DeviceInformation
     QString hostMac;
     QString ipAddress;
     UsbAddress usbAddress;
+    SubnetReservation reservation;
 };
 
 class DeviceManager : public QObject
diff --git a/qdb/server/networkconfigurator.cpp b/qdb/server/networkconfigurator.cpp
index f8dffb4..cbcac35 100644
--- a/qdb/server/networkconfigurator.cpp
+++ b/qdb/server/networkconfigurator.cpp
@@ -45,15 +45,16 @@ void NetworkConfigurator::configure()
         return;
     }
 
-    const std::pair<Subnet, bool> configuration = findUnusedSubnet();
-    if (!configuration.second) {
+    SubnetReservation reservation = findUnusedSubnet();
+    if (!reservation) {
         qCCritical(configuratorC) << "Could not find a free subnet to use for the network of device"
                                   << m_device.serial;
         emit configured(m_device, false);
         return;
     }
 
-    const auto &subnet = configuration.first;
+    m_device.reservation = reservation;
+    const auto subnet = reservation->subnet();
     const auto subnetString
             = QString{"%1/%2"}.arg(subnet.address.toString()).arg(subnet.prefixLength);
 
diff --git a/qdb/server/subnet.cpp b/qdb/server/subnet.cpp
index 61c80b8..4752779 100644
--- a/qdb/server/subnet.cpp
+++ b/qdb/server/subnet.cpp
@@ -36,28 +36,16 @@ std::vector<Subnet> fetchUsedSubnets()
 
 } // anonymous namespace
 
-std::pair<Subnet, bool> findUnusedSubnet()
+SubnetReservation findUnusedSubnet()
 {
+    SubnetPool *pool = SubnetPool::instance();
     const std::vector<Subnet> usedSubnets = fetchUsedSubnets();
-    const std::vector<Subnet> candidateSubnets = {{QHostAddress{"172.16.58.1"}, 30},
-                                                  {QHostAddress{"172.17.58.1"}, 30},
-                                                  {QHostAddress{"172.18.58.1"}, 30},
-                                                  {QHostAddress{"172.19.58.1"}, 30},
-                                                  {QHostAddress{"172.20.58.1"}, 30},
-                                                  {QHostAddress{"172.21.58.1"}, 30},
-                                                  {QHostAddress{"172.22.58.1"}, 30},
-                                                  {QHostAddress{"172.23.58.1"}, 30},
-                                                  {QHostAddress{"172.24.58.1"}, 30},
-                                                  {QHostAddress{"172.25.58.1"}, 30},
-                                                  {QHostAddress{"172.26.58.1"}, 30},
-                                                  {QHostAddress{"172.27.58.1"}, 30},
-                                                  {QHostAddress{"172.28.58.1"}, 30},
-                                                  {QHostAddress{"172.29.58.1"}, 30},
-                                                  {QHostAddress{"172.30.58.1"}, 30},
-                                                  {QHostAddress{"172.31.58.1"}, 30},
-                                                  {QHostAddress{"192.168.58.1"}, 30},
-                                                  {QHostAddress{"10.17.20.1"}, 30}};
-    return findUnusedSubnet(candidateSubnets, usedSubnets);
+    const std::pair<Subnet, bool> result = findUnusedSubnet(pool->candidates(),
+                                                            usedSubnets);
+    if (!result.second)
+        return SubnetReservation{};
+
+    return pool->reserve(result.first);
 }
 
 std::pair<Subnet, bool> findUnusedSubnet(const std::vector<Subnet> &candidateSubnets,
@@ -75,3 +63,92 @@ std::pair<Subnet, bool> findUnusedSubnet(const std::vector<Subnet> &candidateSub
     }
     return std::make_pair(Subnet{}, false);
 }
+
+SubnetPool *SubnetPool::instance()
+{
+    static SubnetPool pool;
+    return &pool;
+}
+
+std::vector<Subnet> SubnetPool::candidates()
+{
+    QMutexLocker locker{&m_lock};
+    std::vector<Subnet> freeCandidates;
+    freeCandidates.reserve(m_candidates.size() - m_reserved.size());
+    std::copy_if(m_candidates.begin(), m_candidates.end(),
+                 std::back_inserter(freeCandidates),
+                 [&](const Subnet &subnet) {
+                     return std::find(m_reserved.begin(), m_reserved.end(), subnet)
+                             == m_reserved.end();
+                 });
+    return freeCandidates;
+}
+
+SubnetReservation SubnetPool::reserve(const Subnet &subnet)
+{
+    QMutexLocker locker{&m_lock};
+
+    const auto iter = std::find(m_reserved.begin(), m_reserved.end(), subnet);
+    if (iter != m_reserved.end()) // Already reserved
+        return SubnetReservation{};
+
+    m_reserved.push_back(subnet);
+    return std::make_shared<SubnetReservationImpl>(subnet);
+}
+
+void SubnetPool::free(const Subnet &subnet)
+{
+    QMutexLocker locker{&m_lock};
+
+    const auto iter = std::find(m_reserved.begin(), m_reserved.end(), subnet);
+    if (iter != m_reserved.end())
+        m_reserved.erase(iter);
+}
+
+SubnetPool::SubnetPool()
+    : m_lock{},
+      m_candidates(),
+      m_reserved{}
+{
+    // MSVC 2013 gave error C2707 when trying to use initializer list for m_candidates
+    m_candidates.push_back({QHostAddress{"172.16.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.17.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.18.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.19.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.20.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.21.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.22.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.23.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.24.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.25.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.26.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.27.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.28.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.29.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.30.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"172.31.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"192.168.58.1"}, 30});
+    m_candidates.push_back({QHostAddress{"10.17.20.1"}, 30});
+}
+
+bool operator==(const Subnet &lhs, const Subnet &rhs)
+{
+    return lhs.address == rhs.address
+            && lhs.prefixLength == rhs.prefixLength;
+}
+
+SubnetReservationImpl::SubnetReservationImpl(const Subnet &subnet)
+    : m_subnet(subnet) // uniform initialization with {} fails in MSVC 2013 with error C2797
+{
+
+}
+
+SubnetReservationImpl::~SubnetReservationImpl()
+{
+    SubnetPool::instance()->free(m_subnet);
+}
+
+Subnet SubnetReservationImpl::subnet() const
+{
+    return m_subnet;
+}
diff --git a/qdb/server/subnet.h b/qdb/server/subnet.h
index bc8833d..66f5196 100644
--- a/qdb/server/subnet.h
+++ b/qdb/server/subnet.h
@@ -21,8 +21,11 @@
 #ifndef SUBNET_H
 #define SUBNET_H
 
+#include <QtCore/qmutex.h>
 #include <QtNetwork/qhostaddress.h>
 
+#include <memory>
+
 struct Subnet
 {
     QHostAddress address;
@@ -30,7 +33,41 @@ struct Subnet
 };
 Q_DECLARE_METATYPE(Subnet)
 
-std::pair<Subnet, bool> findUnusedSubnet();
+bool operator==(const Subnet &lhs, const Subnet &rhs);
+
+class SubnetReservationImpl
+{
+public:
+    SubnetReservationImpl(const Subnet &subnet);
+    ~SubnetReservationImpl();
+
+    Subnet subnet() const;
+
+private:
+    Subnet m_subnet;
+};
+
+using SubnetReservation = std::shared_ptr<SubnetReservationImpl>;
+
+class SubnetPool
+{
+public:
+    static SubnetPool *instance();
+
+    std::vector<Subnet> candidates();
+    SubnetReservation reserve(const Subnet &subnet);
+    void free(const Subnet &subnet);
+
+private:
+    SubnetPool();
+    SubnetPool(const std::vector<Subnet> &subnet);
+
+    QMutex m_lock;
+    std::vector<Subnet> m_candidates;
+    std::vector<Subnet> m_reserved;
+};
+
+SubnetReservation findUnusedSubnet();
 std::pair<Subnet, bool> findUnusedSubnet(const std::vector<Subnet> &candidateSubnets,
                                          const std::vector<Subnet> &usedSubnets);
 
diff --git a/qdb/server/usb-host/usbdevice.h b/qdb/server/usb-host/usbdevice.h
index e44d3f3..69604d3 100644
--- a/qdb/server/usb-host/usbdevice.h
+++ b/qdb/server/usb-host/usbdevice.h
@@ -21,6 +21,7 @@
 #ifndef USBDEVICE_H
 #define USBDEVICE_H
 
+#include "../subnet.h"
 #include "usbcommon.h"
 
 #include <QtCore/qstring.h>
@@ -63,6 +64,7 @@ struct UsbDevice
     UsbAddress address;
     LibUsbDevice usbDevice;
     UsbInterfaceInfo interfaceInfo;
+    SubnetReservation reservation;
 };
 
 #endif // USBDEVICE_H
diff --git a/qdb/server/usb-host/usbdeviceenumerator.cpp b/qdb/server/usb-host/usbdeviceenumerator.cpp
index 1ed346a..c87813f 100644
--- a/qdb/server/usb-host/usbdeviceenumerator.cpp
+++ b/qdb/server/usb-host/usbdeviceenumerator.cpp
@@ -135,7 +135,8 @@ std::pair<bool, UsbDevice> makeUsbDeviceIfQdbDevice(libusb_device *device)
     const auto address = getAddress(device);
     const auto serial = getSerialNumber(device, handle);
 
-    const UsbDevice usbDevice{serial, address, LibUsbDevice{device}, interfaceResult.second};
+    const UsbDevice usbDevice{serial, address, LibUsbDevice{device}, interfaceResult.second,
+                              SubnetReservation{}};
     return std::make_pair(true, usbDevice);
 }
 
diff --git a/tests/tst_subnet.cpp b/tests/tst_subnet.cpp
index 5aa36cf..90733f0 100644
--- a/tests/tst_subnet.cpp
+++ b/tests/tst_subnet.cpp
@@ -25,12 +25,6 @@
 
 using Subnets = std::vector<Subnet>;
 
-bool operator==(const Subnet &lhs, const Subnet &rhs)
-{
-    return lhs.address == rhs.address
-            && lhs.prefixLength == rhs.prefixLength;
-}
-
 class tst_Subnet : public QObject
 {
     Q_OBJECT
@@ -40,6 +34,9 @@ private slots:
     void freeSubnets_data();
     void usedSubnets();
     void usedSubnets_data();
+    void reserveOne();
+    void reserveOne_data();
+    void reserveSome();
 };
 
 void tst_Subnet::freeSubnets()
@@ -95,6 +92,66 @@ void tst_Subnet::usedSubnets_data()
     QTest::newRow("3") << candidates << subnets;
 }
 
+void tst_Subnet::reserveOne()
+{
+    QFETCH(Subnet, subnet);
+
+    SubnetPool *pool = SubnetPool::instance();
+    const auto amountOfCandidates = pool->candidates().size();
+    QVERIFY(amountOfCandidates >= 1);
+    {
+        const auto reservation = pool->reserve(subnet);
+        const auto candidates = pool->candidates();
+        QCOMPARE(candidates.size(), amountOfCandidates - 1);
+        QVERIFY(std::none_of(candidates.begin(), candidates.end(),
+                             [&](const Subnet &other) {
+                                 return subnet == other;
+                             }));
+    }
+    const auto candidates = pool->candidates();
+    QCOMPARE(candidates.size(), amountOfCandidates);
+    QVERIFY(std::find(candidates.begin(), candidates.end(), subnet) != candidates.end());
+}
+
+void tst_Subnet::reserveOne_data()
+{
+    QTest::addColumn<Subnet>("subnet");
+    QTest::newRow("first") << Subnet{QHostAddress{"172.16.58.1"}, 30};
+    QTest::newRow("middle") << Subnet{QHostAddress{"172.25.58.1"}, 30};
+    QTest::newRow("last") << Subnet{QHostAddress{"10.17.20.1"}, 30};
+}
+
+void tst_Subnet::reserveSome()
+{
+    SubnetPool *pool = SubnetPool::instance();
+    const Subnets subnetsToReserve {{QHostAddress{"172.18.58.1"}, 30},
+                                    {QHostAddress{"172.19.58.1"}, 30},
+                                    {QHostAddress{"172.20.58.1"}, 30}};
+    const auto amountOfCandidates = pool->candidates().size();
+
+    std::vector<SubnetReservation> reservations;
+    for (const Subnet &subnet : subnetsToReserve)
+    {
+        reservations.push_back(pool->reserve(subnet));
+        const auto candidates = pool->candidates();
+        QCOMPARE(candidates.size(), amountOfCandidates - reservations.size());
+        QVERIFY(std::find(candidates.begin(), candidates.end(), subnet) == candidates.end());
+    }
+    const auto candidates = pool->candidates();
+    QCOMPARE(candidates.size(), amountOfCandidates - reservations.size());
+    QVERIFY(std::none_of(candidates.begin(), candidates.end(),
+                         [&](const Subnet &subnet) {
+                             return std::find(subnetsToReserve.begin(),
+                                              subnetsToReserve.end(),
+                                              subnet)
+                                     != subnetsToReserve.end();
+                         }));
+
+    reservations.clear();
+
+    QCOMPARE(pool->candidates().size(), amountOfCandidates);
+}
+
 QTEST_APPLESS_MAIN(tst_Subnet)
 
 #include "tst_subnet.moc"