Conversation
…ic Updates (#117) * Update rrdInterface.c * Update rrdInterface.c * Update rrdInterface.c * Update cov_build.sh * Update configure.ac * Update rrdInterface.c * Update rrdInterface.h * Update cov_build.sh * Update rrdInterface.c * Update rrdInterface.h * Update rrdInterface.c --------- Co-authored-by: nhanasi <navihansi@gmail.com>
…method (#123) * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdCommon.h * Update rrdIarmEvents.c * Update rrdInterface.h * Update rrdCommon.h * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdDynamic.c * Update rrdJsonParser.c * Update rrdCommon.h * Update rrdIarmEvents.c * Update rrdInterface.h * Update rrdInterface.h * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdJsonParser.c * Update rrdEventProcess.c * Update rrdJsonParser.c * Update rrdJsonParser.c * Update rrdJsonParser.c * Update rrdEventProcess.c * Update rrdCommon.h * Update rrdIarmEvents.c * Update rrdIarmEvents.c * Update rrdIarmEvents.c
…method (#125) * Update rrdIarmEvents.c * Update rrdInterface.c * Update rrdInterface.c * Update cov_build.sh * Update rrdIarmEvents.c --------- Co-authored-by: nhanasi <navihansi@gmail.com>
…d message type (SCXI11BEI) (#121) * Update rrdJsonParser.c * Update rrdJsonParser.c * Update rrdEventProcess.c * Update rrdInterface.c * Update rrdInterface.c * Update rrdInterface.c * Update rrdJsonParser.c * Update rrdInterface.c * Update rrdJsonParser.c * Update rrdEventProcess.c * Update rrdJsonParser.c * Update rrdEventProcess.c * Update rrdEventProcess.c * Update rrdInterface.c * Update rrdInterface.c --------- Co-authored-by: nhanasi <navihansi@gmail.com>
Added release to fix remotedebugger bug fixes
… process exited with error code." (#134) * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c * Update rrdRunCmdThread.c
…rogress (#136) * Update rrdInterface.c * Update rrdInterface.c * Update rrdInterface.c
Release 1.2.8 tag for RRD
|
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
* Update Client_Mock.cpp * Update Client_Mock.h * Update rrdUnitTestRunner.cpp * Update rrdUnitTestRunner.cpp * Update rrdUnitTestRunner.cpp * Update rrdInterface.c * Update rrdInterface.h * Update rrdRunCmdThread.c * Update rrdIarmEvents.c * Update rrdInterface.h * Update rrdDynamic.c * Update code-coverage.yml
Create deepsleep_main.c Update Makefile Create power_controller.h Update run_l2.sh Update cov_build.sh Create pwrMgr.h Update cov_build.sh Update cov_build.sh Update Makefile Rename deepsleep_main.c to main.c Update Makefile update Makefile Delete test/functional-tests/tests/rrdIarmEvents.c Update run_l2.sh Update remote_debugger.json Update remote_debugger.json Create test_deepsleep_static.py Update run_l2.sh Update run_l2.sh Create test_deepsleep_dynamic.py Update run_l2.sh Update rrdInterface.c Update create_json.sh Create test_append.py Create test_category.py Update test_rrd_dynamic_profile_missing_report.py Create test_rrd_dynamic_with_download_harmful.py Create test_rrd_negative.py Update configure.ac Update cov_build.sh Update run_l2.sh Update main.c Rename main.c to deepsleep_main.c Update Makefile Create rrd_append_report.feature Create rrd_deepsleep_static.feature Create rrd_deepsleep_dynamic.feature Create rrd_dynamic_with_download_harmful_report.feature Update rrd_dynamic_profile_missing_report.feature Update test_rrd_dynamic_profile_missing_report.py Update cov_build.sh Update test_rrd_single_instance.py Update test_category.py Update run_l2.sh Update test_rrd_dynamic_profile_missing_report.py Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdIarmEvents.c Update rrdIarmEvents.c Delete test/functional-tests/tests/test_deepsleep_dynamic.py Delete test/functional-tests/tests/test_rrd_negative.py Update run_l2.sh Update test_category.py Update cov_build.sh Update configure.ac Update configure.ac Delete test/functional-tests/features/rrd_deepsleep_dynamic.feature Update rrd_dynamic_profile_missing_report.feature Update rrd_dynamic_profile_missing_report.feature Delete test/functional-tests/tests/test_rrd_dynamic_with_download_harmful.py Delete test/functional-tests/tests/deepsleep_main.c Delete test/functional-tests/tests/Makefile Update run_l2.sh Update run_l2.sh Delete test/functional-tests/features/rrd_dynamic_with_download_harmful_report.feature Create rrd_dynamic_profile_subcategory_report.feature Update run_l2.sh Rename test_append.py to test_rrd_append_report.py Rename test_category.py to test_rrd_dynamic_subcategory_report.py Rename test_deepsleep_static.py to test_rrd_deepsleep_static_report.py Rename rrd_deepsleep_static.feature to rrd_deepsleep_static_report.feature Update rrd_dynamic_profile_missing_report.feature Update rrd_dynamic_profile_missing_report.feature Update rrd_dynamic_profile_missing_report.feature
RDK-56291 - [RDKE] Increase L2 Test Coverage For Remote Debugger : Target 80% [ Phase 2 ]
…elop Deploy cla action
Deploy fossid_integration_stateless_diffscan_target_repo action
…rget 80% [ Phase 2 ] (#157) * Update rrdInterface.c * Update rrdIarmEvents.c * Update configure.ac * Update cov_build.sh * Update remote_debugger.json * Update create_json.sh * Create Makefile * Create deepsleep_main.c --------- Co-authored-by: nhanasi <navihansi@gmail.com>
RRD 1.2.9 release
… C (#167) * Add comprehensive HLD documentation for uploadRRDLogs.sh migration to C - Requirements document with functional specs, I/O definitions, and constraints - High-Level Design with architecture, module breakdown, and data flows - Detailed flowcharts for all major operations (Mermaid + text-based) - Sequence diagrams showing component interactions - Low-Level Design with complete C code, data structures, and implementations Documentation covers migration of uploadRRDLogs.sh to embedded C program optimized for low-memory, low-CPU embedded systems with full error handling, security considerations, and platform portability. - Replace custom MAC retrieval logic with TR-181 parameters via RBus - Device.DeviceInfo.X_COMCAST-COM_STB_MAC (Video) - Device.DeviceInfo.X_COMCAST-COM_CM_MAC (Broadband) - Device.X_CISCO_COM_MACAddress (Alternative) - Add cache file fallback (/tmp/device_details.cache, /tmp/estb_mac) - Add early exit optimization in config parser when all required properties found - Mark libarchive as mandatory dependency, remove tar command fallback
There was a problem hiding this comment.
Pull request overview
This PR appears to be a rebase that integrates multiple features and improvements for the Remote Debugger component, including:
- New L2 test coverage for deep sleep scenarios and dynamic profile handling
- Integration of power controller functionality for deep sleep testing
- Improved unit test coverage with enhanced mocking capabilities
- Bug fixes for memory leaks and resource cleanup
- New comprehensive documentation for uploadRRDLogs functionality
- Build system improvements for L2 support
Key Changes
- Added three new L2 test modules with corresponding feature files for dynamic subcategory reports, deep sleep static reports, and append request handling
- Introduced power controller test infrastructure with stub implementations
- Enhanced unit tests with better mock support for RBus API and removed conditional compilation around IARMBUS_SUPPORT
- Fixed memory management issues in event processing and improved null-safety checks
- Added extensive documentation (requirements, design, flowcharts, sequence diagrams) for uploadRRDLogs component
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/functional-tests/tests/test_rrd_single_instance.py | Improved PID handling to parse multiple PIDs and verify single instance |
| test/functional-tests/tests/test_rrd_dynamic_subcategory_report.py | New L2 test for dynamic profile subcategory handling |
| test/functional-tests/tests/test_rrd_deepsleep_static_report.py | New L2 test for deep sleep static report generation |
| test/functional-tests/tests/test_rrd_append_report.py | New L2 test for append request functionality |
| test/functional-tests/tests/power_controller.h | New header defining power controller API for deep sleep testing |
| test/functional-tests/tests/deepsleep_main.c | New test main for deep sleep event handling |
| test/functional-tests/tests/create_json.sh | Updated to include additional test configurations |
| test/functional-tests/tests/Makefile | New makefile for building deep sleep tests |
| test/functional-tests/features/*.feature | New BDD feature files for the three new test modules |
| src/unittest/rrdUnitTestRunner.cpp | Major refactor removing conditional IARMBUS compilation, adding new tests |
| src/unittest/mocks/pwrMgr.h | New mock header for power manager |
| src/unittest/mocks/Client_Mock.* | Enhanced with rbus_get support and removed IARMBUS conditionals |
| src/rrdRunCmdThread.c | Added RemainAfterExit flag and conditional test compilation guards |
| src/rrdJsonParser.c | Added deep sleep event validation logic |
| src/rrdInterface.* | Updated conditional compilation for IARMBUS/GTEST support |
| src/rrdIarmEvents.c | Refactored power manager event handling with WebCfg integration |
| src/rrdEventProcess.c | Fixed memory leaks with null-safety checks |
| src/rrdDynamic.c | Fixed buffer size calculation for deep sleep events |
| docs/uploadRRDLogs_*.md | New comprehensive documentation suite |
| run_l2.sh | Added new test executions |
| remote_debugger.json | Added DeepSleep configuration entries |
| cov_build.sh | Updated with USECOV and USE_L2_SUPPORT flags |
| configure.ac | Added L2 support configuration option |
| CHANGELOG.md | Updated with recent releases and changes |
| .github/workflows/* | Updated workflow configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DEBUG_FILE = "Adding Details of Debug commands to Output File" | ||
| assert DEBUG_FILE in grep_rrdlogs(DEBUG_FILE) | ||
|
|
||
| issue_string = "DEEPSLEEP" |
There was a problem hiding this comment.
Variable issue_string is not used.
| @@ -0,0 +1,134 @@ | |||
| import json | |||
| from helper_functions import * | |||
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.
| # limitations under the License. | ||
| ########################################################################## | ||
|
|
||
| from helper_functions import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.
|
|
||
| import json | ||
| import subprocess | ||
| from helper_functions import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.
| # limitations under the License. | ||
| ########################################################################## | ||
|
|
||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
* RDK-58172: integrate dcm-agent uploadstblogs and cleanup logging; update Makefile and upload flow * Add unit tests for RRD upload orchestration This file contains unit tests for the RRD upload orchestration, covering various aspects such as configuration loading, system information retrieval, log directory validation, and the entire upload workflow. * Update Makefile.am * Remove uploadRRDLogs program from Makefile * Refactor uploadDebugoutput to use upload API Updated the uploadDebugoutput function to call the upload API instead of executing a script. Enhanced logging for upload orchestration success and failure. * Refactor upload orchestration into rrd_upload_orchestrate Refactor upload orchestration logic into a separate function and remove deprecated main function. * Update rrd_config.h * Include common_device_api.h in rrd_sysinfo.h Added inclusion of common_device_api.h for device API access. * Add comprehensive functional tests for C API upload orchestration - Add BDD feature file with 20 test scenarios for rrd_upload_orchestrate - Add Python test implementation with 14 test functions - Test coverage includes: * Valid and invalid parameter handling * Configuration loading from multiple sources (properties, RFC, DCM) * MAC address retrieval and timestamp generation * Issue type sanitization and normalization * Archive creation and format validation * Upload execution and lock handling * Cleanup operations * Error code propagation (codes 1-11) * Comprehensive logging verification * Integration with uploadDebugoutput wrapper - Tests validate the complete flow from event trigger through C API execution to upload completion - Helper class provides utilities for test setup, archive validation, and log verification Refactor source validation and log handling. Improve error handling for directory operations and add functionality to move live logs. * Refactor log upload handling and cleanup process Updated log upload process to handle LOGUPLOAD_ENABLE case and adjusted archive creation and upload paths.
* RDKEMW-12334-The log entries in remote-debugger.log RDKEMW-12334-The log entries in remote-debugger.log differ between C binary files and script files when compared * Change the log from Debug to Info * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if '.' in ISSUE_STRING: | ||
| ISSUE_NODE, ISSUE_SUBNODE = ISSUE_STRING.split('.') | ||
| else: | ||
| node = ISSUE_STRING |
There was a problem hiding this comment.
Variable node is not used.
| ISSUE_NODE, ISSUE_SUBNODE = ISSUE_STRING.split('.') | ||
| else: | ||
| node = ISSUE_STRING | ||
| subnode = None |
There was a problem hiding this comment.
Variable subnode is not used.
| node = ISSUE_STRING | ||
| subnode = None | ||
|
|
||
| ISSUE_FOUND = f"Issue Data Node: {ISSUE_NODE} and Sub-Node: {ISSUE_SUBNODE} found in Static JSON File /etc/rrd/remote_debugger.json" |
There was a problem hiding this comment.
Variable ISSUE_FOUND is not used.
| # limitations under the License. | ||
| ########################################################################## | ||
|
|
||
| from helper_functions import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.
RRD release 1.3.1
…ode (#178) * Update Makefile.am * Update rrdExecuteScript.c * Update rrd_upload.h * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update Makefile.am --------- Co-authored-by: nhanasi <navihansi@gmail.com>
This pull request addresses Coverity defects in the RDK Remote Debugger Device Management code, focusing on fixing memory leaks, buffer overflows, and resource handling issues. Changes: Added NULL checks for memory allocations (malloc, strdup, fread) with proper error handling and resource cleanup Fixed buffer overflow vulnerabilities by using correct size parameters in strncpy/strncat operations Corrected file pointer closure to only occur when pointers are valid (v_secure_pclose moved inside if blocks) Fixed memory management issues with rfcbuf allocations and ownership Co-authored-by: mtirum011 <madhubabu_tirumala@comcast.com>
Remote Debugger 1.3.2 release
| #if !defined(GTEST_ENABLE) | ||
| v_secure_system("systemctl stop %s", remoteDebuggerServiceStr); | ||
| v_secure_system("systemctl reset-failed %s", remoteDebuggerServiceStr); | ||
| free(cmdData->rfcvalue); // free rfcvalue received from RRDEventThreadFunc | ||
| free(cmdData->command); // free updated command info received from RRDEventThreadFunc | ||
| free(cmdData); | ||
| #endif |
There was a problem hiding this comment.
The cmdData cleanup (free(cmdData->rfcvalue), free(cmdData->command), free(cmdData)) is inside #if !defined(GTEST_ENABLE). When GTEST_ENABLE is set, this function returns true without freeing those heap allocations, leaking memory in unit tests. Keep the systemctl stop guarded, but free cmdData regardless of GTEST_ENABLE.
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "%s: Removing source directory: %s\n", __FUNCTION__, dir_path); | ||
|
|
||
| char cmd[1024]; | ||
| snprintf(cmd, sizeof(cmd), "rm -rf %s", dir_path); | ||
|
|
||
| int ret = system(cmd); |
There was a problem hiding this comment.
rrd_upload_cleanup_source_dir builds a shell command (rm -rf %s) from dir_path and executes it via system(). Since dir_path can ultimately be derived from RFC/issue-type content, this is a command-injection risk and can also break on spaces/shell metacharacters. Please replace this with a non-shell recursive delete (e.g., nftw() / unlinkat() traversal) and validate that dir_path is within the expected /tmp/rrd/ subtree before deleting.
| // Try to acquire a non-blocking lock on the same file uploadstblogs uses | ||
| int fd = open("/tmp/.log-upload.lock", O_RDONLY | O_CREAT, 0644); | ||
| if (fd == -1) { |
There was a problem hiding this comment.
The lock handling here differs from the existing scripts/uploadRRDLogs.sh (which checks /tmp/.log-upload.pid) and also creates /tmp/.log-upload.lock unconditionally via O_CREAT. Creating the lock file may change behavior for components that rely on file non-existence. Consider aligning with the existing lock convention (or checking both .pid and .lock for backward compatibility) and avoid O_CREAT so a missing lock file is treated as unlocked without side effects.
| AC_ARG_ENABLE([L2support], | ||
| AS_HELP_STRING([--enable-L2support],[enable L2support (default is no)]), | ||
| [ | ||
| case "${enableval}" in | ||
| yes) L2_SUPPORT_ENABLE=true | ||
| L2_SUPPORT_FLAG="-DUSE_L2_SUPPORT" | ||
| m4_if(m4_sysval,[0],[SUBDIRS_L2_SUPPORT="src"]) ;; | ||
| no) L2_SUPPORT_ENABLE=false AC_MSG_ERROR([L2_SUPPORT is disabled]) ;; | ||
| *) AC_MSG_ERROR([bad value ${enableval} for --enable-L2support]) ;; | ||
| esac | ||
| ], | ||
| [echo "L2support is disabled"]) |
There was a problem hiding this comment.
--enable-L2support currently sets L2_SUPPORT_FLAG/L2_SUPPORT_ENABLE but neither is used later (no AC_SUBST, no AM_CONDITIONAL, and src/Makefile.am doesn’t reference the flag). As-is, the option has no effect on the build. Please either wire L2_SUPPORT_FLAG into the relevant CFLAGS via AC_SUBST/Makefile.am (and/or add an AM_CONDITIONAL), or remove the option to avoid a misleading configure interface.
| AC_ARG_ENABLE([L2support], | |
| AS_HELP_STRING([--enable-L2support],[enable L2support (default is no)]), | |
| [ | |
| case "${enableval}" in | |
| yes) L2_SUPPORT_ENABLE=true | |
| L2_SUPPORT_FLAG="-DUSE_L2_SUPPORT" | |
| m4_if(m4_sysval,[0],[SUBDIRS_L2_SUPPORT="src"]) ;; | |
| no) L2_SUPPORT_ENABLE=false AC_MSG_ERROR([L2_SUPPORT is disabled]) ;; | |
| *) AC_MSG_ERROR([bad value ${enableval} for --enable-L2support]) ;; | |
| esac | |
| ], | |
| [echo "L2support is disabled"]) | |
| # Note: legacy --enable-L2support option removed because it had no effect on the build. |
| file_path = "/etc/rrd/remote_debugger.json" | ||
|
|
||
| # Read the existing JSON data | ||
| with open(file_path, "r") as json_file: | ||
| data = json.load(json_file) | ||
|
|
||
| # New entry to add | ||
| new_entry = { | ||
| "Test": { | ||
| "TestRun4": { | ||
| "Commands": "cat /version.txt;cat /tmp/.deviceDetails.cache", | ||
| "Timeout": 10 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # Update the JSON data with the new entry | ||
| data.update(new_entry) | ||
|
|
||
| # Write the updated data back to the JSON file | ||
| with open(file_path, "w") as json_file: | ||
| json.dump(data, json_file, indent=4) |
There was a problem hiding this comment.
This test mutates /etc/rrd/remote_debugger.json at import time (module top-level). That runs before pytest collection/fixtures, makes the test order-dependent, and can fail in environments where /etc is read-only. Please move this JSON update into a test/fixture (e.g., setup/teardown that backs up and restores the original file), or write to a temporary config path and point the component/test to that path.
| try: | ||
| result = subprocess.run(['bash', script_path], check=True, text=True, capture_output=True) | ||
| print("Script output:") | ||
| print(result.stdout) | ||
| except subprocess.CalledProcessError as e: | ||
| print("Error while executing the script:") | ||
| print(e.stderr) |
There was a problem hiding this comment.
The create_json.sh invocation swallows failures: on CalledProcessError it only prints stderr and the test still passes. This can hide setup failures and make the test results misleading. Please fail the test on error (e.g., re-raise or assert with stderr) and/or assert on expected file output from the script.
| SCRIPT_SUCCESS = "Debug Information Report upload Failed" | ||
| SCRIPT_FAILURE = "Debug Information Report upload Success" | ||
| if SCRIPT_SUCCESS in grep_rrdlogs(SCRIPT_SUCCESS): | ||
| print("Script execution success") | ||
| elif SCRIPT_FAILURE in grep_rrdlogs(SCRIPT_FAILURE): | ||
| print("Script execution failed") |
There was a problem hiding this comment.
SCRIPT_SUCCESS/SCRIPT_FAILURE constants are swapped: SCRIPT_SUCCESS is set to the "...upload Failed" log string and then prints "Script execution success" when that failure string is found (and vice versa). This makes the test output misleading. Please rename or swap the constants/branches so success maps to the success log message.
| processIssueTypeInStaticProfile(rbuf, pIssueNode); | ||
| } | ||
| //CID-336989: Resource leak | ||
| free(pIssueNode); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
pIssueNode (and its Node/subNode fields allocated by getIssueInfo()) is no longer freed after processing. This introduces a per-event memory leak. Please restore cleanup (free pIssueNode->Node, pIssueNode->subNode, then pIssueNode) once processing is complete.
No description provided.