RDKEMW-19653 : Deprecated DataModel Removal for XRE and XCaliber code from RDKE - #520
RDKEMW-19653 : Deprecated DataModel Removal for XRE and XCaliber code from RDKE#520madhubabutt wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes deprecated XRE/Xcalibur data model and build/config wiring from the tr69hostif codebase, aligning RDKE builds and runtime configuration away from the legacy XRE/Xcalibur profile.
Changes:
- Removes XRE/Xcalibur-specific build conditionals and linkage (
XRELIB_FLAG,--enable-xre,-ltr69ProfileXcaliber). - Drops Xcalibur/XRE manager mappings from runtime and integration-test configuration.
- Prunes XRE/Xcalibur-related WALDB data model objects from the TV and generic data model XMLs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Makefile.am |
Removes XRELIB_FLAG-based linking and Xcaliber profile library linkage. |
configure.ac |
Removes --enable-xre option, conditional, and XRE substitution variables. |
conf/tr69hostIf.conf |
Removes XRE/Xcalibur object-to-manager mappings. |
conf/mgrlist.conf |
Removes Xcalibur manager entry from the manager list. |
src/integrationtest/conf/mgrlist.conf |
Mirrors mgrlist removal for integration-test configuration. |
src/hostif/parodusClient/waldb/data-model/data-model-tv.xml |
Removes Xcalibur CoPilot object definitions from the TV WALDB model. |
src/hostif/parodusClient/waldb/data-model/data-model-generic.xml |
Removes XRPairing RFC object and large Xcalibur/XRE/TRM sections from the generic WALDB model. |
Code Coverage Summary |
3a7de3d to
d90824e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:133
- The XRE client handler is documented as “xconfCheckNow only”, but handleSetMsg still accepts additional Xcalibur parameters under RDKV_TR69 (xreReceiverRestart/devAppRestartRequest). Since the XRE/Xcalibur data model and manager mappings are being removed, these extra branches are now inconsistent and can leave partially-supported/undocumented behavior. Consider removing the RDKV_TR69-only branches so this handler truly only supports xconfCheckNow.
#ifdef RDKV_TR69
else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
{
ret = setXreReceiverRestart(stMsgData);
}
src/hostif/profiles/DeviceInfo/Makefile.am:27
- This Makefile now has the same MoCA include path listed twice, which is redundant and makes the include list harder to maintain. Remove the duplicate entry.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:214
- The xconfCheckNow input validation accepts prefix matches because it uses strncasecmp(..., strlen("TRUE")) / strlen("CANARY") (e.g., "TRUE123" passes). If the intent is to only accept the exact tokens TRUE or CANARY (case-insensitive), use strcasecmp for full-string comparison.
/**
* @brief Set xconfCheckNow parameter to trigger firmware update check.
* @param[in] stMsgData Message data containing the parameter value.
* @return OK on success, NOK on failure.
*/
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
Code Coverage Summary |
d90824e to
3678902
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:140
handleSetMsg()still referencessetXreReceiverRestart()andsetDevAppRestartRequest()under#ifdef RDKV_TR69, but those functions are not defined anywhere after the XRE/Xcalibur removal. This will fail to compile whenRDKV_TR69is enabled. Since the PR scope says onlyxconfCheckNowremains, drop these legacy branches (or reintroduce their implementations).
#ifdef RDKV_TR69
else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
{
ret = setXreReceiverRestart(stMsgData);
}
else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.DevApp.devAppRestartRequest") == 0)
{
// xreReceiverRestart uses /lib/rdk/restartReceiver.sh, which sometimes doesn't start receiver back
ret = setDevAppRestartRequest(stMsgData);
}
#endif
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:259
get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()reads at most 9 characters and doesn't populateparamLen/paramtype, which can lead to truncated values and inconsistent TR-069 responses. Prefer reading up toTR69HOSTIFMGR_MAX_PARAM_LEN-1, stripping newline, and settingparamLen+paramtype.
fscanf(file, "%9s", stMsgData->paramValue);
fclose(file);
return OK;
src/hostif/profiles/DeviceInfo/Makefile.am:28
AM_CXXFLAGScontains the same MoCA include path twice, which is redundant and makes future maintenance noisier.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
$(GLIB_CFLAGS) $(G_THREAD_CFLAGS) $(MOCAMGR_FLAGS) $(PROCPS_CFLAGS) $(YOCTO_FLAG) \
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:159
- Doc comment has a double period in "device..", which looks accidental and will show up in generated documentation.
* @return Returns the status of the operation.
* @retval OK if it is successfully.
* @retval ERR_INTERNAL_ERROR if not able to get data from the device..
* @ingroup TR-069HOSTIF_XRECLIENT_REQHANDLER_CLASSES
Code Coverage Summary |
3678902 to
bbef606
Compare
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:133
- The file header/note says only
Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNowremains, buthandleSetMsg()still accepts other XRE/DevApp control parameters under#ifdef RDKV_TR69(e.g.,...Client.XRE.xreReceiverRestart,...DevApp.devAppRestartRequest). If those parameters are deprecated/removed from the data model, this branch should be removed so the handler behavior matches the documented scope.
#ifdef RDKV_TR69
else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
{
ret = setXreReceiverRestart(stMsgData);
}
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:255
get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()returnsOKwithout settingparamtype/paramLen, and doesn't clearparamValuebefore reading. Callers can observe stale buffer contents or a zero length even on success; sethostIf_StringTypeandparamLenwhen returning a value.
if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
fclose(file);
return NOK;
}
src/hostif/profiles/DeviceInfo/Makefile.am:28
src/hostif/profiles/DeviceInfo/Makefile.amnow has the MoCA include path listed twice consecutively, which is redundant and makes the include list harder to maintain.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
$(GLIB_CFLAGS) $(G_THREAD_CFLAGS) $(MOCAMGR_FLAGS) $(PROCPS_CFLAGS) $(YOCTO_FLAG) \
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:211
set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()currently usesstrncasecmp(..., strlen("TRUE"))/strlen("CANARY"), which accepts prefix values likeTRUEjunk. It also writes the raw value to/tmp/xconfchecknow_valeven when the input is invalid (write happens before validation). Validate an exact value first and only then persist a canonical value.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
bbef606 to
5f28316
Compare
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:215
- set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() writes the requested value to /tmp before validating it, so invalid inputs still get persisted. It also matches only a prefix (e.g., "TRUE123" passes) and does not set a faultCode for invalid values. Validate first (exact match), set fcInvalidParameterValue on invalid input, then persist the value only on success.
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
}
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:255
- get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() populates paramValue but never sets paramLen. Many other handlers set paramLen after filling paramValue, and callers may rely on it for transport/serialization.
if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
fclose(file);
return NOK;
}
src/hostif/profiles/DeviceInfo/Makefile.am:27
- Duplicate include path: DeviceInfo/Makefile.am adds the MoCA include directory twice consecutively. This is redundant and makes flag lists harder to maintain.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
docs/api/dml_parameter_list.md:9
- The access-count summary table is internally inconsistent: 306 readOnly + 543 readWrite = 849, but the Total row says 848.
| readOnly | 306 | GET only - assert non-empty or expected value |
| readWrite | 543 | GET + SET - verify set/get round-trip |
| **Total** | **848** | |
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:23
- The file header says only xconfCheckNow remains, but handleSetMsg() still supports additional legacy parameters under RDKV_TR69 (xreReceiverRestart/devAppRestartRequest). Either remove those code paths or update this note so the documented scope matches the actual supported parameters.
This issue also appears in the following locations of the same file:
- line 211
- line 251
* @file hostIf_XREClient_ReqHandler.cpp
* @brief Minimal handler for Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow parameter.
* @note All XRE/Xcalibur profile features except xconfCheckNow have been removed.
src/hostif/handlers/include/hostIf_XREClient_ReqHandler.h:23
- The header note claims only xconfCheckNow is supported, but the implementation still has additional legacy SET parameters under RDKV_TR69. Update the note (or remove the legacy paths) so API docs match behavior.
* @file hostIf_XREClient_ReqHandler.h
* @brief Minimal handler for Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow parameter.
* @note All XRE/Xcalibur profile features except xconfCheckNow have been removed.
*/
5f28316 to
4f2b8db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:204
set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()persists the raw input to/tmp/xconfchecknow_valbefore validating it, and the currentstrncasecmp(..., strlen("TRUE"))/strncasecmp(..., strlen("CANARY"))checks accept prefixes (e.g. "TRUE123") even though the error message/documentation says only "TRUE" or "CANARY" are valid. This can leave an invalid value persisted even when returningNOK, and it can trigger the updater for unintended strings.
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
}
src/Makefile.am:58
- WiFi profile support is being removed from the build here (the
WITH_WIFI_PROFILEblock that definedUSE_WIFI_PROFILEand added the WiFi include path was deleted), butconfigure.acstill exposes--enable-wifi/WITH_WIFI_PROFILEand the default runtime config still mapsDevice.WiFitowifiMgr(e.g.conf/tr69hostIf.conf:10,conf/mgrlist.conf:9). As-is,--enable-wifibecomes misleading (WiFi libs may build, but the main binary no longer compiles/links WiFi handler support) and the shipped configs reference a manager that will never be registered.
if WITH_DHCP_PROFILE
AM_CXXFLAGS += -DUSE_DHCPv4_PROFILE
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/profiles/DHCPv4
endif
docs/integration/build-setup.md:33
- The build documentation still states that
--enable-wifienables WiFi profile support, but this PR removes WiFi handler/profile wiring from the build (e.g.WITH_WIFI_PROFILEblocks removed fromsrc/Makefile.amandsrc/hostif/handlers/Makefile.am). This flag description should be updated to match the new behavior (removed/unsupported, or clarify what it still builds).
The top-level `configure.ac` currently exposes feature toggles including:
| Flag | Effect |
|------|--------|
| `--enable-moca` / `--enable-moca2` | Enable MoCA profile support |
| `--enable-DHCPv4` | Enable DHCPv4 profile support |
| `--enable-StorageService` | Enable StorageService profile support |
| `--enable-InterfaceStack` | Enable InterfaceStack profile support |
| `--enable-notification` | Enable value-change notification support |
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3766
set_xRDKCentralComRFC()no longer handlesDevice.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.LXC.XRE.Enable(XRE container RFC), but the DeviceInfo gtest suite still asserts this parameter returnsOK(e.g.DeviceInfo/gtest/gtest_main.cpp:3608) andrun_ut.shstill creates/opt/XRE_container_enable. This will cause the existing unit test to fail (and leaves stale test scaffolding).
else if (strcasecmp(stMsgData->paramName,LE_RFC_ENABLE) == 0)
{
ret = set_xRDKCentralComRFCLoudnessEquivalenceEnable(stMsgData);
}
else if (strcasecmp(stMsgData->paramName,DAB_RFC_ENABLE) == 0)
{
ret = set_xRDKCentralComDABRFCEnable(stMsgData);
}
else if (strcasecmp(stMsgData->paramName,RFC_CTL_RETRIEVE_NOW) == 0)
{
ret = set_xRDKCentralComRFCRetrieveNow(stMsgData);
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:246
xconfCheckNowset/get behavior is now part of the always-built XREClient handler, andget_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()has new error-handling (fscanf return check). There are gtest suites in the repo, but no tests currently exerciseDevice.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow(set/get, invalid input, missing file, read failure), so regressions here will be untested.
int get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
FILE *file = fopen("/tmp/xconfchecknow_val", "r");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for read.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
}
if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
fclose(file);
return NOK;
}
fclose(file);
return OK;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/Makefile.am:55
- WITH_WIFI_PROFILE support appears to have been removed from the build flags, but the project still has a WiFi profile (configure.ac defines WIFI_PROFILE_FLAG and conf files reference Device.WiFi=wifiMgr). As-is, enabling WiFi via configure will no longer define USE_WIFI_PROFILE or add the WiFi include path, so WiFi manager code won’t compile/route correctly.
This issue also appears on line 138 of the same file.
if WITH_DHCP_PROFILE
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:204
- set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() writes the provided value to /tmp/xconfchecknow_val before validating it. If the input is invalid, the function returns NOK but leaves the persisted state set to the invalid value, which can break subsequent GETs and observability.
This issue also appears on line 242 of the same file.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
src/Makefile.am:140
- WITH_WIFI_PROFILE is no longer linked into the tr69hostif binary. Since the WiFi profile library still exists (profiles/wifi/libhostIfWiFi.la) and the default configs still map Device.WiFi to wifiMgr, the resulting binary will miss WiFi support even when configured.
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/parodusClient/pal
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/parodusClient/waldb
tr69hostif_LDADD += $(top_builddir)/src/hostif/parodusClient/waldb/libwaldb.la
src/hostif/handlers/Makefile.am:39
- WIFI profile include/define flags were removed from handler compilation. When WiFi is enabled, handler sources that include hostIf_WiFi_ReqHandler.h rely on USE_WIFI_PROFILE and the wifi include path; without these, WiFi handler support cannot be built even though configure.ac still exposes WITH_WIFI_PROFILE/WIFI_PROFILE_FLAG.
-I./include $(GLIB_CFLAGS) $(G_THREAD_CFLAGS) \
$(MOCAMGR_FLAGS) $(SOUP_CFLAGS) $(MOCA2_FLAG) \
$(XRDK_SDCARD_PROFILE_FLAG) $(XRDK_EMMC_PROFILE_FLAG) \
src/hostif/handlers/Makefile.am:114
- WiFi handler source is no longer added to libMsgHandlers when WITH_WIFI_PROFILE is enabled, so wifiMgr routing (Device.WiFi) will fail at runtime and/or link-time if other code expects WiFiReqHandler symbols.
if WITH_MOCA_PROFILE
libMsgHandlers_la_SOURCES += src/hostIf_MoCAClient_ReqHandler.cpp
endif
if WITH_DHCP_PROFILE
libMsgHandlers_la_SOURCES += src/hostIf_DHCPv4Client_ReqHandler.cpp
endif
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:248
- get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK without populating paramtype/paramLen. Other handlers consistently set these fields, and leaving them stale can cause incorrect serialization/processing of the response.
if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
fclose(file);
return NOK;
}
fclose(file);
return OK;
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/hostif/profiles/DeviceInfo/Makefile.am:27
- The same MoCA include directory is listed twice in AM_CXXFLAGS, which is redundant and makes future edits noisier.
AM_CXXFLAGS = -I$(top_srcdir)/src/hostif/include \
-I$(top_srcdir)/src/hostif/handlers/include \
-I$(top_srcdir)/src/hostif/handlers/src \
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:202
- set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() currently opens/writes /tmp/xconfchecknow_val before validating the input and uses prefix matching via strncasecmp+strlen (so values like "TRUEFOO" would be accepted). This can persist invalid/failed SET values and trigger the upgrader unexpectedly. Consider (1) validating exact allowed values, (2) running the script, and only then (3) persisting the value on success.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:246
- get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK/NOK but does not populate stMsgData->paramtype or stMsgData->paramLen, which other handlers rely on for correct serialization/logging. Also, fscanf("%9s") truncates the stored value and stops at whitespace; using fgets and trimming newline is safer and preserves the full stored string.
if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
fclose(file);
return NOK;
}
4c4a596 to
5ddc18e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:206
set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()persistsstMsgData->paramValueto/tmp/xconfchecknow_valbefore validating that the input is one of the supported tokens (TRUE/CANARY). If an invalid value is set, the function returnsNOKbut the invalid value still gets written, which can cause later GETs to return an unsupported value and makes the failure non-atomic. Validate first, then write the file only for valid inputs.
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
}
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:227
- The xconfCheckNow setter accepts prefix matches (e.g., "TRUE123" passes because it uses strncasecmp with strlen("TRUE")). Also, the value is persisted to /tmp/xconfchecknow_val before validation, so invalid inputs (or script execution failures) can leave an incorrect persisted value even though the SET returns NOK.
return OK;
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:248
- After successfully reading xconfCheckNow, the handler returns OK without updating stMsgData->paramtype and stMsgData->paramLen. Other GET handlers populate these fields, and leaving them stale can cause incorrect serialization of the response.
fclose(file);
return OK;
Code Coverage Summary |
| </syntax> | ||
| </parameter> | ||
| </object> | ||
| <object base="Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.XRPairing." access="readOnly" minEntries="0" maxEntries="1" > |
There was a problem hiding this comment.
Is Xfiniity Remote pairing actually need to be removed ?
0e5aef1 to
36bf4ba
Compare
36bf4ba to
4cf71f4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:206
set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()writes the requested value to/tmp/xconfchecknow_valbefore validating it, so invalid inputs (or a later script failure) still persist and can affect subsequent GETs. Alsov_secure_system()is only checked for-1, which misses non-zero exit statuses.
FILE *file = fopen("/tmp/xconfchecknow_val", "w");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
}
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:88
getLock()callsg_mutex_init()every time it is invoked. Re-initializing aGMutexafter it has been used is undefined behavior and can lead to races/crashes under concurrent access.
void XREClientReqHandler::reset()
{
getLock();
RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Entering..\n", __FUNCTION__, __FILE__);
RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Exiting..\n", __FUNCTION__, __FILE__);
src/Makefile.am:58
- This PR removes the
WITH_WIFI_PROFILEwiring from the top-level build (no-DUSE_WIFI_PROFILEdefine, no WiFi include path, and no link oflibhostIfWiFi.la), butconfigure.acstill exposes--enable-wifiandsrc/hostif/profiles/Makefile.amstill builds thewifi/subdir underWITH_WIFI_PROFILE. As-is,--enable-wifibecomes effectively a no-op and may mislead integrators.
if WITH_DHCP_PROFILE
AM_CXXFLAGS += -DUSE_DHCPv4_PROFILE
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/profiles/DHCPv4
endif
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:240
get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow()returns OK without populatingparamtype/paramLen, which other handlers rely on for correct responses. Usingfscanf("%9s")also unnecessarily truncates the stored value (buffer is 4KB).
int get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
FILE *file = fopen("/tmp/xconfchecknow_val", "r");
if (file == NULL) {
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for read.\n",__FILE__,__FUNCTION__,__LINE__);
return NOK;
docs/api/dml_parameter_list.md:9
- The parameter-list documentation now drops from 904 → 747 total parameters, which includes removing the entire
Device.WiFi.*section in addition to XRE/Xcalibur removals. Since the PR description only calls out XRE/Xcalibur deprecation removal, this doc update suggests broader DataModel scope than described.
| readOnly | 254 | GET only - assert non-empty or expected value |
| readWrite | 493 | GET + SET - verify set/get round-trip |
| **Total** | **747** | |
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:134
- In handleSetMsg(), the unsupported-parameter branch sets faultCode=fcInvalidParameterName but returns NOT_HANDLED. For a manager that has already been selected based on the prefix, returning NOT_HANDLED can cause the request to be treated as unhandled rather than an invalid parameter. This should return NOK (consistent with other handlers) when the parameter name is invalid.
else
{
stMsgData->faultCode = fcInvalidParameterName;
ret = NOT_HANDLED;
}
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:248
- get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK after reading the value, but it never sets stMsgData->paramLen (and leaves paramtype unchanged). Other handlers populate paramLen for string results; leaving it unset can produce incomplete/incorrect responses for consumers that rely on length.
fclose(file);
return OK;
src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:128
- The PR’s main requirement is to preserve Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow support, but there are no unit tests covering the XREClientReqHandler get/set flow (including invalid input handling). Adding a focused gtest would help prevent regressions now that the wider XRE/Xcalibur surface has been removed.
int XREClientReqHandler::handleSetMsg(HOSTIF_MsgData_t *stMsgData)
{
int ret = NOT_HANDLED;
stMsgData->instanceNum = 0;
RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s:%d] Found string as %s\n", __FUNCTION__, __FILE__, __LINE__, stMsgData->paramName);
getLock();
if (strcasecmp (stMsgData->paramName, "Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow") == 0)
{
ret = set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow (stMsgData);
Reason for change: Removal of deprecated XRE/Xcalibur profile infrastructure (USE_XRESRC, XRELIB_FLAG, and all Xcalibur parameters except xconfCheckNow) while preserving minimal xconfCheckNow parameter support.
Test Procedure: Build RDKE with the change, validate Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow functionality, perform stability/reboot tests ensuring no crashes.
Risks: Medium
Priority: P1
Signed-off-by: Tirumala, Madhubabu (Contractor) Madhubabu_Tirumala@comcast.com