diff --git a/libnymea-core/hardware/network/networkdevicediscoveryimpl.cpp b/libnymea-core/hardware/network/networkdevicediscoveryimpl.cpp index a5c1771f..a350667b 100644 --- a/libnymea-core/hardware/network/networkdevicediscoveryimpl.cpp +++ b/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, [=](){ - - // Search cache for mac address and update last seen - 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); - } - } - }); + // Note: we use any ping used trough this method also for the monitor evaluation + watchPingReply(reply); + return reply; +} +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 - PingReply *reply = ping(targetAddress, 1); + // Retry only once to ping a device and lookup the hostname on success + 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()); diff --git a/libnymea-core/hardware/network/networkdevicediscoveryimpl.h b/libnymea-core/hardware/network/networkdevicediscoveryimpl.h index e8dc9de8..36b082a5 100644 --- a/libnymea-core/hardware/network/networkdevicediscoveryimpl.h +++ b/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 m_runningPingRepies; QHash m_monitors; + QHash m_monitorsReferenceCount; QHash 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); diff --git a/libnymea-core/hardware/network/networkdevicemonitorimpl.cpp b/libnymea-core/hardware/network/networkdevicemonitorimpl.cpp index b40bfe62..c0191ff2 100644 --- a/libnymea-core/hardware/network/networkdevicemonitorimpl.cpp +++ b/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 diff --git a/libnymea-core/hardware/network/networkdevicemonitorimpl.h b/libnymea-core/hardware/network/networkdevicemonitorimpl.h index 1363fb80..e438d2d7 100644 --- a/libnymea-core/hardware/network/networkdevicemonitorimpl.h +++ b/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; diff --git a/libnymea/network/ping.cpp b/libnymea/network/ping.cpp index 26f227dd..b59a356a 100644 --- a/libnymea/network/ping.cpp +++ b/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); - reply->m_targetHostAddress = hostAddress; - reply->m_networkInterface = NetworkUtils::getInterfaceForHostaddress(hostAddress); + PingReply *reply = createReply(hostAddress); reply->m_retries = retries; - connect(reply, &PingReply::timeout, this, [=](){ - // Note: this is not the ICMP timeout, here we actually got nothing from nobody... - finishReply(reply, PingReply::ErrorTimeout); - }); + // Perform the reply in the next event loop to give the user time to do the reply connects + m_replyQueue.enqueue(reply); + sendNextReply(); - connect(reply, &PingReply::aborted, this, [=](){ - finishReply(reply, PingReply::ErrorAborted); - }); + return reply; +} + +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 diff --git a/libnymea/network/ping.h b/libnymea/network/ping.h index dc7b4900..42f26cea 100644 --- a/libnymea/network/ping.h +++ b/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: diff --git a/libnymea/network/pingreply.cpp b/libnymea/network/pingreply.cpp index f16db831..42ca099a 100644 --- a/libnymea/network/pingreply.cpp +++ b/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(); diff --git a/libnymea/network/pingreply.h b/libnymea/network/pingreply.h index 09fb0b65..530a55f7 100644 --- a/libnymea/network/pingreply.h +++ b/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;