RDK-61724: Update Network Manager to publish a new event for route change. - #328
Open
gururaajar wants to merge 16 commits into
Open
RDK-61724: Update Network Manager to publish a new event for route change.#328gururaajar wants to merge 16 commits into
gururaajar wants to merge 16 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
+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
+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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
interface/INetworkManager.h:286
INetworkManager::INotificationis a COM(-RPC) interface; addingonRouteChangein the middle shifts vtable slots and can break ABI for existing notification subscribers built against the previous header. To preserve binary compatibility, append new notification methods 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 */){};
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:103
QueryInterface()shouldAddRef()the returned interface to follow theCore::IUnknowncontract. Returning a raw pointer without incrementing the ref count can lead to mismatched lifetime management if the caller follows COM rules.
{
if ((id == Exchange::INetworkManager::INotification::ID) || (id == Core::IUnknown::ID)) {
return static_cast<Exchange::INetworkManager::INotification*>(this);
}
return nullptr;
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:273
- This
printfunconditionally indexesretrievedEndpoints[0], which is undefined behavior when the vector is empty (e.g., if the default endpoint isn't present due to earlier state).
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
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:96
Release()is expected to return a reference count (>= 1 while alive), but it currently returnsCore::ERROR_NONE(0).Unregister()callsRelease(), so returning 0 can imply the object is destroyed even though it is stack-allocated.
uint32_t Release() const override
{
return Core::ERROR_NONE;
}
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:91
AddRef()is expected to return a reference count (>= 1), but it currently returnsCore::ERROR_NONE(0). SinceRegister()callsAddRef(), returning 0 can break lifetime/diagnostics assumptions for COM-style objects.
This issue also appears in the following locations of the same file:
- line 93
- line 99
uint32_t AddRef() const override
{
return Core::ERROR_NONE;
}
Comment on lines
+1112
to
+1121
| _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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (7)
interface/INetworkManager.h:286
INetworkManager::INotificationis a COM(-RPC) interface; insertingonRouteChangehere shifts vtable slots and can break ABI for existing out-of-process subscribers built against older headers. To preserve binary compatibility, new notification methods should be appended at the end of the interface (or introduced via a new 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:1123
ReportRouteChange(const string&, const string&, const IPAddress&)logsipversionbut sendssettings.ipversionto subscribers. If these ever diverge (e.g., ifsettings.ipversionis unset), consumers will receive inconsistent data.
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
}
_notificationLock.Unlock();
plugin/NetworkManagerImplementation.cpp:274
- When the config provides no connectivity endpoints, this unconditionally sets the built-in monitor to the hardcoded default, even if
ConnectivityMonitoralready loaded cached endpoints from the cache file. This regresses the previous behavior of keeping cached endpoints when config is empty.
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");
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:273
- This test indexes
retrievedEndpoints[0]before asserting the vector is non-empty. If the implementation ever returns an empty list (e.g., regression or backend change), the test will crash instead of reporting a failed expectation.
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:1107
ReportRouteChange(const string&, const string&)callsGetIPSettings(iface, ...), which can updateifacewhen the input interface is empty (inout param), but the subsequent call uses the originalinterfaceargument. This can emit an event with an empty/wrong interface name.
if (GetIPSettings(iface, ipversion, settings) != Core::ERROR_NONE) {
return;
}
ReportRouteChange(interface, ipversion, settings);
}
plugin/NetworkManagerConnectivityClient.cpp:93
SetInternetStatusChangeHandler(nullptr)currently still callsensureOpen(), which can trigger an unnecessary connection attempt during shutdown/disable (e.g., when clearing the handler right beforereset()). Only requestOpen()when a non-null handler is set.
void NetworkManagerConnectivityClient::SetInternetStatusChangeHandler(InternetStatusChangeHandler handler)
{
{
std::lock_guard<std::mutex> lock(mLock);
mInternetStatusChangeHandler = std::move(handler);
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:377
- These tests assume
useConnectivityCheckMgrin the config line deterministically enables/disables delegation, but the implementation gives higher precedence to the RFC flagDevice.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.ConnectivityCheckMgr.Enablewhen available. If the RFC flag is set/enforced in the test environment, the tests can become nondeterministic or assert the opposite behavior.
const string configLine = R"({"loglevel":3,"useConnectivityCheckMgr":false})";
ASSERT_EQ(interface->Configure(configLine), Core::ERROR_NONE);
NetworkManagerImplementation->OnDelegatedInternetStatusChange(Exchange::INetworkManager::INTERNET_LIMITED);
EXPECT_FALSE(notification.WaitForCount(1, std::chrono::milliseconds(250)));
Comment on lines
+120
to
+136
| void NetworkManagerConnectivityClient::registerEvents() | ||
| { | ||
| if (mConnectivity == nullptr) { | ||
| return; | ||
| } | ||
| if (mNotificationRegistered) { | ||
| return; | ||
| } | ||
|
|
||
| if (auto r = mConnectivity->Register(&mNotification); r != Core::ERROR_NONE) { | ||
| NMLOG_ERROR("ConnectivityCheckMgr register(notification) failed (%u)", r); | ||
| return; | ||
| } | ||
|
|
||
| mNotificationRegistered = true; | ||
| NMLOG_INFO("registered for ConnectivityCheckMgr internet-status notifications"); | ||
| } |
Comment on lines
+138
to
+152
| void NetworkManagerConnectivityClient::unregisterEvents() | ||
| { | ||
| if (mConnectivity == nullptr) { | ||
| mNotificationRegistered = false; | ||
| return; | ||
| } | ||
| if (!mNotificationRegistered) { | ||
| return; | ||
| } | ||
|
|
||
| if (auto r = mConnectivity->Unregister(&mNotification); r != Core::ERROR_NONE) { | ||
| NMLOG_ERROR("ConnectivityCheckMgr unregister(notification) failed (%u)", r); | ||
| } | ||
| mNotificationRegistered = false; | ||
| } |
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (5)
interface/INetworkManager.h:286
- Adding
onRouteChangeintoINetworkManager::INotificationbefore existing methods changes the vtable layout for this COM(-RPC) interface, which can break ABI for existing out-of-process subscribers built against the previous header. To preserve binary compatibility, new notification methods should be appended at the end of the interface (or introduced via a new 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:1122
ReportRouteChangelogs theipversionargument but publishessettings.ipversionto subscribers. Ifsettings.ipversionis unset or differs from the requested family, clients will receive an incorrectipversion. Use theipversionparameter for the event payload (or ensuresettings.ipversionis always normalized to match).
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
}
plugin/NetworkManagerConnectivityClient.cpp:126
registerEvents()reads/writesmConnectivityandmNotificationRegisteredwithoutmLock, while other threads (e.g.,Operational(false)/ destructor) modify these undermLock. This is a C++ data race and can lead to undefined behavior or callingRegister()on a released proxy. Consider taking a temporary AddRef under the lock, then releasing the lock before calling into COM-RPC to avoid deadlocks.
void NetworkManagerConnectivityClient::registerEvents()
{
if (mConnectivity == nullptr) {
return;
}
plugin/NetworkManagerConnectivityClient.cpp:144
unregisterEvents()accessesmConnectivity/mNotificationRegisteredwithoutmLock, which races withOperational()and the destructor updating/releasing the proxy undermLock. This can lead to undefined behavior or attempting to unregister on a stale/released proxy. Use the same pattern asregisterEvents()(take AddRef under the lock, then callUnregister()without holding the lock).
void NetworkManagerConnectivityClient::unregisterEvents()
{
if (mConnectivity == nullptr) {
mNotificationRegistered = false;
return;
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:96
- This test notification implements
Core::IUnknown, butAddRef()/Release()returnCore::ERROR_NONE(0) rather than a reference count. Returning 0 can be interpreted as “object destroyed” by callers and does not match COM/IUnknown semantics. For this stack-allocated probe, return a stable non-zero value (or implement a small refcount that never reaches 0 during the test).
uint32_t AddRef() const override
{
return Core::ERROR_NONE;
}
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
interface/INetworkManager.h:286
INetworkManager::INotificationis a COM(-RPC) interface; addingonRouteChangebetween existing methods shifts vtable slots and can break ABI for out-of-process subscribers built against the previous header. To preserve binary compatibility, append new notification methods at the end of the interface (or introduce a new 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:1122
ReportRouteChangecurrently holds_notificationLockwhile invoking callbacks, which contradicts the existing pattern indispatchEvent()(snapshot callbacks + release lock) and risks deadlocks/long stalls if a subscriber calls back into NetworkManager. Also, the event passessettings.ipversionbut logs/acceptsipversionas the family; ifsettings.ipversionis empty this can violate the JSON schema/docs.
_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);
plugin/NetworkManagerConnectivityClient.cpp:154
unregisterEvents()also readsmConnectivity/mNotificationRegisteredwithoutmLock, which can race withOperational(true/false)and teardown. Using a locked snapshot (with AddRef) avoids callingUnregister()on a proxy that is being released concurrently.
void NetworkManagerConnectivityClient::unregisterEvents()
{
if (mConnectivity == nullptr) {
mNotificationRegistered = false;
return;
}
if (!mNotificationRegistered) {
return;
}
if (auto r = mConnectivity->Unregister(&mNotification); r != Core::ERROR_NONE) {
NMLOG_ERROR("ConnectivityCheckMgr unregister(notification) failed (%u)", r);
}
mNotificationRegistered = false;
}
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:273
- This test prints/indexes
retrievedEndpoints[0]unconditionally; if the vector is empty (e.g., regression, different defaults, or earlier failure), this is undefined behavior and can crash the test before the assertion runs. Guard the index or assert non-empty before accessing element 0.
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/NetworkManagerConnectivityClient.cpp:134
registerEvents()accessesmConnectivityandmNotificationRegisteredwithout synchronization, but these are mutated fromOperational()and during teardown. This can race (including use-after-free) ifOperational(false)runs concurrently. Consider protecting state withmLockand using an AddRef'd local proxy when calling into COM-RPC without holding the lock (to avoid the deadlock you noted).
void NetworkManagerConnectivityClient::registerEvents()
{
if (mConnectivity == nullptr) {
return;
}
if (mNotificationRegistered) {
return;
}
if (auto r = mConnectivity->Register(&mNotification); r != Core::ERROR_NONE) {
NMLOG_ERROR("ConnectivityCheckMgr register(notification) failed (%u)", r);
return;
}
Comment on lines
232
to
234
| /* @brief Get Internet Connectivty Status */ | ||
| virtual uint32_t IsConnectedToInternet(string &ipversion /* @inout */, string &interface /* @inout */, InternetStatus& status /* @out */) = 0; | ||
| virtual uint32_t IsConnectedToInternet(string &ipversion /* @inout */, string &interface /* @inout */, InternetStatus& status /* @out */, string& reason /* @out */) = 0; | ||
| /* @brief Get Authentication URL if the device is behind Captive Portal */ |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
interface/INetworkManager.h:234
- The COM-RPC interface method signature for
INetworkManager::IsConnectedToInternetwas changed (added thereasonout-param) without changing the interface ID. This alters the vtable/RPC contract and will break binary compatibility for any existing out-of-process clients/implementations built against the previous 3-argument signature; the non-virtual overload added below does not preserve ABI.
/* @brief Get Internet Connectivty Status */
virtual uint32_t IsConnectedToInternet(string &ipversion /* @inout */, string &interface /* @inout */, InternetStatus& status /* @out */, string& reason /* @out */) = 0;
plugin/NetworkManagerImplementation.cpp:1124
ReportRouteChange()invokes notification callbacks while holding_notificationLock. If any callback re-entersRegister/Unregister(or other code that takes_notificationLock), this can deadlock and also blocks other notifications. Other events avoid this by not holding the lock while invoking callbacks.
for (const auto callback : _notificationCallbacks) {
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
}
_notificationLock.Unlock();
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:273
- This test prints and indexes
retrievedEndpoints[0]before asserting the vector is non-empty. If the API ever returns an empty list, the test will crash (out-of-bounds) instead of failing with a useful assertion.
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/NetworkManagerConnectivityClient.cpp:23
- This file uses
std::chrono::secondsinopenThreadLoop()but does not include<chrono>directly (it currently relies on transitive includes). Adding the include makes this translation unit portable across standard library implementations.
#include "NetworkManagerConnectivityClient.h"
#include "NetworkManagerLogger.h"
#include <com/com.h>
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:112
InternetStatusNotificationProbeimplementsCore::IUnknownbutAddRef()/Release()return an error code andQueryInterface()does notAddRef()before returningthis. SinceNetworkManagerImplementation::Register()callsAddRef()andUnregister()callsRelease(), this probe should behave like a normal COM object to avoid masking lifetime issues in the test.
uint32_t AddRef() const override
{
return Core::ERROR_NONE;
}
Contributor
Contributor
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (6)
interface/INetworkManager.h:233
- Changing the pure-virtual
INetworkManager::IsConnectedToInternetsignature (adding thereasonout-param) changes the COM-RPC vtable/RPC contract while keeping the same interface ID. This can break ABI/RPC compatibility for existing out-of-process clients built against the previous 3-arg method, even though a non-virtual overload is provided for source compatibility.
/* @brief Get Internet Connectivty Status */
virtual uint32_t IsConnectedToInternet(string &ipversion /* @inout */, string &interface /* @inout */, InternetStatus& status /* @out */, string& reason /* @out */) = 0;
plugin/NetworkManagerConnectivityClient.cpp:141
registerEvents()readsmConnectivityandmNotificationRegisteredwithout synchronization, but both are mutated fromOperational()(potentially on a different thread). This is a data race and can lead to callingRegister()on a stale/null proxy or double-registering.
void NetworkManagerConnectivityClient::registerEvents()
{
if (mConnectivity == nullptr) {
return;
}
plugin/NetworkManagerConnectivityClient.cpp:159
unregisterEvents()also reads/writesmConnectivityandmNotificationRegisteredwithout synchronization, which can race withOperational(false)and lead to unregistering via an invalid proxy or leaving the registration flag in an inconsistent state.
void NetworkManagerConnectivityClient::unregisterEvents()
{
if (mConnectivity == nullptr) {
mNotificationRegistered = false;
return;
plugin/NetworkManagerImplementation.cpp:1120
ReportRouteChange()holds_notificationLockwhile invoking subscriber callbacks, unlike the queued-event path (dispatchEvent) which snapshots callbacks and releases the lock before calling into external code. Calling callbacks under_notificationLockcan deadlock if a subscriber calls back into NetworkManager (e.g., Register/Unregister) and can block other publishers.
_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:272
retrievedEndpoints[0]is used unconditionally in the debugprintf, which will read out of bounds and potentially crash the test if the vector is unexpectedly empty (e.g., a regression or a different configuration path).
std::vector<std::string> retrievedEndpoints = NetworkManagerImplementation->connectivityMonitor->getConnectivityMonitorEndpoints();
printf("Retrieved Endpoints Size: %zu %s\n", retrievedEndpoints.size(), retrievedEndpoints[0].c_str());
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:104
- This test notification object implements the COM-style
AddRef/Release/QueryInterfacecontract incorrectly:AddRef/Releasealways returnCore::ERROR_NONE(0) andQueryInterfacereturns an interface pointer without callingAddRef(). This can cause refcount-related issues (or different behavior) in code that expects COM semantics during Register/Unregister.
uint32_t AddRef() const override
{
return Core::ERROR_NONE;
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (5)
interface/INetworkManager.h:233
- Changing the existing COM(-RPC) virtual method signature of IsConnectedToInternet (adding the
reasonout-param) is an ABI/RPC breaking change: existing out-of-process consumers built against the previous 3-arg virtual will dispatch/deserialize incorrectly at runtime. The non-virtual 3-arg overload below only preserves source compatibility for rebuilt clients, not binary/RPC compatibility. Consider keeping the original 3-arg virtual and adding a new method (new name or versioned interface/ID) for the extended reason output.
virtual uint32_t IsConnectedToInternet(string &ipversion /* @inout */, string &interface /* @inout */, InternetStatus& status /* @out */, string& reason /* @out */) = 0;
plugin/NetworkManagerImplementation.cpp:1122
- ReportRouteChange() logs the
ipversionargument but sendssettings.ipversionto subscribers. If these ever diverge (e.g., settings populated without ipversion), clients could receive the wrong ipversion even though the caller passed the correct one. Use theipversionparameter consistently when notifying.
callback->onRouteChange(interface, settings.ipversion, settings.ipaddress,
settings.gateway, settings.primarydns);
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:96
- INotification is reference-counted (Unregister() calls Release()). Returning Core::ERROR_NONE (0) from Release() is not a valid refcount value and can confuse lifetime management/debugging; return a non-zero reference count.
uint32_t Release() const override
{
return Core::ERROR_NONE;
}
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:273
- This test indexes
retrievedEndpoints[0](and prints it) without first asserting the vector is non-empty. If the implementation ever returns an empty list, this becomes undefined behavior and the test may crash instead of failing cleanly.
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
tests/l2Test/rdk/l2_test_rdkproxyImpl.cpp:91
- INotification is reference-counted (Register() calls AddRef()). Returning Core::ERROR_NONE (0) from AddRef() is not a valid refcount value and can confuse lifetime management/debugging; return a non-zero reference count.
This issue also appears on line 93 of the same file.
uint32_t AddRef() const override
{
return Core::ERROR_NONE;
}
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