Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to add support for the Device.DeviceInfo.ProductClass parameter in the WALDB data model and updates the DeviceInfo implementation to source the ProductClass value from device.properties (via fwutils).
Changes:
- Link DeviceInfo profile against
libfwutilsand include fwutils header(s). - Re-implement
get_Device_DeviceInfo_ProductClassto readDEVICE_NAMEfromdevice.properties. - Add
ProductClassto the STB and TV WALDB data model XMLs underDevice.DeviceInfo.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/hostif/profiles/DeviceInfo/Makefile.am |
Adds -lfwutils to link DeviceInfo against fwutils for device property access. |
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp |
Changes ProductClass getter to read DEVICE_NAME from device.properties using fwutils. |
src/hostif/parodusClient/waldb/data-model/data-model-tv.xml |
Exposes Device.DeviceInfo.ProductClass in the TV data model. |
src/hostif/parodusClient/waldb/data-model/data-model-stb.xml |
Exposes Device.DeviceInfo.ProductClass in the STB data model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData) | ||
| { |
There was a problem hiding this comment.
The function signature was changed to remove the bool *pChanged parameter, but the class header still declares get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t *, bool *pChanged = NULL) and there are existing call sites that pass a second argument (e.g., notification/value-changed paths). This will cause a build break and also removes change-detection behavior relied on for notifications. Please either restore the pChanged parameter (and implement change detection as before) or update the header and all callers/notification logic consistently.
| * @ingroup TR69_HOSTIF_DEVICEINFO_API | ||
| */ | ||
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData, bool *pChanged) | ||
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData) |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Parse recovery warning
declaration is incompatible with "int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t *, bool *)" (declared at line 511 of "Device_DeviceInfo.h")
Low Impact, CWE-none
RW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @see get_Device_DeviceInfo_ProductClass. | ||
| */ | ||
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *, bool *pChanged = NULL); | ||
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *); |
There was a problem hiding this comment.
The test code in gtest_main.cpp (lines 681, 699) still calls get_Device_DeviceInfo_SoftwareVersion with two parameters (including &bChanged), but the header signature was changed to only accept one parameter. This will cause test compilation failures.
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *); | |
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *, bool *pChanged = NULL); |
| * @see get_Device_DeviceInfo_ProductClass. | ||
| */ | ||
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *, bool *pChanged = NULL); | ||
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *); |
There was a problem hiding this comment.
The header file declares that get_Device_DeviceInfo_SoftwareVersion no longer has the second parameter (bool *pChanged), but the implementation in Device_DeviceInfo.cpp at line 447 still has the old signature with the bool *pChanged parameter. This mismatch will cause compilation errors. Additionally, call sites in hostIf_DeviceClient_ReqHandler.cpp (line 1146) still pass two parameters to this function. Either the implementation and all call sites need to be updated to match this header change, or this header change should be reverted.
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *); | |
| int get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *, bool *pChanged = NULL); |
| * @ingroup TR69_HOSTIF_DEVICEINFO_API | ||
| */ | ||
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData, bool *pChanged) | ||
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData) |
There was a problem hiding this comment.
The documentation comment (lines 845-846) still references the pChanged parameter which was removed from the function signature. This outdated documentation should be updated to match the new function signature.
Coverity Issue - Parse recovery warningdeclaration is incompatible with "int hostIf_DeviceInfo::get_Device_DeviceInfo_SoftwareVersion(HOSTIF_MsgData_t *)" (declared at line 578 of "Device_DeviceInfo.h") Low Impact, CWE-none Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ret; | ||
| #endif /* FETCH_PRODUCTCLASS_FROM_MFRLIB */ | ||
| fclose(fp); | ||
| strncpy((char *)stMsgData->paramValue, device_name,sizeof(stMsgData->paramValue)-1); |
There was a problem hiding this comment.
After copying device_name into stMsgData->paramValue, the code doesn’t ensure NUL-termination and doesn’t update stMsgData->paramLen. Other getters in this file set both, and downstream serialization/response formatting relies on paramLen being accurate.
| strncpy((char *)stMsgData->paramValue, device_name,sizeof(stMsgData->paramValue)-1); | |
| size_t bufSize = sizeof(stMsgData->paramValue); | |
| if (bufSize > 0) | |
| { | |
| strncpy((char *)stMsgData->paramValue, device_name, bufSize - 1); | |
| ((char *)stMsgData->paramValue)[bufSize - 1] = '\0'; | |
| stMsgData->paramLen = strlen((char *)stMsgData->paramValue); | |
| } |
| int hostIf_DeviceInfo::get_Device_DeviceInfo_ProductClass(HOSTIF_MsgData_t * stMsgData, bool *pChanged) | ||
| { | ||
| RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s()]\n", __FUNCTION__); | ||
| stMsgData->paramtype = hostIf_StringType; | ||
| RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s()]\n", __FUNCTION__); | ||
|
|
There was a problem hiding this comment.
The Doxygen comment for get_Device_DeviceInfo_ProductClass (immediately above this function) still states “Currently not implemented” and describes a -1 return, but the function is now implemented and returns OK/NOK. Please update the comment block to match the current behavior and document that the value is read from /etc/device.properties (key DEVICE_NAME).
| AM_CXXFLAGS += $(BLE_TILE_PROFILE) | ||
| endif | ||
|
|
||
| AM_LDFLAGS = -lfwutils |
There was a problem hiding this comment.
AM_LDFLAGS is being reassigned to -lfwutils, which overwrites the existing link flags declared earlier in this Makefile (and any conditionally appended flags). This will drop required libs like -lIARMBus, -lrbus, etc. If fwutils is needed, append it (or add it to the libtool library’s *_LIBADD) rather than replacing the whole variable.
| AM_LDFLAGS = -lfwutils | |
| AM_LDFLAGS += -lfwutils |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fclose(fp); | ||
| strncpy((char *)stMsgData->paramValue, device_name,sizeof(stMsgData->paramValue)-1); | ||
| return OK; |
There was a problem hiding this comment.
get_Device_DeviceInfo_ProductClass populates stMsgData->paramValue but never sets stMsgData->paramLen, which can lead to truncated/incorrect responses for callers that rely on paramLen. Also, strncpy(..., sizeof(paramValue)-1) will pad thousands of NUL bytes unnecessarily. Populate paramValue with a bounded formatting/copy that guarantees NUL-termination, then set paramLen to the resulting string length.
| while (fgets(line, sizeof(line), fp)) { | ||
| if (strncmp(line, "DEVICE_NAME=", 12) == 0) { | ||
| char *value = line + 12; | ||
| value[strcspn(value, "\r\n")] = 0; | ||
| strncpy(device_name, value, sizeof(device_name) - 1); | ||
| device_name[sizeof(device_name) - 1] = '\0'; | ||
| break; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "Failed in IARM_Bus_Call() for parameter : %s [param.type:%d with error code:%d]\n",stMsgData->paramName,param.type, ret); | ||
| ret = NOK; | ||
| } | ||
| return ret; | ||
| #endif /* FETCH_PRODUCTCLASS_FROM_MFRLIB */ | ||
| fclose(fp); | ||
| strncpy((char *)stMsgData->paramValue, device_name,sizeof(stMsgData->paramValue)-1); | ||
| return OK; |
There was a problem hiding this comment.
pChanged/backupProductClass/bCalledProductClass are part of this class’s change-notification pattern (used by other DeviceInfo getters), but this updated implementation no longer sets *pChanged or updates the backup value when ProductClass changes. To keep value-change notifications consistent, compare against backupProductClass, set *pChanged = true when it differs, and update the backup + bCalledProductClass.
No description provided.