RDKEMW-21912: Enabling support for RFC - #529
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
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.
Suppressed comments (1)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:4031
- get_xRDKCentralComRFCLogChronoEnable() appears to return the inverted value: it sets isEnabled=true when the disable flag file exists. Since set_xRDKCentralComRFCLogChronoEnable(true) removes LOGCHRONO_DISABLE_FILE and set(false) creates it, the getter should report enabled when the file is absent (e.g., access(...) != 0).
stMsgData->paramtype = hostIf_BooleanType;
bool isEnabled = (access(LOGCHRONO_DISABLE_FILE, F_OK) == 0) ? true : false;
put_boolean(stMsgData->paramValue, isEnabled);
stMsgData->paramLen = sizeof(bool);
stMsgData->faultCode = fcNoFault;
| #include <cmath> | ||
| #include <cstring> | ||
| #include <string> | ||
| #include <fstream> | ||
| #include <cstdio> | ||
| #include <errno.h> | ||
| #include <unistd.h> |
Code Coverage Summary |
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.
Suppressed comments (2)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:47
- The newly added standard headers are duplicated later in this file (e.g.,
<string>,<fstream>, and<errno.h>already appear again around lines 57–64). This increases include noise and can slow compilation; please remove the duplicates and keep only the minimal new headers required for this change.
#include <string>
#include <fstream>
#include <cstdio>
#include <errno.h>
#include <unistd.h>
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3950
- New RFC set/get behavior was added for
Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.LogChrono.Enable, but there are no corresponding DeviceInfo gtests for it. There are already tests for similar RFC toggles (e.g.,NewNtpEnableinprofiles/DeviceInfo/gtest/gtest_main.cpp), so adding tests for LogChrono enable/disable and GET default behavior would help prevent regressions.
int hostIf_DeviceInfo::set_xRDKCentralComRFCLogChronoEnable(HOSTIF_MsgData_t *stMsgData)
{
int ret = NOK;
bool enable;
LOG_ENTRY_EXIT;
Code Coverage Summary |
|
I have read the CLA Document and I hereby sign the CLA |
Code Coverage Summary |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:47
- The newly added headers duplicate existing includes later in this file (e.g., , , <errno.h> are already included again below). This increases compile time/noise and can cause subtle ordering issues; it’s better to include each header only once.
#include <string>
#include <fstream>
#include <cstdio>
#include <errno.h>
#include <unistd.h>
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3983
- This PR introduces a new RFC setter (LogChrono.Enable) with filesystem side effects, but there’s no corresponding unit test coverage. There are existing gtests for similar flag-file RFC setters (e.g., NewNtpEnable in src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp) that could be mirrored to verify disable_logchrono creation/removal and return codes.
int hostIf_DeviceInfo::set_xRDKCentralComRFCLogChronoEnable(HOSTIF_MsgData_t *stMsgData)
{
int ret = NOK;
bool enable;
LOG_ENTRY_EXIT;
if(stMsgData->paramtype == hostIf_BooleanType)
{
enable = get_boolean(stMsgData->paramValue);
if(enable)
{
RDK_LOG(RDK_LOG_INFO,LOG_TR69HOSTIF,"[%s] set LogChrono.Enable to true\n", __FUNCTION__);
if(remove(LOGCHRONO_DISABLE_FILE) != 0 && errno != ENOENT)
{
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] Unable to clear disable logchrono flag %s\n",
__FUNCTION__, LOGCHRONO_DISABLE_FILE);
ret = NOK;
}
else
{
ret = OK;
}
}
else
{
RDK_LOG(RDK_LOG_INFO,LOG_TR69HOSTIF,"[%s] set LogChrono.Enable to false\n", __FUNCTION__);
std::ofstream disableFile(LOGCHRONO_DISABLE_FILE);
if(!disableFile.is_open())
{
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] Unable to create disable logchrono flag %s\n",
__FUNCTION__, LOGCHRONO_DISABLE_FILE);
ret = NOK;
}
else
{
disableFile.close();
ret = OK;
}
Code Coverage Summary |
There was a problem hiding this comment.
🟡 Changes recommended
The new LogChrono toggle implementation introduces security/operability issues (flag-file creation permissions and missing errno details) and some maintainability regressions that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:47
- These headers are already included later in this file (e.g., , , <errno.h>), so adding them again here introduces redundant/duplicate includes and makes the include list harder to maintain.
#include <string>
#include <fstream>
#include <cstdio>
#include <errno.h>
#include <unistd.h>
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3983
- On failure to remove the disable flag, the log drops the underlying error, which makes field debugging difficult. Please include strerror(errno) (as done elsewhere in this file, e.g. DistributedTracing handling).
if(remove(LOGCHRONO_DISABLE_FILE) != 0 && errno != ENOENT)
{
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] Unable to clear disable logchrono flag %s\n",
__FUNCTION__, LOGCHRONO_DISABLE_FILE);
ret = NOK;
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
| RDK_LOG(RDK_LOG_INFO,LOG_TR69HOSTIF,"[%s] set LogChrono.Enable to false\n", __FUNCTION__); | ||
| std::ofstream disableFile(LOGCHRONO_DISABLE_FILE); | ||
| if(!disableFile.is_open()) | ||
| { | ||
| RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] Unable to create disable logchrono flag %s\n", | ||
| __FUNCTION__, LOGCHRONO_DISABLE_FILE); | ||
| ret = NOK; | ||
| } | ||
| else | ||
| { | ||
| disableFile.close(); | ||
| ret = OK; | ||
| } |
| <syntax> | ||
| <boolean/> | ||
| <default type="factory" value="true"/> | ||
| </syntax> |
RDKEMW-19847: Migrate RDKE Logupload Triggers from System Calls to AP…
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect error handling, validation, and test coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3971
- Please add tests for this new side-effecting RFC path. The existing DeviceInfo suite covers RFC setters and invalid-type branches, but neither the true/false transitions nor file-operation failures for
disable_logchronoare exercised, so regressions in the flag state or return status can pass CI.
int hostIf_DeviceInfo::set_xRDKCentralComRFCLogChronoEnable(HOSTIF_MsgData_t *stMsgData)
{
int ret = NOK;
bool enable;
LOG_ENTRY_EXIT;
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:4003
is_open()only verifies that the stream was opened; a write/close error can still leave the disable flag unavailable. Becauseretis set toOKunconditionally afterclose(), the SET can report success even though the flag was not successfully committed. Check the stream state after closing before returning success.
disableFile.close();
ret = OK;
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3976
- This type check runs after
set_xRDKCentralComRFChas already persistedstMsgData->paramValueto the RFC store, so a non-boolean SET can write an invalid value even though this handler returnsNOK; the error branch at lines 4007-4010 also leavesfaultCodeunchanged. Validate this parameter as boolean before the generic store write and reportfcInvalidParameterTypeon rejection.
if(stMsgData->paramtype == hostIf_BooleanType)
{
enable = get_boolean(stMsgData->paramValue);
if(enable)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| else if (!strcasecmp(stMsgData->paramName, LOGCHRONO_RFC_ENABLE)) | ||
| { | ||
| ret = set_xRDKCentralComRFCLogChronoEnable(stMsgData); |
Code Coverage Summary |
No description provided.