rebase#156
Conversation
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 includes significant additions for L2 test coverage improvements and new documentation for an uploadRRDLogs C implementation. The changes include:
- Enhanced test coverage with new test files and improved existing tests
- Comprehensive documentation for uploadRRDLogs migration from shell script to C
- New power management integration with deepsleep functionality
- Build system improvements with L2 support configuration
- Updated CI/CD workflows and project maintenance files
Reviewed changes
Copilot reviewed 27 out of 27 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 with proper list operations |
| test/functional-tests/tests/test_rrd_dynamic_subcategory_report.py | New comprehensive test for dynamic subcategory reporting |
| test/functional-tests/tests/test_rrd_deepsleep_static_report.py | New test for deepsleep static report functionality |
| test/functional-tests/tests/test_rrd_append_report.py | New test for report append functionality |
| test/functional-tests/tests/power_controller.h | New power controller API header with comprehensive interface |
| test/functional-tests/tests/deepsleep_main.c | New C test implementation for deepsleep event handling |
| test/functional-tests/tests/create_json.sh | Updated JSON creation with additional test configurations |
| test/functional-tests/tests/Makefile | New Makefile for compiling C test binaries |
| test/functional-tests/features/*.feature | New Gherkin feature files for BDD test scenarios |
| src/unittest/mocks/pwrMgr.h | New mock header for power manager IARM interface |
| src/rrdInterface.c | Updated RBus subscription logic with L2 support conditionals |
| src/rrdIarmEvents.c | Updated WebCfg force sync error handling with L2 support |
| run_l2.sh | Added new test executions and configuration copy |
| remote_debugger.json | Added DeepSleep configuration profiles |
| docs/uploadRRDLogs_*.md | New comprehensive documentation for uploadRRDLogs C migration |
| cov_build.sh | Updated build script with L2 support flags |
| configure.ac | Added L2 support configuration option |
| CHANGELOG.md | Updated with 1.2.9 release notes |
| .github/workflows/*.yml | Updated CI/CD workflows with improved security |
| .github/CODEOWNERS | Simplified code ownership |
💡 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.
| issue_string = "DEEPSLEEP" |
| @@ -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'.
| from helper_functions import * | |
| from helper_functions import ( | |
| check_file_exists, | |
| check_directory_exists, | |
| kill_rrd, | |
| remove_logfile, | |
| run_shell_silent, | |
| run_shell_command, | |
| grep_rrdlogs, | |
| JSON_FILE, | |
| OUTPUT_DIR, | |
| APPEND_JSON_FILE, | |
| DYNAMIC_DIR, | |
| ) |
| # 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'.
| from helper_functions import * | |
| from helper_functions import ( | |
| JSON_FILE, | |
| check_file_exists, | |
| OUTPUT_DIR, | |
| check_directory_exists, | |
| kill_rrd, | |
| remove_logfile, | |
| run_shell_silent, | |
| run_shell_command, | |
| grep_rrdlogs, | |
| remove_outdir_contents | |
| ) |
|
|
||
| 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'.
| from helper_functions import * | |
| from helper_functions import ( | |
| JSON_FILE, | |
| check_file_exists, | |
| OUTPUT_DIR, | |
| check_directory_exists, | |
| kill_rrd, | |
| remove_logfile, | |
| run_shell_silent, | |
| run_shell_command, | |
| sleep, | |
| grep_rrdlogs, | |
| ) |
| # limitations under the License. | ||
| ########################################################################## | ||
|
|
||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
| import json |
* 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 56 out of 56 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SCRIPT_SUCCESS = "Debug Information Report upload Failed" | ||
| SCRIPT_FAILURE = "Debug Information Report upload Success" |
There was a problem hiding this comment.
The variable names SCRIPT_SUCCESS and SCRIPT_FAILURE appear to be swapped. SCRIPT_SUCCESS is assigned "Debug Information Report upload Failed" but should be "Debug Information Report upload Success". Similarly, SCRIPT_FAILURE is assigned "Debug Information Report upload Success" but should be "Debug Information Report upload Failed".
| 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.
| @@ -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'.
| # 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.
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>
| mkdir -p /media/apps/RDK-RRD-Test/etc/rrd | ||
|
|
||
| touch /media/apps/RDK-RRD-Test/etc/rrd/remote_debugger.json | ||
| echo "AA:BB:CC:DD:EE:FF" >> /tmp/.estb_mac |
There was a problem hiding this comment.
This appends the mock MAC to /tmp/.estb_mac on every run (>>), which can leave multiple lines and break any consumer expecting a single MAC value. Use overwrite (>) and/or ensure the file is truncated/rewritten deterministically.
| echo "AA:BB:CC:DD:EE:FF" >> /tmp/.estb_mac | |
| echo "AA:BB:CC:DD:EE:FF" > /tmp/.estb_mac |
| 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.
The --enable-L2support option sets L2_SUPPORT_FLAG/L2_SUPPORT_ENABLE, but these variables are never AC_SUBST’d or wired into any AM_CONDITIONAL/Makefile logic, so the flag has no effect on the build. Either plumb this through (e.g., AC_SUBST(L2_SUPPORT_FLAG) and append it in src/Makefile.am, or add an AM_CONDITIONAL) or remove the option to avoid misleading configure behavior.
| /* Get the device MAC address. | ||
| * @param mac_addr Buffer to store MAC address (min 18 bytes) | ||
| * @param size Size of buffer | ||
| * @return 0 on success, -1 on error | ||
| */ | ||
| int rrd_sysinfo_get_mac_address(char *mac_addr, size_t size); |
There was a problem hiding this comment.
The header comment says the MAC buffer needs at least 18 bytes, but the implementation accepts 13 bytes (12 hex chars + NUL) after stripping colons. Update the comment (or the implementation) so the contract is consistent.
| # Path to the existing JSON file | ||
| 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 modifies /etc/rrd/remote_debugger.json at import/collection time (top-level file I/O). If the file is missing or not writable, pytest will fail before running any tests, and it also creates global side effects that can leak into other tests. Move the JSON read/update/write into a fixture or a dedicated test function with proper setup/teardown (and preferably write to a temporary copy instead of editing the system file in place).
| # Verify archive was created | ||
| ARCHIVE_CREATED = "Archive created:" | ||
| if ARCHIVE_CREATED in grep_rrdlogs(ARCHIVE_CREATED): | ||
| print("Archive creation confirmed") | ||
| else: | ||
| print("Archive creation not found in logs") |
There was a problem hiding this comment.
The test looks for the literal string "Archive created:" in logs, but the new archive implementation logs "Archive created successfully:" (see rrd_archive_create). With the current search string (including the colon immediately after "created"), this will not match and will produce false negatives. Update the expected log substring to match the new message (e.g., "Archive created successfully" or "Archive created successfully:").
| int rrd_upload_cleanup_source_dir(const char *dir_path) { | ||
| if (!dir_path) return -1; | ||
|
|
||
| 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); | ||
| if (ret == 0) { | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "%s: Successfully removed source directory: %s\n", | ||
| __FUNCTION__, dir_path); | ||
| return 0; | ||
| } else { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "%s: Failed to remove source directory: %s (ret: %d)\n", | ||
| __FUNCTION__, dir_path, ret); | ||
| return -1; |
There was a problem hiding this comment.
rrd_upload_cleanup_source_dir shells out to rm -rf %s using system() with an unquoted path. Since dir_path ultimately comes from runtime inputs (upload directory), this is command-injection prone and can also mis-handle paths with spaces/shell metacharacters. Replace this with a safe recursive delete implementation (e.g., nftw()/FTS, or explicit opendir/readdir/unlink/rmdir) and avoid invoking a shell.
| typedef enum PowerController_ThermalTemperature { | ||
| THERMAL_TEMPERATURE_UNKNOWN = 0 /* UNKNOWN Thermal Temperature */, | ||
| THERMAL_TEMPERATURE_NORMAL = 1 /* Normal Thermal Temperature */, | ||
| THERMAL_TEMPERATURE_HIGH = 2 /* High Thermal Temperature */, | ||
| THERMAL_TEMPERATURE_CRITICAL = 4 /* Critial Thermal Temperature */ | ||
| } PowerController_ThermalTemperature_t; |
There was a problem hiding this comment.
Spelling: "Critial" should be "Critical" in the enum comment/value description to keep the public header clean and searchable.
| 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 are assigned to the opposite log messages ("upload Failed" vs "upload Success"), and the subsequent prints also invert success/failure. This makes the test output misleading and can hide real failures; swap the constants/branches so success corresponds to the success log line.
| if '.' in ISSUE_STRING: | ||
| ISSUE_NODE, ISSUE_SUBNODE = ISSUE_STRING.split('.') | ||
| else: | ||
| 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.
In the else branch, ISSUE_NODE/ISSUE_SUBNODE are never set, but they are still used to build ISSUE_FOUND below. This will raise UnboundLocalError when ISSUE_STRING has no '.' (and currently the default ISSUE_STRING in helper_functions is Device.Info, but this test should be correct for all inputs). Initialize ISSUE_NODE/ISSUE_SUBNODE in both branches (or build the expected log message conditionally).
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
| 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]) ;; |
There was a problem hiding this comment.
The new --enable-L2support option fails the configure step when explicitly set to "no" (AC_MSG_ERROR), and the computed L2_SUPPORT_FLAG/SUBDIRS_L2_SUPPORT variables are never AC_SUBST’d or used anywhere else. This makes the option either unusable or a no-op; consider treating "no" as a normal value and wiring the flag via AC_SUBST/AM_CONDITIONAL so it actually affects compilation.
| #endif | ||
| #if !defined(GTEST_ENABLE) | ||
| rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, 3); | ||
| ret = rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, 3); |
There was a problem hiding this comment.
RRD_subscribe() may subscribe only 2 RBUS events (when IARMBUS_SUPPORT && !USE_L2_SUPPORT), but RRD_unsubscribe() always unsubscribes 3. This mismatch can cause RBUS unsubscribe to fail or access an unsubscribed slot; track the subscribed count (2 vs 3) and pass the same count into rbusEvent_UnsubscribeEx().
| ret = rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, 3); | |
| int subscriptionCount = 3; | |
| #if defined(IARMBUS_SUPPORT) && !defined(USE_L2_SUPPORT) | |
| subscriptionCount = 2; | |
| #endif | |
| ret = rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, subscriptionCount); |
| 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); | ||
| if (ret == 0) { |
There was a problem hiding this comment.
rrd_upload_cleanup_source_dir() builds a shell command with unquoted dir_path and executes it via system("rm -rf %s"). If dir_path contains spaces or shell metacharacters, this can delete unintended paths or enable command injection. Prefer a safe recursive delete implementation (nftw/unlinkat) or at minimum avoid invoking a shell / properly escape and validate the path.
| // Trim leading | ||
| while (*start == ' ' || *start == '\t' || *start == '\n' || *start == '\r') start++; | ||
|
|
||
| // Trim trailing | ||
| end = start + strlen(start) - 1; | ||
| while (end > start && (*end == ' ' || *end == '\t' || *end == '\n' || *end == '\r')) *end-- = 0; | ||
|
|
There was a problem hiding this comment.
trim() computes end = start + strlen(start) - 1 without handling the case where the string becomes empty after trimming leading whitespace. When strlen(start) == 0, this does pointer arithmetic to before the buffer (undefined behavior). Add an early return when *start == '\0' before calculating end.
| def reset_issuetype_rfc(): | ||
| command = 'rbuscli set Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType string ""' | ||
| result = subprocess.run(command, shell=True, capture_output=True, text=True) | ||
| assert result.returncode == 0 | ||
|
|
There was a problem hiding this comment.
subprocess is used in reset_issuetype_rfc() (subprocess.run) but this file does not import subprocess. This will raise NameError at runtime; add the missing import or route through an existing helper that wraps command execution.
| def reset_issuetype_rfc(): | ||
| command = 'rbuscli set Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType string ""' | ||
| result = subprocess.run(command, shell=True, capture_output=True, text=True) | ||
| assert result.returncode == 0 |
There was a problem hiding this comment.
reset_issuetype_rfc() uses subprocess.run but subprocess is not imported in this file. This will raise NameError at runtime; add the missing import.
| } | ||
| } | ||
| }, | ||
| "DeepSleep": { |
There was a problem hiding this comment.
The codebase constant for deep sleep handling is DEEP_SLEEP_STR="DEEPSLEEP" (src/rrdCommon.h). This JSON adds a category key "DeepSleep" which will not be found by the case-sensitive JSON lookup (cJSON_GetObjectItem). Consider renaming the key to "DEEPSLEEP" (or making lookups case-insensitive) so deep sleep events resolve correctly.
| "DeepSleep": { | |
| "DEEPSLEEP": { |
| And the directory should be created to store the executed output | ||
| And Sanity check to validate the commands should be executed | ||
| And Command output shopuld be added to the output file | ||
| And the issuetype systemd service should start successfully | ||
| And the journalctl service should start successfully |
There was a problem hiding this comment.
Typo in step text: "shopuld" should be "should".
| And the directory should be created to store the executed output | ||
| And Sanity check to validate the commands should be executed | ||
| And Command output shopuld be added to the output file | ||
| And the issuetype systemd service should start successfully | ||
| And the journalctl service should start successfully | ||
| And the process should sleep with timeout |
There was a problem hiding this comment.
Typo in step text: "shopuld" should be "should".
| if [ ! -d tr69hostif ]; then | ||
| git clone https://github.com/rdkcentral/tr69hostif.git | ||
| fi | ||
| git clone https://github.com/rdkcentral/dcm-agent.git -b develop |
There was a problem hiding this comment.
dcm-agent is cloned unconditionally. If cov_build.sh is re-run in an environment where /usr/dcm-agent already exists, git clone will fail and stop the build. Consider guarding it like the other dependencies (clone only if the directory is missing) or doing a git -C dcm-agent fetch/reset if it already exists.
| git clone https://github.com/rdkcentral/dcm-agent.git -b develop | |
| if [ ! -d dcm-agent ]; then | |
| git clone https://github.com/rdkcentral/dcm-agent.git -b develop | |
| else | |
| cd dcm-agent | |
| git fetch origin develop | |
| git reset --hard origin/develop | |
| cd .. | |
| fi |
* RDKEMW-16897: Add Agentic Support for the RRD * Updated the instructions --------- Co-authored-by: Hanasi <nhanas001c@cable.comcast.com>
…ile Data (#183) * Update rrdInterface.h Update rrdInterface.h Update rrdInterface.c Update rrdInterface.h Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Add helper functions for profile data processing Added helper functions for profile data processing. Update rrdInterface.c Update rrdInterface.h Update Client_Mock.h Add mock implementations for RBusApiWrapper methods Remove mock profile handler functions for GTEST Removed mock implementations for profile handler functions when GTEST_ENABLE is defined. Update Client_Mock.cpp Update Client_Mock.h Update rrdInterface.h Update Makefile.am Update src/rrdInterface.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Create profileTestValid.json Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Update rrdUnitTestRunner.cpp Create profileTestInvalid.json Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Update rrdInterface.c Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdInterface.c Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> fix: add NULL guard to has_direct_commands to prevent crash on NULL input Agent-Logs-Url: https://github.com/rdkcentral/remote_debugger/sessions/c84bc9e8-e1d2-4412-81d4-54eb9c67be5f Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Create test.py Convert run_l2.sh to Python with RDK tests Refactor run_l2.sh to Python and implement RDK Remote Debugger tests. L2 Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdInterface.c fix: correct set_rbus_response to use real rbus API signatures rbusValue_Init() returns rbusValue_t (not rbusError_t) and rbusValue_SetString() returns void in the real rbus library, so capturing their return values as rbusError_t caused a build error. - Remove incorrect return-value capture from rbusValue_Init and rbusValue_SetString; check rbusValue != NULL after init instead - Add NULL guard for prop parameter - Update RRDProfileHandlerTest SetUp() mock so rbusValue_Init actually sets *value to a non-null pointer, keeping SetRbusResponse_ValidInput test passing Agent-Logs-Url: https://github.com/rdkcentral/remote_debugger/sessions/e887c84a-2e12-4903-9d59-2bd6f90e2527 Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Update rrdInterface.c Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> fix: resolve Coverity dead code and add NULL guard to get_all_categories_json - read_profile_json_file: split combined NULL check into two separate checks (file_size first, then filename) so *file_size is safely zeroed before the filename check; eliminates the ternary 'filename ? filename : "(null)"' and the Coverity dead-code report - get_all_categories_json: add early NULL guard for json parameter that returns an empty JSON object string to prevent crash when callers (including unit tests) pass NULL Agent-Logs-Url: https://github.com/rdkcentral/remote_debugger/sessions/c686520e-0adf-4e7e-9915-9b9e4e8a74fd Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdUnitTestRunner.cpp Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdInterface.c Update rrdInterface.c Update rrdUnitTestRunner.cpp Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdUnitTestRunner.cpp Update src/rrdInterface.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdInterface.c Clean up comments in save_profile_category function Removed comments explaining file opening flags. RDKEMW-16897: Add Agentic Support for the RRD (#182) * RDKEMW-16897: Add Agentic Support for the RRD * Updated the instructions --------- Co-authored-by: Hanasi <nhanas001c@cable.comcast.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Rename test.py to test_rrd_profile_data.py rrd Update src/rrdInterface.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update src/rrdInterface.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update rrdUnitTestRunner.cpp * Update rrdInterface.c * Update rrdUnitTestRunner.cpp
| def reset_issuetype_rfc(): | ||
| command = 'rbuscli set Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType string ""' | ||
| result = subprocess.run(command, shell=True, capture_output=True, text=True) | ||
| assert result.returncode == 0 |
There was a problem hiding this comment.
subprocess is used in reset_issuetype_rfc/test_remote_debugger_trigger_event, but this file only imports helper_functions. This will raise NameError at runtime unless subprocess is imported here. Add an explicit import subprocess (or refactor to use helper utilities).
| 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") | ||
| else: |
There was a problem hiding this comment.
The success/failure constants are inverted: SCRIPT_SUCCESS is set to the "...upload Failed" message and SCRIPT_FAILURE to the "...upload Success" message, so the printed status will be wrong. Swap these strings (or swap the conditional branches) to match the log semantics.
| 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.
If create_json.sh fails, the exception is caught and only printed, so the test still passes and later assertions may become misleading. After logging e.stderr, explicitly fail the test (e.g., re-raise, assert False, or pytest.fail).
|
|
||
| # Define the compiler flags | ||
| COMMON_CXXFLAGS = -frtti -fprofile-arcs -ftest-coverage | ||
| COMMON_CXXFLAGS = -frtti -fprofile-arcs -ftest-coverage -fpermissive |
There was a problem hiding this comment.
Adding -fpermissive masks C++ type/const correctness errors and can hide real issues in the unit test build. Prefer fixing the underlying compilation errors (especially in mocks/interfaces) and keep the build strict.
| git clone https://github.com/rdkcentral/dcm-agent.git -b develop | ||
|
|
||
| cd dcm-agent | ||
| sh cov_build.sh | ||
| autoreconf -i | ||
| ./configure | ||
| make && make install | ||
| cp uploadstblogs/include/*.h /usr/local/include | ||
| cd - |
There was a problem hiding this comment.
dcm-agent is cloned unconditionally on every run, and the build is pinned to the moving develop branch. This makes builds non-reproducible and will fail if dcm-agent/ already exists. Add an existence guard (similar to other deps) and pin to a tag/commit (or allow an override) to keep CI/local builds deterministic.
| def reset_issuetype_rfc(): | ||
| command = 'rbuscli set Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType string ""' | ||
| result = subprocess.run(command, shell=True, capture_output=True, text=True) | ||
| assert result.returncode == 0 | ||
|
|
||
| def test_remote_debugger_trigger_event(): | ||
| APPEND_STRING1 = "Test.TestRun4_apnd" | ||
| reset_issuetype_rfc() | ||
| sleep(10) | ||
| command = [ | ||
| 'rbuscli', 'set', | ||
| 'Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType', | ||
| 'string', APPEND_STRING1 | ||
| ] | ||
| result = subprocess.run(command, capture_output=True, text=True) | ||
| assert result.returncode == 0 |
There was a problem hiding this comment.
subprocess is used in reset_issuetype_rfc and test_remote_debugger_trigger_event, but it's never imported in this file, which will raise NameError when the tests run. Add the missing import (or use the helper function wrappers consistently).
| print("Test Case 1: Setting profile data to 'all'") | ||
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "all" {set_param}') | ||
| print(f"Set command result: stdout='{stdout}', stderr='{stderr}', rc={rc}") | ||
| assert rc == 0, f"rbuscli set 'all' failed: {stderr}" |
There was a problem hiding this comment.
These rbuscli set invocations use a different argument order and omit the value type compared to the rest of the test suite (e.g., rbuscli set <param> string <value> used elsewhere). Align the command format with the established usage in this repo to avoid false failures due to incorrect CLI syntax.
| def remove_upload_lock(): | ||
| """Remove the upload lock file to prevent test hangs""" | ||
| lock_file = "/tmp/.log-upload.lock" | ||
| try: | ||
| if os.path.exists(lock_file): | ||
| os.remove(lock_file) | ||
| print(f"Upload lock file {lock_file} removed.") | ||
| else: | ||
| print(f"Upload lock file {lock_file} does not exist.") | ||
| except Exception as e: | ||
| print(f"Could not remove upload lock file {lock_file}: {e}") |
There was a problem hiding this comment.
Removing /tmp/.log-upload.lock is not a reliable way to clear an active flock lock (unlinking doesn’t release the lock), and can accidentally bypass the lock by creating a new inode at the same path. Instead of deleting the lock file, wait using the same locking mechanism (or ensure no uploader process is running in the test environment).
RRD release for static profile DML
No description provided.