Hold reference count of monitor objects

Make host lookup optional
Cleanup pending ping on monitor unregister

libnymea-core/hardware/network/networkdevicediscoveryimpl.cpp

@@ -146,16 +146,21 @@ bool NetworkDeviceDiscoveryImpl::running() const
 
 NetworkDeviceMonitor *NetworkDeviceDiscoveryImpl::registerMonitor(const MacAddress &macAddress)
 {
-    qCInfo(dcNetworkDeviceDiscovery()) << "Register new network device monitor for" << macAddress;
-    // Make sure we create only one monitor per MAC
-    if (m_monitors.contains(macAddress))
-        return m_monitors.value(macAddress);
-
     if (macAddress.isNull()) {
         qCWarning(dcNetworkDeviceDiscovery()) << "Could not register monitor for invalid" << macAddress;
         return nullptr;
     }
 
+    // Make sure we create only one monitor per MAC and keep track how many user
+    // have access to this monitor otherwise an unregister could cause a crash in
+    // an other plugin plugin which might still need it
+    if (m_monitors.contains(macAddress)) {
+        m_monitorsReferenceCount[macAddress] += 1;
+        qCInfo(dcNetworkDeviceDiscovery()) << "Register network device monitor for" << macAddress << "which already exists. Returning existing monitor having now" << m_monitorsReferenceCount[macAddress] << "references.";
+        return m_monitors.value(macAddress);
+    }
+
+    qCInfo(dcNetworkDeviceDiscovery()) << "Register new network device monitor for" << macAddress;
 
     // Fill in cached information
     NetworkDeviceInfo info;
@@ -168,6 +173,7 @@ NetworkDeviceMonitor *NetworkDeviceDiscoveryImpl::registerMonitor(const MacAddre
     NetworkDeviceMonitorImpl *monitor = new NetworkDeviceMonitorImpl(macAddress, this);
     monitor->setNetworkDeviceInfo(info);
     m_monitors.insert(macAddress, monitor);
+    m_monitorsReferenceCount[macAddress] = 1;
 
     if (!available()) {
         qCWarning(dcNetworkDeviceDiscovery()) << "Registered monitor but the hardware resource is not available. The monitor will not work as expected" << monitor;
@@ -189,10 +195,19 @@ NetworkDeviceMonitor *NetworkDeviceDiscoveryImpl::registerMonitor(const MacAddre
 
 void NetworkDeviceDiscoveryImpl::unregisterMonitor(const MacAddress &macAddress)
 {
+    if (m_monitorsReferenceCount.contains(macAddress)) {
+        m_monitorsReferenceCount[macAddress] -= 1;
+        if (m_monitorsReferenceCount[macAddress] > 0) {
+            qCDebug(dcNetworkDeviceDiscovery()) << "Unregistered monitor for" << macAddress.toString() << "but keeping the monitor. There are still" << m_monitorsReferenceCount[macAddress] << "references to it.";
+            return;
+        }
+    }
+
     if (m_monitors.contains(macAddress)) {
         NetworkDeviceMonitor *monitor = m_monitors.take(macAddress);
         qCInfo(dcNetworkDeviceDiscovery()) << "Unregister" << monitor;
         monitor->deleteLater();
+        m_monitorsReferenceCount.remove(macAddress);
     }
 }
 
@@ -203,32 +218,17 @@ void NetworkDeviceDiscoveryImpl::unregisterMonitor(NetworkDeviceMonitor *network
 
 PingReply *NetworkDeviceDiscoveryImpl::ping(const QHostAddress &address, uint retries)
 {
-    // Note: we use any ping used trough this method also for the monitor evaluation
     PingReply *reply = m_ping->ping(address, retries);
-    connect(reply, &PingReply::finished, this, [=](){
+    // Note: we use any ping used trough this method also for the monitor evaluation
-
+    watchPingReply(reply);
-        // Search cache for mac address and update last seen
+    return reply;
-        if (reply->error() == PingReply::ErrorNoError) {
+}
-            foreach (const NetworkDeviceInfo &info, m_networkInfoCache) {
-                if (info.address() == address) {
-                    // Found info for this ip, update the cache
-                    MacAddress macAddress(info.macAddress());
-                    if (!macAddress.isNull() && m_networkInfoCache.contains(macAddress)) {
-                        m_lastSeen[macAddress] = QDateTime::currentDateTime();
-                        saveNetworkDeviceCache(m_networkInfoCache.value(macAddress));
-                    }
-                }
-            }
-        }
-
-        // Update any monitor
-        foreach (NetworkDeviceMonitorImpl *monitor, m_monitors.values()) {
-            if (monitor->networkDeviceInfo().address() == address) {
-                processMonitorPingResult(reply, monitor);
-            }
-        }
-    });
 
+PingReply *NetworkDeviceDiscoveryImpl::ping(const QHostAddress &address, bool lookupHost, uint retries)
+{
+    PingReply *reply = m_ping->ping(address, lookupHost, retries);
+    // Note: we use any ping used trough this method also for the monitor evaluation
+    watchPingReply(reply);
     return reply;
 }
 
@@ -304,8 +304,8 @@ void NetworkDeviceDiscoveryImpl::pingAllNetworkDevices()
                 if (targetAddress == entry.ip())
                     continue;
 
-                // Retry only once to ping a device
+                // Retry only once to ping a device and lookup the hostname on success
-                PingReply *reply = ping(targetAddress, 1);
+                PingReply *reply = ping(targetAddress, true, 1);
                 m_runningPingRepies.append(reply);
                 connect(reply, &PingReply::finished, this, [=](){
                     m_runningPingRepies.removeAll(reply);
@@ -339,6 +339,32 @@ void NetworkDeviceDiscoveryImpl::processMonitorPingResult(PingReply *reply, Netw
     }
 }
 
+void NetworkDeviceDiscoveryImpl::watchPingReply(PingReply *reply)
+{
+    connect(reply, &PingReply::finished, this, [=](){
+        // Search cache for mac address and update last seen
+        if (reply->error() == PingReply::ErrorNoError) {
+            foreach (const NetworkDeviceInfo &info, m_networkInfoCache) {
+                if (info.address() == reply->targetHostAddress()) {
+                    // Found info for this ip, update the cache
+                    MacAddress macAddress(info.macAddress());
+                    if (!macAddress.isNull() && m_networkInfoCache.contains(macAddress)) {
+                        m_lastSeen[macAddress] = QDateTime::currentDateTime();
+                        saveNetworkDeviceCache(m_networkInfoCache.value(macAddress));
+                    }
+                }
+            }
+        }
+
+        // Update any monitor
+        foreach (NetworkDeviceMonitorImpl *monitor, m_monitors.values()) {
+            if (monitor->networkDeviceInfo().address() == reply->targetHostAddress()) {
+                processMonitorPingResult(reply, monitor);
+            }
+        }
+    });
+}
+
 void NetworkDeviceDiscoveryImpl::loadNetworkDeviceCache()
 {
     qCInfo(dcNetworkDeviceDiscovery()) << "Loading cached network device information from" << m_cacheSettings->fileName();
@@ -403,6 +429,9 @@ void NetworkDeviceDiscoveryImpl::removeFromNetworkDeviceCache(const MacAddress &
 
 void NetworkDeviceDiscoveryImpl::saveNetworkDeviceCache(const NetworkDeviceInfo &deviceInfo)
 {
+    if (!deviceInfo.isValid() || !deviceInfo.isComplete())
+        return;
+
     m_cacheSettings->beginGroup("NetworkDeviceInfos");
     m_cacheSettings->beginGroup(deviceInfo.macAddress());
     m_cacheSettings->setValue("address", deviceInfo.address().toString());
@@ -435,6 +464,12 @@ void NetworkDeviceDiscoveryImpl::updateCache(const NetworkDeviceInfo &deviceInfo
 
 void NetworkDeviceDiscoveryImpl::evaluateMonitor(NetworkDeviceMonitorImpl *monitor)
 {
+    if (monitor->networkDeviceInfo().address().isNull())
+        return;
+
+    if (monitor->currentPingReply())
+        return;
+
     // Start action if we have not seen the device for gracePeriod seconds
     QDateTime currentDateTime = QDateTime::currentDateTime();
 
@@ -452,12 +487,6 @@ void NetworkDeviceDiscoveryImpl::evaluateMonitor(NetworkDeviceMonitorImpl *monit
     if (!requiresRefresh)
         return;
 
-    if (monitor->networkDeviceInfo().address().isNull())
-        return;
-
-    if (monitor->currentPingReply())
-        return;
-
     // Try to ping first
     qCDebug(dcNetworkDeviceDiscovery()) << monitor << "try to ping" << monitor->networkDeviceInfo().address().toString();
     PingReply *reply = m_ping->ping(monitor->networkDeviceInfo().address(), monitor->pingRetries());

libnymea-core/hardware/network/networkdevicediscoveryimpl.h

@@ -72,6 +72,7 @@ public:
     void unregisterMonitor(NetworkDeviceMonitor *networkDeviceMonitor) override;
 
     PingReply *ping(const QHostAddress &address, uint retries = 3) override;
+    PingReply *ping(const QHostAddress &address, bool lookupHost, uint retries = 3);
 
     MacAddressDatabaseReply *lookupMacAddress(const QString &macAddress) override;
     MacAddressDatabaseReply *lookupMacAddress(const MacAddress &macAddress) override;
@@ -104,6 +105,7 @@ private:
     QList<PingReply *> m_runningPingRepies;
 
     QHash<MacAddress, NetworkDeviceMonitorImpl *> m_monitors;
+    QHash<MacAddress, int> m_monitorsReferenceCount;
     QHash<MacAddress, QDateTime> m_lastSeen;
 
     QSettings *m_cacheSettings;
@@ -113,6 +115,8 @@ private:
 
     void processMonitorPingResult(PingReply *reply, NetworkDeviceMonitorImpl *monitor);
 
+    void watchPingReply(PingReply *reply);
+
     void loadNetworkDeviceCache();
     void removeFromNetworkDeviceCache(const MacAddress &macAddress);
     void saveNetworkDeviceCache(const NetworkDeviceInfo &deviceInfo);

libnymea-core/hardware/network/networkdevicemonitorimpl.cpp

@@ -44,7 +44,9 @@ NetworkDeviceMonitorImpl::NetworkDeviceMonitorImpl(const MacAddress &macAddress,
 
 NetworkDeviceMonitorImpl::~NetworkDeviceMonitorImpl()
 {
-
+    if (m_currentPingReply) {
+        m_currentPingReply->abort();
+    }
 }
 
 MacAddress NetworkDeviceMonitorImpl::macAddress() const

libnymea-core/hardware/network/networkdevicemonitorimpl.h

@@ -45,7 +45,7 @@ class NetworkDeviceMonitorImpl : public NetworkDeviceMonitor
 
 public:
     explicit NetworkDeviceMonitorImpl(const MacAddress &macAddress, QObject *parent = nullptr);
-    ~NetworkDeviceMonitorImpl();
+    ~NetworkDeviceMonitorImpl() override;
 
     MacAddress macAddress() const override;
 

libnymea/network/ping.cpp

@@ -113,19 +113,21 @@ PingReply::Error Ping::error() const
 
 PingReply *Ping::ping(const QHostAddress &hostAddress, uint retries)
 {
-    PingReply *reply = new PingReply(this);
+    PingReply *reply = createReply(hostAddress);
-    reply->m_targetHostAddress = hostAddress;
-    reply->m_networkInterface = NetworkUtils::getInterfaceForHostaddress(hostAddress);
     reply->m_retries = retries;
 
-    connect(reply, &PingReply::timeout, this, [=](){
+    // Perform the reply in the next event loop to give the user time to do the reply connects
-        // Note: this is not the ICMP timeout, here we actually got nothing from nobody...
+    m_replyQueue.enqueue(reply);
-        finishReply(reply, PingReply::ErrorTimeout);
+    sendNextReply();
-    });
 
-    connect(reply, &PingReply::aborted, this, [=](){
+    return reply;
-        finishReply(reply, PingReply::ErrorAborted);
+}
-    });
+
+PingReply *Ping::ping(const QHostAddress &hostAddress, bool lookupHost, uint retries)
+{
+    PingReply *reply = createReply(hostAddress);
+    reply->m_retries = retries;
+    reply->m_doHostLookup = lookupHost;
 
     // Perform the reply in the next event loop to give the user time to do the reply connects
     m_replyQueue.enqueue(reply);
@@ -143,7 +145,7 @@ void Ping::sendNextReply()
         return;
 
     PingReply *reply = m_replyQueue.dequeue();
-    //qCDebug(dcPing()) << "Send next reply," << m_replyQueue.count() << "left in queue";
+    qCDebug(dcPing()) << "Send next reply," << m_replyQueue.count() << "left in queue";
     m_queueTimer->start();
     QTimer::singleShot(0, reply, [=]() { performPing(reply); });
 }
@@ -294,6 +296,24 @@ quint16 Ping::calculateRequestId()
     return requestId;
 }
 
+PingReply *Ping::createReply(const QHostAddress &hostAddress)
+{
+    PingReply *reply = new PingReply(this);
+    reply->m_targetHostAddress = hostAddress;
+    reply->m_networkInterface = NetworkUtils::getInterfaceForHostaddress(hostAddress);
+
+    connect(reply, &PingReply::timeout, this, [=](){
+        // Note: this is not the ICMP timeout, here we actually got nothing from nobody...
+        finishReply(reply, PingReply::ErrorTimeout);
+    });
+
+    connect(reply, &PingReply::aborted, this, [=](){
+        finishReply(reply, PingReply::ErrorAborted);
+    });
+
+    return reply;
+}
+
 void Ping::finishReply(PingReply *reply, PingReply::Error error)
 {
     // Check if we should retry
@@ -387,15 +407,21 @@ void Ping::onSocketReadyRead(int socketDescriptor)
             timeValueSubtract(&receiveTimeValue, &reply->m_startTime);
             reply->m_duration = qRound((receiveTimeValue.tv_sec * 1000 + (double)receiveTimeValue.tv_usec / 1000) * 100) / 100.0;
 
-            // Note: due to a Qt bug < 5.9 we need to use old SLOT style and cannot make use of lambda here
-            int lookupId = QHostInfo::lookupHost(senderAddress.toString(), this, SLOT(onHostLookupFinished(QHostInfo)));
-            m_pendingHostLookups.insert(lookupId, reply);
-
             qCDebug(dcPing()) << "Received ICMP response" << reply->targetHostAddress().toString() << ICMP_PACKET_SIZE << "[Bytes]"
                           << "ID:" << QString("0x%1").arg(responsePacket->icmp_id, 4, 16, QChar('0'))
                           << "Sequence:" << htons(responsePacket->icmp_seq)
                           << "Time:" << reply->duration() << "[ms]";
 
+            if (reply->doHostLookup()) {
+                // Note: due to a Qt bug < 5.9 we need to use old SLOT style and cannot make use of lambda here
+                int lookupId = QHostInfo::lookupHost(senderAddress.toString(), this, SLOT(onHostLookupFinished(QHostInfo)));
+                m_pendingHostLookups.insert(lookupId, reply);
+            } else {
+                finishReply(reply, PingReply::ErrorNoError);
+            }
+
+
+
         } else if (responsePacket->icmp_type == ICMP_DEST_UNREACH) {
 
             // Get the sending package

libnymea/network/ping.h

@@ -63,6 +63,7 @@ public:
     PingReply::Error error() const;
 
     PingReply *ping(const QHostAddress &hostAddress, uint retries = 3);
+    PingReply *ping(const QHostAddress &hostAddress, bool lookupHost, uint retries = 3);
 
 signals:
     void availableChanged(bool available);
@@ -99,6 +100,7 @@ private:
     void timeValueSubtract(struct timeval *start, struct timeval *stop);
     quint16 calculateRequestId();
 
+    PingReply *createReply(const QHostAddress &hostAddress);
     void finishReply(PingReply *reply, PingReply::Error error);
 
 private slots:

libnymea/network/pingreply.cpp

@@ -83,6 +83,11 @@ PingReply::Error PingReply::error() const
     return m_error;
 }
 
+bool PingReply::doHostLookup() const
+{
+    return m_doHostLookup;
+}
+
 void PingReply::abort()
 {
     m_timer->stop();

libnymea/network/pingreply.h

@@ -78,6 +78,8 @@ public:
 
     Error error() const;
 
+    bool doHostLookup() const;
+
 public slots:
     void abort();
 
@@ -95,6 +97,8 @@ private:
     QString m_hostName;
     QNetworkInterface m_networkInterface;
 
+    bool m_doHostLookup = false;
+
     uint m_retries = 0;
     uint m_retryCount = 0;
     uint m_timeout = 3;