RDK-61724: Update Network Manager to publish a new event for route change. - #328
Open
gururaajar wants to merge 8 commits into
Open
RDK-61724: Update Network Manager to publish a new event for route change.#328gururaajar wants to merge 8 commits into
gururaajar wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new route-change notification/event to NetworkManager and introduces an opt-in build option to delegate internet-connectivity checks to the ConnectivityCheckMgr plugin instead of using NetworkManager’s built-in connectivity monitor.
Changes:
- Add a new
onRouteChangenotification path end-to-end (implementation → COM notification → JSON-RPC event) and emit it from GNOME NetworkManager cache updates / default-route-owner changes. - Add
USE_CONNECTIVITY_CHECK_MGRbuild option and a COM-RPC client (NetworkManagerConnectivityClient) to delegateIsConnectedToInternet/GetCaptivePortalURIto ConnectivityCheckMgr. - Update build wiring (CMake) to select connectivity implementation based on the new option.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/NetworkManagerJsonRpc.cpp | Emits new JSON-RPC event onRouteChange with route-related parameters. |
| plugin/NetworkManagerImplementation.h | Adds route-change reporting APIs; switches connectivity component include/member behind USE_CONNECTIVITY_CHECK_MGR. |
| plugin/NetworkManagerImplementation.cpp | Implements route-change reporting; conditionally delegates connectivity queries and disables ConnectivityMonitor paths when USE_CONNECTIVITY_CHECK_MGR is enabled. |
| plugin/NetworkManagerConnectivityClient.h | Declares COM-RPC client wrapper for Exchange::IConnectivityCheck. |
| plugin/NetworkManagerConnectivityClient.cpp | Implements delegation and status mapping to INetworkManager::InternetStatus. |
| plugin/NetworkManager.h | Plumbs the new notification callback (onRouteChange) from implementation into the plugin. |
| plugin/gnome/NetworkManagerGnomeEvents.cpp | Emits coalesced “route ready” notifications from NM cache snapshots and on active-interface changes. |
| plugin/CMakeLists.txt | Adds connectivity source selection logic and conditional compile definition/source inclusion. |
| interface/INetworkManager.h | Extends the notification interface with onRouteChange. |
| CMakeLists.txt | Adds the USE_CONNECTIVITY_CHECK_MGR CMake option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
282
to
286
| virtual void onInterfaceStateChange(const InterfaceState state /* @in */, const string interface /* @in */){}; | ||
| virtual void onActiveInterfaceChange(const string prevActiveInterface /* @in */, const string currentActiveInterface /* @in */){}; | ||
| virtual void onIPAddressChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const IPStatus status /* @in */){}; | ||
| virtual void onRouteChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const string gateway /* @in */, const string primarydns /* @in */){}; | ||
| virtual void onInternetStatusChange(const InternetStatus prevState /* @in */, const InternetStatus currState /* @in */, const string interface /* @in */){}; |
Comment on lines
+1075
to
+1086
| void NetworkManager::onRouteChange(const string interface, const string ipversion, const string ipaddress, const string gateway, const string primarydns) | ||
| { | ||
| JsonObject parameters; | ||
| parameters["interface"] = interface; | ||
| parameters["ipversion"] = ipversion; | ||
| parameters["ipaddress"] = ipaddress; | ||
| parameters["gateway"] = gateway; | ||
| parameters["primarydns"] = primarydns; | ||
|
|
||
| LOG_INPARAM(); | ||
| Notify(_T("onRouteChange"), parameters); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
interface/INetworkManager.h:286
- Adding a new method to the COM-RPC notification interface (
INetworkManager::INotification) changes the vtable layout and can break binary compatibility for any out-of-tree components built against the previous interface. If this interface is consumed across independently versioned components, consider introducing a new notification interface/ID (e.g.,INotification2) or bumping the notification interface ID/version and regenerating stubs accordingly.
virtual void onActiveInterfaceChange(const string prevActiveInterface /* @in */, const string currentActiveInterface /* @in */){};
virtual void onIPAddressChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const IPStatus status /* @in */){};
virtual void onRouteChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const string gateway /* @in */, const string primarydns /* @in */){};
virtual void onInternetStatusChange(const InternetStatus prevState /* @in */, const InternetStatus currState /* @in */, const string interface /* @in */){};
Comment on lines
+196
to
198
| std::vector<std::string> retrievedEndpoints = NetworkManagerImplementation->connectivityMonitor->getConnectivityMonitorEndpoints(); | ||
| printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str()); | ||
| EXPECT_TRUE(retrievedEndpoints.empty() != true && retrievedEndpoints[0] == "http://clients3.google.com/generate_204"); // default endpoint should remain |
Comment on lines
+223
to
227
| if (connectEndpts.size() < 1) | ||
| { | ||
| std::vector<std::string> backup; | ||
| NMLOG_INFO("Connectivity endpoints are empty in config; use the default"); | ||
| backup.push_back("http://clients3.google.com/generate_204"); |
Comment on lines
+906
to
+914
| _notificationLock.Lock(); | ||
| NMLOG_INFO("Posting onRouteChange %s %s ip=%s gw=%s dns=%s", | ||
| interface.c_str(), ipversion.c_str(), settings.ipaddress.c_str(), | ||
| settings.gateway.c_str(), settings.primarydns.c_str()); | ||
| for (const auto callback : _notificationCallbacks) { | ||
| callback->onRouteChange(interface, settings.ipversion, settings.ipaddress, | ||
| settings.gateway, settings.primarydns); | ||
| } | ||
| _notificationLock.Unlock(); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:198
retrievedEndpoints[0]is used unconditionally in the printf and expectation, which is undefined behavior if the vector is empty (and will crash the test before the assertion). Add an assertion that the vector is non-empty before indexing, or guard the printf/indexing.
std::vector<std::string> retrievedEndpoints = NetworkManagerImplementation->connectivityMonitor->getConnectivityMonitorEndpoints();
printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str());
EXPECT_TRUE(retrievedEndpoints.empty() != true && retrievedEndpoints[0] == "http://clients3.google.com/generate_204"); // default endpoint should remain
plugin/NetworkManagerImplementation.cpp:351
- When ConnectivityCheckMgr delegation is enabled, this method returns
Core::ERROR_NONEbut silently ignores the provided endpoints (because the built-in monitor is disabled). Consider returningCore::ERROR_NOT_SUPPORTEDin this mode so callers can detect that the operation is a no-op.
/* @brief Set ConnectivityTest Endpoints */
uint32_t NetworkManagerImplementation::SetConnectivityTestEndpoints(IStringIterator* const endpoints /* @in */)
{
LOG_ENTRY_FUNCTION();
std::vector<std::string> tmpEndpoints;
if(endpoints && (endpoints->Count() >= 1))
{
string endpoint{};
while(endpoints->Next(endpoint))
{
/* The url must be atleast 7 letters to be a valid `http://` url */
if(!endpoint.empty() && endpoint.size() > 7)
{
tmpEndpoints.push_back(endpoint);
}
}
if(!m_useConnectivityCheckMgr && connectivityMonitor)
connectivityMonitor->setConnectivityMonitorEndpoints(tmpEndpoints);
}
return Core::ERROR_NONE;
}
plugin/NetworkManagerImplementation.cpp:913
ReportRouteChange(..., ipversion, settings)logs theipversionparameter but sendssettings.ipversionto callbacks. Ifsettings.ipversionis unset/empty (or diverges from the requested family), subscribers may receive an incorrect/emptyipversion. Use theipversionargument consistently for the notification payload.
NMLOG_INFO("Posting onRouteChange %s %s ip=%s gw=%s dns=%s",
interface.c_str(), ipversion.c_str(), settings.ipaddress.c_str(),
settings.gateway.c_str(), settings.primarydns.c_str());
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
}
plugin/NetworkManagerJsonRpc.cpp:1086
- A new JSON-RPC event name (
onRouteChange) is emitted here, but it is not defined in the plugin API schema/docs (e.g.definition/NetworkManager.json/ generated markdown). Without updating the definition, downstream tooling and consumers relying on the schema won't discover the event or its parameters.
void NetworkManager::onRouteChange(const string interface, const string ipversion, const string ipaddress, const string gateway, const string primarydns)
{
JsonObject parameters;
parameters["interface"] = interface;
parameters["ipversion"] = ipversion;
parameters["ipaddress"] = ipaddress;
parameters["gateway"] = gateway;
parameters["primarydns"] = primarydns;
LOG_INPARAM();
Notify(_T("onRouteChange"), parameters);
}
Comment on lines
316
to
322
| uint32_t NetworkManagerImplementation::GetConnectivityTestEndpoints(IStringIterator*& endpoints/* @out */) const | ||
| { | ||
| LOG_ENTRY_FUNCTION(); | ||
| std::vector<std::string> tmpEndpoints = connectivityMonitor.getConnectivityMonitorEndpoints(); | ||
| std::vector<std::string> tmpEndpoints; | ||
| if(!m_useConnectivityCheckMgr && connectivityMonitor) | ||
| tmpEndpoints = connectivityMonitor->getConnectivityMonitorEndpoints(); | ||
| endpoints = (Core::Service<RPC::StringIterator>::Create<RPC::IStringIterator>(tmpEndpoints)); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
plugin/NetworkManagerImplementation.cpp:252
- Current endpoint configuration logic overwrites any endpoints loaded from the connectivity cache file when the config provides no endpoints. Since EndpointManager loads cached endpoints in its constructor, this branch will reset them to the hardcoded default even when a user previously configured custom endpoints. Preserve cached endpoints by only applying the config/default when the monitor currently has no endpoints.
if (connectEndpts.size() < 1)
{
std::vector<std::string> backup;
NMLOG_INFO("Connectivity endpoints are empty in config; use the default");
backup.push_back("http://clients3.google.com/generate_204");
if(!m_useConnectivityCheckMgr && connectivityMonitor)
connectivityMonitor->setConnectivityMonitorEndpoints(backup);
}
else if (!m_useConnectivityCheckMgr && connectivityMonitor && connectivityMonitor->getConnectivityMonitorEndpoints().size() < 1)
{
NMLOG_INFO("Use the connectivity endpoint from config");
connectivityMonitor->setConnectivityMonitorEndpoints(connectEndpts);
}
plugin/NetworkManagerImplementation.cpp:1082
- ReportRouteChange(interface, ipversion, settings) notifies callbacks while holding _notificationLock, which is the opposite of dispatchEvent()'s documented approach (snapshot callbacks and release the lock before invoking). This can deadlock if callbacks are slow or re-enter NetworkManager. It also forwards
settings.ipversioninstead of theipversionargument, which can produce inconsistent event data (log usesipversion, callbacks getsettings.ipversion).
_notificationLock.Lock();
NMLOG_INFO("Posting onRouteChange %s %s ip=%s gw=%s dns=%s",
interface.c_str(), ipversion.c_str(), settings.ipaddress.c_str(),
settings.gateway.c_str(), settings.primarydns.c_str());
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
plugin/NetworkManagerJsonRpc.cpp:1086
- The plugin emits a new JSON-RPC event name
onRouteChange, but the public JSON-RPC schema/spec (definition/NetworkManager.json) does not define this event. Without updating the schema, generated client bindings and documentation will not expose/subscription-filter this event consistently.
void NetworkManager::onRouteChange(const string interface, const string ipversion, const string ipaddress, const string gateway, const string primarydns)
{
JsonObject parameters;
parameters["interface"] = interface;
parameters["ipversion"] = ipversion;
parameters["ipaddress"] = ipaddress;
parameters["gateway"] = gateway;
parameters["primarydns"] = primarydns;
LOG_INPARAM();
Notify(_T("onRouteChange"), parameters);
}
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:198
- This test prints and asserts using
retrievedEndpoints[0]even though the vector may be empty, which can crash the test process and hide the real failure reason. Prefer asserting non-empty before indexing.
std::vector<std::string> retrievedEndpoints = NetworkManagerImplementation->connectivityMonitor->getConnectivityMonitorEndpoints();
printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str());
EXPECT_TRUE(retrievedEndpoints.empty() != true && retrievedEndpoints[0] == "http://clients3.google.com/generate_204"); // default endpoint should remain
plugin/NetworkManagerImplementation.cpp:1069
- ReportRouteChange(const string&, const string&) intentionally copies
interfaceintoifacebecause GetIPSettings may fill in the default interface when the input is empty (it takesstring&). But the subsequent call forwards the originalinterface, so an empty input results in notifications with an empty interface name.
void NetworkManagerImplementation::ReportRouteChange(const string& interface, const string& ipversion)
{
string iface = interface;
Exchange::INetworkManager::IPAddress settings{};
if (GetIPSettings(iface, ipversion, settings) != Core::ERROR_NONE) {
return;
}
ReportRouteChange(interface, ipversion, settings);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
plugin/NetworkManagerImplementation.cpp:1091
- ReportRouteChange holds _notificationLock while invoking subscriber callbacks and also passes settings.ipversion instead of the ipversion argument. This can deadlock (callbacks may call back into NetworkManager) and can emit an empty/wrong ipversion when settings.ipversion is unset. Take a snapshot of callbacks (AddRef/Release) like dispatchEvent() does, and pass the ipversion parameter through consistently.
_notificationLock.Lock();
NMLOG_INFO("Posting onRouteChange %s %s ip=%s gw=%s dns=%s",
interface.c_str(), ipversion.c_str(), settings.ipaddress.c_str(),
settings.gateway.c_str(), settings.primarydns.c_str());
for (const auto callback : _notificationCallbacks) {
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:197
- This printf unconditionally indexes retrievedEndpoints[0]; if the vector is empty (e.g., a future change stops pre-populating a default endpoint), the test will crash with out-of-bounds access. Make the log conditional or avoid indexing when empty.
printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str());
interface/INetworkManager.h:286
- Adding a new virtual method to the existing COM-RPC notification interface (INetworkManager::INotification) changes the vtable layout and can break binary compatibility for any out-of-tree subscribers built against the old header. If ABI compatibility matters, consider introducing a new notification interface ID (e.g., INotification2) and only calling onRouteChange when the subscriber supports it via QueryInterface.
virtual void onInterfaceStateChange(const InterfaceState state /* @in */, const string interface /* @in */){};
virtual void onActiveInterfaceChange(const string prevActiveInterface /* @in */, const string currentActiveInterface /* @in */){};
virtual void onIPAddressChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const IPStatus status /* @in */){};
virtual void onRouteChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const string gateway /* @in */, const string primarydns /* @in */){};
virtual void onInternetStatusChange(const InternetStatus prevState /* @in */, const InternetStatus currState /* @in */, const string interface /* @in */){};
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:161
- The test dereferences NetworkManagerImplementation->connectivityMonitor without checking for null. With the new runtime backend selection, connectivityMonitor can be reset when ConnectivityCheckMgr delegation is enabled, which would make this test crash rather than fail cleanly.
NetworkManagerImplementation->connectivityMonitor->setConnectivityMonitorEndpoints(mockEndpoints);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
interface/INetworkManager.h:286
INetworkManager::INotificationis a COM(-RPC) interface; addingonRouteChange()in the middle shifts vtable slots for the methods that follow (e.g.,onInternetStatusChange, WiFi notifications), which can break ABI for existing out-of-process subscribers built against the previous header. To preserve binary compatibility, append new virtuals at the end of the interface (or introduce a versioned notification interface/ID).
virtual void onInterfaceStateChange(const InterfaceState state /* @in */, const string interface /* @in */){};
virtual void onActiveInterfaceChange(const string prevActiveInterface /* @in */, const string currentActiveInterface /* @in */){};
virtual void onIPAddressChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const IPStatus status /* @in */){};
virtual void onRouteChange(const string interface /* @in */, const string ipversion /* @in */, const string ipaddress /* @in */, const string gateway /* @in */, const string primarydns /* @in */){};
virtual void onInternetStatusChange(const InternetStatus prevState /* @in */, const InternetStatus currState /* @in */, const string interface /* @in */){};
plugin/NetworkManagerImplementation.cpp:254
- When config connectivity endpoints are empty, this code unconditionally sets the default endpoint, which overwrites any endpoints that were loaded from the cache file (
/tmp/nm.plugin.endpoints) byEndpointManager. This regresses the intended persistence behavior (cached endpoints should take precedence when present).
if (connectEndpts.size() < 1)
{
std::vector<std::string> backup;
NMLOG_INFO("Connectivity endpoints are empty in config; use the default");
backup.push_back("http://clients3.google.com/generate_204");
if(!m_useConnectivityCheckMgr && connectivityMonitor)
connectivityMonitor->setConnectivityMonitorEndpoints(backup);
}
else if (!m_useConnectivityCheckMgr && connectivityMonitor && connectivityMonitor->getConnectivityMonitorEndpoints().size() < 1)
{
NMLOG_INFO("Use the connectivity endpoint from config");
connectivityMonitor->setConnectivityMonitorEndpoints(connectEndpts);
}
plugin/NetworkManagerImplementation.cpp:1094
ReportRouteChange()logs theipversionargument but notifies callbacks usingsettings.ipversion. Ifsettings.ipversionis unset or diverges from the argument (e.g., when populated viaGetIPSettings()), subscribers can receive the wrong/emptyipversionvalue.
NMLOG_INFO("Posting onRouteChange %s %s ip=%s gw=%s dns=%s",
interface.c_str(), ipversion.c_str(), settings.ipaddress.c_str(),
settings.gateway.c_str(), settings.primarydns.c_str());
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
}
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:198
- This test unconditionally indexes
retrievedEndpoints[0]in theprintf, which will crash if the vector is empty. Since the purpose is to verify the default endpoint remains, guard the access (or assert non-empty first).
std::vector<std::string> retrievedEndpoints = NetworkManagerImplementation->connectivityMonitor->getConnectivityMonitorEndpoints();
printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str());
EXPECT_TRUE(retrievedEndpoints.empty() != true && retrievedEndpoints[0] == "http://clients3.google.com/generate_204"); // default endpoint should remain
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: Added new onroutechange event and added macro for enabling connectivitycheckmgr plugin instead of using networkmanager connectivity