RDK-61247: IP caching and refresh logic#310
Open
tukken-comcast wants to merge 17 commits into
Open
Conversation
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Implements per-interface/per-IP-family IP caching and refresh to serve IP settings from event-driven state (instead of repeatedly querying NetworkManager/IARM), and improves correctness around address change notifications (including better link-local detection).
Changes:
- Introduces
IpFamilyCache(IPv4/IPv6, eth/wlan) with a mutex and uses it inGetIPSettings()fast-paths. - Adds libnm-driven refresh logic in GNOME events to diff address sets and emit acquired/lost notifications.
- Normalizes IPv6 link-local detection via a shared
isIPv6LinkLocal()helper and updates logging for IP change events.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/rdk/NetworkManagerRDKProxy.cpp | Serves IP settings from new cache and populates cache from IARM results. |
| plugin/NetworkManagerImplementation.h | Adds IpFamilyCache, isIPv6LinkLocal(), and cache members + mutex. |
| plugin/NetworkManagerImplementation.cpp | Removes old per-interface address fields usage; updates IP change logging. |
| plugin/gnome/NetworkManagerGnomeProxy.cpp | Uses event-driven cache when available; updates fallback caching and IPv6 link-local detection. |
| plugin/gnome/NetworkManagerGnomeEvents.h | Declares refreshIpFamilyCache() API for cache refresh/diffing. |
| plugin/gnome/NetworkManagerGnomeEvents.cpp | Implements cache refresh from libnm state, hooks additional signals, emits acquired/lost diffs. |
| plugin/gnome/gdbus/NetworkManagerGdbusUtils.cpp | Switches IPv6 link-local detection to isIPv6LinkLocal(). |
| plugin/gnome/gdbus/NetworkManagerGdbusEvent.cpp | Switches IPv6 link-local detection to isIPv6LinkLocal(). |
| plugin/gnome/gdbus/NetworkManagerGdbusClient.cpp | Uses cache fast-path and attempts to populate cache in fallback path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
bpunnuru
reviewed
May 14, 2026
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
2 times, most recently
from
350a197 to
3f78d4f
Compare
bpunnuru
previously approved these changes
May 21, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
plugin/gnome/NetworkManagerGnomeEvents.cpp:663
- DHCP option change handlers are connected with
userData = device, but shutdown cleanup disconnects signals by&nmEventsand does not explicitly disconnect DHCP option callbacks. This leaves these connections behind across shutdown/restart. Align the userData/disconnect strategy (connect with&nmEventsor add explicit disconnects for DHCP callbacks).
/* Subscribe to DHCP option changes so dhcpserver stays current mid-lease. */
NMActiveConnection* connInit = nm_device_get_active_connection(device);
if (connInit) {
NMDhcpConfig* dhcp4Init = nm_active_connection_get_dhcp4_config(connInit);
NMDhcpConfig* dhcp6Init = nm_active_connection_get_dhcp6_config(connInit);
if (dhcp4Init)
g_signal_connect(dhcp4Init, "notify::options", G_CALLBACK(dhcp4OptionsCb), device);
if (dhcp6Init)
g_signal_connect(dhcp6Init, "notify::options", G_CALLBACK(dhcp6OptionsCb), device);
89974d5 to
d25da5b
Compare
…ation - Subscribe to notify::options on NMDhcpConfig in all three wiring sites (device-added, startup walk, ip4/ip6ConfigChangedCb) so dhcpserver stays current across mid-lease renewals; remove stale TODO comment - Replace IpFamilyCache::globalAddresses (std::set) + prefix (uint32_t) with std::map<string,uint32_t> so toIPAddress() always projects ipaddress and prefix from the same entry - Remove dead GnomeNetworkManagerEvents::onAddressChangeCb() definition and declaration, superseded by the cache-diff path in refreshIpFamilyCache
…tection Extract isIPv6LinkLocal() helper into NetworkManagerImplementation.h and use it at all four call sites. Also remove now-unused #include <set>.
Update cache insert calls in gnome and rdk proxy fallback paths to use
the map<string, uint32_t> API (insert({addr, prefix})) and remove the
now-absent top-level c->prefix field assignments.
refreshIpFamilyCache() was reading addresses from nm_active_connection_get_ip4/6_config(), which returns NULL on platforms like xione-uk where NetworkManager does not manage the IP configuration directly. The signal handlers (ip4ChangedCb, ip6ChangedCb) are connected to the device-level NMIPConfig objects, so the cache must also read from nm_device_get_ip4/6_config() to see the same addresses that triggered the notification. This mismatch caused the cache to remain empty and no IP_ACQUIRED events were ever emitted despite signals firing correctly.
nm_ip_config_get_nameservers() returns a NULL-terminated strv. In newer
libnm versions this is guaranteed non-NULL even when empty (returns a
pointer to {NULL}). The previous code accessed dnsArr[1] unconditionally
after checking dnsArr non-NULL, which reads past the single-element
allocation when there are zero DNS servers configured.
On the xione-uk platform, the device-level IP config has addresses from
the kernel but no DNS servers via NetworkManager, so the returned strv
is empty. Reading dnsArr[1] past the allocation boundary causes SIGSEGV
on this embedded platform.
Fix: only access dnsArr[1] when dnsArr[0] is confirmed non-NULL. Also
use the correct const type to avoid the C-style cast.
Three changes fix the missing IP_LOST events when disconnecting: 1. Call refreshIpFamilyCache in deviceStateChangeCb before reporting INTERFACE_LINK_DOWN or INTERFACE_REMOVED. When NM disconnects a device, it batches State and Ip4Config/Ip6Config property changes into a single D-Bus PropertiesChanged signal. libnm updates all properties atomically then emits notify signals in arbitrary order. If notify::state fires before notify::ip4-config, the cache was being cleared (by ReportInterfaceStateChange) before refreshIpFamilyCache could diff against it. By explicitly calling refreshIpFamilyCache from the state callback, the diff runs while the cache still has the old addresses, producing IP_LOST events through the canonical path with proper logging. 2. ReportInterfaceStateChange now emits IP_LOST for every cached address before clearing the cache. This is a safety net: if refreshIpFamilyCache already handled the transition (because notify::ip4-config fired first), the cache will be empty and this emits nothing. If it didn't, this catches any remaining addresses. 3. Move the nm_device_get_ip4/6_config() read outside the if(conn) gate in refreshIpFamilyCache. The device-level IP config does not require an active connection, so the cache can detect address presence or absence during teardown when the active connection is already gone.
- refreshIpFamilyCache now checks device state: when devState <= NM_DEVICE_STATE_DISCONNECTED, it skips the libnm read so newCache is empty and the diff emits IP_LOST for all cached addresses. This eliminates the signal-ordering dead zone where addresses were still present in libnm but about to be cleared. - Remove IP_LOST emission from ReportInterfaceStateChange (was duplicating the cache-clear + emit logic). That function now only manages interface state (connected flags, default interface, connectivity monitor). - Move the authoritative 'IP acquired:'/'IP lost:' log line into ReportIPAddressChange, which is the single guaranteed call site for every IP event regardless of origin. - Remove the now-redundant 'IP acquired:'/'IP lost:' prints from refreshIpFamilyCache's diff section. Verified: disconnect produces exactly one IP_LOST per cached address, no spurious IP_ACQUIRED, and IP_LOST events precede LINK_DOWN.
Replace four hardcoded cache fields (m_ethIPv4Cache, m_wlanIPv4Cache, m_ethIPv6Cache, m_wlanIPv6Cache) with a single std::map keyed by (interface, ipFamily) pair. Add getIpCache() convenience accessor. Change toIPAddress(bool isIPv6) to toIPAddress() with auto-detection via inet_pton, since the cache is already partitioned by IP family. This removes ~60 lines of repetitive if/else-if interface selection boilerplate across all backends (GnomeProxy, GdbusClient, RDKProxy) and eliminates hardcoded interface-name assumptions from the storage layer.
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
d25da5b to
c665c08
Compare
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
c665c08 to
7b7c664
Compare
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
7b7c664 to
863ffc0
Compare
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
863ffc0 to
0d4ca77
Compare
- isIPv6ULA: replace non-portable s6_addr32 with s6_addr[0] byte check
- isIPv6MacBased: rewrite with binary EUI-64 comparison via inet_pton
and memcmp, fixing broken string matching from inet_ntop zero suppression
- Add parseMac() helper accepting both colon-separated and bare hex MACs
- Add #include <cstring> and <cstdio> for memcmp/sscanf
- cleanupSignalHandlers: explicitly disconnect DHCP option callbacks
- Remove stale 'GetIPSettings fallback path' comment
- GetIPSettings: return ERROR_NONE with empty result when cache has no
entry for the requested interface+family, since absence of an address
is not an error
- GetIPSettings: reset result to IPAddress{} with correct ipversion
before cache lookup, and preserve requested family after toIPAddress()
- Remove onAddressChangeCb L2 test: the old callback was replaced by
event-driven cache diffs in refreshIpFamilyCache(); the test had no
assertions and exercised only dead compatibility code
- deviceAddedCB: seed IP cache after attaching signal handlers so
GetIPSettings returns data immediately for hotplugged devices
- Encapsulate m_ipCacheMap/m_ipCacheMutex as private; expose
lookupIpCache() and swapIpCache() locked accessors to prevent
unsynchronised access from outside the class
- lookupIpCache returns IPAddress directly (via toIPAddress() under the
lock) instead of copying the entire IpFamilyCache by value
- refreshIpFamilyCache: make this an internal helper function
- deviceRemovedCB: disconnect all IP-config, DHCP, and device-level
signal handlers symmetrically with deviceAddedCB, and clear the IP
cache (emitting IP_LOST events) to prevent stale entries and handler
accumulation on device removal/re-addition
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
0d4ca77 to
9a4aab6
Compare
…in GetIPSettings The ActiveConnection's Dhcp4Config property is not populated until the connection reaches ACTIVATED state, but ip4ChangedCb fires earlier (when addresses appear on the IP config object). At that point, nm_active_connection_get_dhcp4_config() returns NULL, causing refreshIpFamilyCache() to store an empty dhcpserver in the cache. Switch to nm_device_get_dhcp4_config() / nm_device_get_dhcp6_config() which read from the Device object's Dhcp4Config property. This property is set when the device enters IP_CONFIG state and its options are populated before the IP address appears — ensuring dhcpserver is available when the cache is built. Apply the same fix to DHCP signal handler connections in ip4ConfigChangedCb, ip6ConfigChangedCb, deviceAddedCB, networkMangerEventMonitor, deviceRemovedCB, and cleanupSignalHandlers.
Comment on lines
658
to
662
| if(interface == "eth0") | ||
| { | ||
| m_ethIPv4Address = {}; | ||
| m_ethIPv6Address = {}; | ||
| m_ethConnected.store(false); | ||
| setDefaultInterface("wlan0"); // If WiFi is connected, make it the default interface | ||
| // As default interface is changed to wlan0, switch connectivity monitor to initial check |
Comment on lines
665
to
669
| else if(interface == "wlan0") | ||
| { | ||
| m_wlanIPv4Address = {}; | ||
| m_wlanIPv6Address = {}; | ||
| m_wlanConnected.store(false); | ||
| bool triggerConnectivityCheck; | ||
| if(m_ethConnected.load()) |
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
f506f4d to
da5e06b
Compare
Comment on lines
658
to
660
| if(interface == "eth0") | ||
| { | ||
| m_ethIPv4Address = {}; | ||
| m_ethIPv6Address = {}; | ||
| m_ethConnected.store(false); |
Comment on lines
665
to
668
| else if(interface == "wlan0") | ||
| { | ||
| m_wlanIPv4Address = {}; | ||
| m_wlanIPv6Address = {}; | ||
| m_wlanConnected.store(false); | ||
| bool triggerConnectivityCheck; |
Comment on lines
206
to
210
| WPEFramework::Plugin::GnomeNetworkManagerEvents::onInterfaceStateChangeCb(Exchange::INetworkManager::INTERFACE_ACQUIRING_IP, "eth0"); | ||
| WPEFramework::Plugin::GnomeNetworkManagerEvents::onInterfaceStateChangeCb(Exchange::INetworkManager::INTERFACE_REMOVED, "eth0"); | ||
| WPEFramework::Plugin::GnomeNetworkManagerEvents::onInterfaceStateChangeCb(Exchange::INetworkManager::INTERFACE_DISABLED, "eth0"); | ||
| } | ||
|
|
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
from
da5e06b to
5fcde20
Compare
…hIpFamilyCache - Remove 5 GetIPSettings error-path tests that exercised libnm API call sequences no longer present in the cache-based implementation - Replace 5 GetIPSettings data tests with cache-based equivalents that populate IpFamilyCache via swapIpCache() and verify the JSON-RPC response - Fix 5 event tests (disconnected/unmanaged/unknown for wlan0 and eth0) by changing nm_device_get_iface and nm_device_get_state from WillOnce to WillRepeatedly to accommodate additional calls from refreshIpFamilyCache() - Fix platformInit test: GetIPSettings now returns success:true with empty data for valid interfaces when the cache is empty
Comment on lines
+1259
to
+1267
| bool NetworkManagerImplementation::lookupIpCache( | ||
| const std::string& iface, const std::string& ipFamily, | ||
| Exchange::INetworkManager::IPAddress& out) const | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_ipCacheMutex); | ||
| auto it = m_ipCacheMap.find({iface, ipFamily}); | ||
| if (it != m_ipCacheMap.end() && it->second.valid) { | ||
| out = it->second.toIPAddress(); | ||
| return true; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for change: Implement IP caching and refresh logic
Test Procedure: See ticket
Priority: P1
Risk: Medium