RDK-61247: IP caching and refresh logic#310
Open
tukken-comcast wants to merge 14 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
from
90e059f to
350a197
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.
The IPAddress.ula field was incorrectly being populated with IPv6
link-local addresses (fe80::/10). ULA (Unique Local Address, fc00::/7)
is a different address class — analogous to RFC 1918 private IPv4.
Changes:
- Add isIPv6ULA() helper using the same bitmask as NetworkManager's
nm_ip6_addr_is_ula(): (s6_addr32[0] & 0xfe000000) == 0xfc000000
- Add ulaAddress field to IpFamilyCache, separate from linkLocalAddress
- Map IpFamilyCache::ulaAddress to IPAddress::ula in toIPAddress()
- Add three-way IPv6 classification (link-local / ULA / global) in
GnomeEvents, GnomeProxy, GdbusClient, GdbusEvent, and RDK proxy
- ULA addresses are NOT reported as global: they do not go into
globalAddresses and do not trigger IP_ACQUIRED/IP_LOST events
- Fix pre-existing gdbus bug: globalAddresses.insert() now uses the
correct {address, prefix} pair instead of bare string
- Fix NetworkManager.json example to use valid ULA (fd00::/8 prefix)
- Fix docs example: IPv4 result should have empty ula field
- Replace IN_IS_ADDR_LINKLOCAL macro with isIPv4LinkLocal() inline
helper for consistency with the IPv6 helpers; remove now-unnecessary
arpa/inet.h includes from GnomeProxy and GnomeEvents
Store interface HW address in IpFamilyCache.macAddress (populated from nm_device_get_hw_address). toIPAddress() iterates globalAddresses preferring non-MAC-based globals; falls back to MAC-based if all are EUI-64 derived. Direct-read (fallback) path in GnomeProxy also filters at selection time. Adds isIPv6MacBased() helper to detect EUI-64 addresses derived from the interface MAC.
… event-driven cache The event thread seeds the IP cache synchronously during startup (via refreshIpFamilyCache) before entering g_main_loop_run(), so the cache is always populated before any RPC can arrive. The fallback path that read directly from the RPC thread's m_nmClient was effectively dead code. Remove the ~250-line fallback (m_nmClient reads, connection lookups, IPv4/IPv6 address iteration, and fallback cache writes). On cache miss, return Core::ERROR_GENERAL with a warning log. Also remove the now-unused isAutoConnectEnabled() helper and its 10 local variable declarations that only served the fallback.
…header - Split IpFamilyCache into three containers: globalAddresses (map, event-diffable), linkLocalAddresses (set), uniqueLocalAddresses (set) - Move isIPv4LinkLocal, isIPv6LinkLocal, isIPv6ULA, toIPAddress from inline header definitions to NetworkManagerImplementation.cpp - Add explicit constructors to GlobalAddressInfo for C++11 compatibility - Simplify event diffing in refreshIpFamilyCache to operate on globalAddresses keys directly without type filtering
tukken-comcast
force-pushed
the
topic/RDK-61488
branch
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);
Comment on lines
+512
to
+520
| /* Subscribe to DHCP option changes so dhcpserver stays current mid-lease. */ | ||
| NMActiveConnection* connAdded = nm_device_get_active_connection(device); | ||
| if (connAdded) { | ||
| NMDhcpConfig* dhcp4Added = nm_active_connection_get_dhcp4_config(connAdded); | ||
| NMDhcpConfig* dhcp6Added = nm_active_connection_get_dhcp6_config(connAdded); | ||
| if (dhcp4Added) | ||
| g_signal_connect(dhcp4Added, "notify::options", G_CALLBACK(dhcp4OptionsCb), device); | ||
| if (dhcp6Added) | ||
| g_signal_connect(dhcp6Added, "notify::options", G_CALLBACK(dhcp6OptionsCb), device); |
- 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
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