Rebase #48

Abhinavpv28 · 2026-01-09T05:09:52Z

…ts To C Implementation (#35)

Created a comprehensive L2 test suite with 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies Updated build scripts and CI/CD workflow to compile with L2_TEST_ENABLED flag and execute the new test suite

This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.

…ts To C Implementation (#35) * Created a comprehensive L2 test suite with 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies Updated build scripts and CI/CD workflow to compile with L2_TEST_ENABLED flag and execute the new test suite This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.

Copilot

Pull request overview

This pull request adds a comprehensive L2 (Level 2) test suite for the uploadSTBLogs service, consisting of 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies. The changes include conditionally disabling sleep() calls in the reboot strategy implementation when testing is enabled, and updates to build scripts and CI/CD workflows to support the new test infrastructure.

Key Changes

Conditional compilation of sleep() calls in strategy_reboot.c using L2_TEST_ENABLED flag to speed up testing
New test infrastructure with Python-based test suites and Gherkin feature files for behavior-driven testing
Build configuration updates to include L2_TEST_ENABLED flag and reference the correct branch

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 82 comments.

Show a summary per file

File	Description
uploadstblogs/src/strategy_reboot.c	Wraps sleep() calls with #ifndef L2_TEST_ENABLED to allow faster test execution
test/run_uploadstblogs_l2.sh	Test runner script that executes all 6 L2 test suites with proper environment setup
test/functional-tests/tests/uploadstblogs_helper.py	Helper functions for test execution including log parsing, process management, and file operations
test/functional-tests/tests/test_uploadstblogs_*.py	Six comprehensive test modules covering different aspects of the uploadSTBLogs service
test/functional-tests/features/*.feature	Gherkin feature files describing test scenarios in behavior-driven format
cov_build.sh	Updated to use feature/upload_L2 branch and add L2_TEST_ENABLED compiler flag
.github/workflows/L2-tests.yml	Updated CI/CD workflow to execute the new L2 test suite

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-09T05:18:59Z

test/functional-tests/tests/uploadstblogs_helper.py

+    result = subprocess.run(f"grep '{marker}' /opt/logs/telemetry*.log 2>/dev/null", 
+                          shell=True, capture_output=True, text=True)


Using shell=True with subprocess.run poses a security risk when the marker parameter contains unsanitized input. The marker parameter is directly interpolated into a shell command. Consider using a list of arguments without shell=True or properly sanitizing the marker parameter.

Copilot · 2026-01-09T05:18:59Z

test/functional-tests/tests/uploadstblogs_helper.py

+    result = subprocess.run(f"grep -c '{pattern}' {log_file} 2>/dev/null || echo 0", 
+                          shell=True, capture_output=True, text=True)
+    return int(result.stdout.strip())


Using shell=True with subprocess.run poses a security risk when the pattern parameter contains unsanitized input. The pattern parameter is directly interpolated into a grep command. Consider validating/sanitizing the pattern parameter or using a Python-based search instead of grep.

Suggested change

result = subprocess.run(f"grep -c '{pattern}' {log_file} 2>/dev/null || echo 0",

shell=True, capture_output=True, text=True)

return int(result.stdout.strip())

try:

count = 0

with open(log_file, "r", encoding="utf-8", errors="ignore") as f:

for line in f:

if pattern in line:

count += 1

return count

except (FileNotFoundError, PermissionError, OSError):

return 0

Copilot · 2026-01-09T05:18:59Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+        # Check for archive creation logs
+        archive_logs = grep_uploadstb_logs_regex(r"Archive created successfully")
+        # Process should complete successfully
+        assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"


The assertion message contains an f-string that is not properly formatted. The message should use an f-string prefix to include the variable values, i.e., f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

Suggested change

assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

assert len(archive_logs) > 0, f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

Copilot · 2026-01-09T05:19:00Z

test/functional-tests/features/uploadstblogs_retry_logic.feature

@@ -0,0 +1,70 @@
+####################################################################################
+# If not stated otherwise in this file or this component's Licenses


The comment header has a typo. It should say "Licenses.txt file" instead of just "Licenses file". This inconsistency appears across multiple files in this PR and should be corrected for consistency with standard RDK copyright headers.

Suggested change

# If not stated otherwise in this file or this component's Licenses

# If not stated otherwise in this file or this component's Licenses.txt file

Copilot · 2026-01-09T05:19:00Z

cov_build.sh

 cd telemetry
 cp include/*.h /usr/local/include
-sh  build_inside_container.sh 
+sh  build_inside_container.sh


This line has an unnecessary trailing space after the shell command. The space should be removed for cleaner code.

Suggested change

sh build_inside_container.sh

sh build_inside_container.sh

Copilot · 2026-01-09T05:19:19Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+"""
+
+import pytest
+import time


Import of 'time' is not used.

Copilot · 2026-01-09T05:19:19Z

test/functional-tests/tests/test_uploadstblogs_security.py

+"""
+
+import pytest
+import time


Import of 'time' is not used.

Copilot · 2026-01-09T05:19:19Z

test/functional-tests/tests/uploadstblogs_helper.py

+import os
+import time
+import re
+import json


Import of 'json' is not used.

Copilot · 2026-01-09T05:19:19Z

test/functional-tests/tests/uploadstblogs_helper.py

+    """Get file size in bytes"""
+    try:
+        return os.path.getsize(filepath)
+    except:


Except block directly handles BaseException.

Copilot · 2026-01-09T05:19:20Z

test/run_uploadstblogs_l2.sh

+cd /usr/common_utilities
+sed -i '/file_upload\.sslverify/s/= 1;/= 0;/' uploadutils/mtls_upload.c
+sed -i 's/\(ret_code = setCommonCurlOpt(curl, s3url, NULL, \)true\()\)/\1false\2/g' uploadutils/uploadUtil.c
+sed -i '/if (auth) {/,/}/s/^/\/\/ /' uploadutils/uploadUtil.c


The test runner script directly patches /usr/common_utilities/uploadutils/mtls_upload.c and uploadutils/uploadUtil.c to disable TLS certificate verification and bypass authentication by forcing file_upload.sslverify to 0, changing setCommonCurlOpt(..., true) to false, and commenting out the if (auth) { ... } block. If this script is run in any environment whose binaries are then used beyond isolated testing, it produces builds that accept unverified HTTPS servers and skip authentication, enabling man-in-the-middle attacks and unauthorized log uploads. Instead of source-level sed patches that weaken security controls, introduce test-specific configuration or compile-time flags that preserve proper certificate validation and authentication in all non-test builds.

…ts To C Implementation (#49) * Simplify some pieces of code over engineered by AI models. * Update run_uploadstblogs_l2.sh for L2 coverage --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>

…ts To C Implementation (#51) * Remove logging of response URLs * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Shibu Kakkoth Vayalambron <shibu.kakkoth@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot · 2026-01-13T18:18:12Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for configuration error detection
+        error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid")


Variable error_logs is not used.

Suggested change

error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid")

error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid")

assert len(error_logs) > 0, "Configuration error should be logged for corrupted device properties"

Copilot · 2026-01-13T18:18:13Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for informative message
+        logs = grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload")


Variable logs is not used.

Suggested change

logs = grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload")

grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload")

Copilot · 2026-01-13T18:18:13Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Should not attempt upload
+        upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")


Variable upload_logs is not used.

Suggested change

upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")

upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")

assert len(upload_logs) == 0, "Should not log upload attempts without files"

…ts To C Implementation (#54) * Update cov_build.sh to enable the rdkcertselector feature in the common_utilities build configuration to support the migration of operation support log upload scripts to C implementation. The change is part of resolving L2 test failures.

Copilot

Pull request overview

Copilot reviewed 61 out of 62 changed files in this pull request and generated 29 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T19:05:58Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for configuration error detection
+        error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid")


Copilot · 2026-01-21T19:05:58Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for size-related logs
+        logs = grep_uploadstb_logs_regex(r"size|large|limit|truncate")


Copilot · 2026-01-21T19:05:58Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+        assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"
+
+        upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")
+        # Telemetry should be attempted
+        assert len(archive_logs) > 0, "Upload Process should complete and succeed"


Variable upload_logs is not used.

Suggested change

assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")

# Telemetry should be attempted

assert len(archive_logs) > 0, "Upload Process should complete and succeed"

assert len(archive_logs) > 0, (

f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

)

upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")

# Telemetry should be attempted

assert len(upload_logs) > 0, (

f"Upload process should complete and succeed. Found {len(upload_logs)} upload-related logs: {upload_logs}"

)

Copilot · 2026-01-21T19:05:59Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+        # Check for compression/archive activity
+        compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz")


Variable compression_logs is not used.

Suggested change

# Check for compression/archive activity

compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz")

Copilot · 2026-01-21T19:05:59Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for no files detection
+        no_files_logs = grep_uploadstb_logs_regex(r"no.*file|empty|not found|no logs")


Variable no_files_logs is not used.

Copilot · 2026-01-21T19:06:04Z

test/functional-tests/tests/test_uploadstblogs_upload_strategies.py

+        result = run_uploadstblogs()
+
+        # Check for event publishing
+        event_logs = grep_uploadstb_logs_regex(r"event|publish|success")


Variable event_logs is not used.

Suggested change

event_logs = grep_uploadstb_logs_regex(r"event|publish|success")

grep_uploadstb_logs_regex(r"event|publish|success")

Copilot · 2026-01-21T19:06:05Z

test/functional-tests/tests/test_uploadstblogs_upload_strategies.py

+        result = run_uploadstblogs(args)
+
+        # Check strategy logging
+        logs = grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type")


Variable logs is not used.

Suggested change

logs = grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type")

grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type")

Copilot · 2026-01-21T19:06:05Z

test/functional-tests/tests/uploadstblogs_helper.py

+    try:
+        subprocess.run(f"echo '' > {UPLOADSTB_LOG}", shell=True)
+        return True
+    except:


Except block directly handles BaseException.

Copilot · 2026-01-21T19:06:05Z

test/functional-tests/tests/uploadstblogs_helper.py

+        if os.path.exists(LOCK_FILE):
+            os.remove(LOCK_FILE)
+        return True
+    except:


Except block directly handles BaseException.

Copilot · 2026-01-21T19:06:05Z

test/functional-tests/tests/uploadstblogs_helper.py

+            for chunk in iter(lambda: f.read(4096), b""):
+                md5_hash.update(chunk)
+        return md5_hash.hexdigest()
+    except:


Except block directly handles BaseException.

…ts To C Implementation (#60)

Copilot

Pull request overview

Copilot reviewed 62 out of 63 changed files in this pull request and generated 22 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-03T09:08:48Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+        result = run_uploadstblogs()
+
+        # Check for size limit detection
+        size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed")


Variable size_logs is not used.

Suggested change

size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed")

grep_uploadstb_logs_regex(r"size.*limit|too large|exceed")

Copilot · 2026-02-03T09:08:48Z

test/functional-tests/tests/test_uploadstblogs_resource_management.py

+
+        # Check archives before
+        archives_before = sp.run("ls /tmp/*.tgz 2>/dev/null | wc -l", 
+                                shell=True, capture_output=True, text=True)
+
+        result = run_uploadstblogs()
+        time.sleep(2)
+


Variable archives_before is not used.

Suggested change

# Check archives before

archives_before = sp.run("ls /tmp/*.tgz 2>/dev/null | wc -l",

shell=True, capture_output=True, text=True)

result = run_uploadstblogs()

time.sleep(2)

result = run_uploadstblogs()

time.sleep(2)

Copilot · 2026-02-03T09:08:48Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+
+        upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP")
+        # Telemetry should be attempted
+        assert len(archive_logs) > 0, "Upload Process should complete and succeed"


Variable upload_logs is not used.

Suggested change

assert len(archive_logs) > 0, "Upload Process should complete and succeed"

assert len(upload_logs) > 0, "Upload Process should complete and succeed"

Copilot · 2026-02-03T09:08:49Z

test/functional-tests/tests/test_uploadstblogs_resource_management.py

+        proc = sp.Popen([UPLOADSTB_BINARY], stdout=sp.PIPE, stderr=sp.PIPE)
+        time.sleep(2)
+
+        memory_during = check_memory_usage("uploadSTBLogs")


Variable memory_during is not used.

Suggested change

memory_during = check_memory_usage("uploadSTBLogs")

memory_during = check_memory_usage("uploadSTBLogs")

assert memory_during > 0, "Process should use memory while running"

Copilot · 2026-02-03T09:08:49Z

test/functional-tests/tests/test_uploadstblogs_retry_logic.py

+
+        start_time = time.time()
+        result = run_uploadstblogs()
+        elapsed = time.time() - start_time


Variable elapsed is not used.

Suggested change

elapsed = time.time() - start_time

elapsed = time.time() - start_time

assert elapsed >= 0.0, "Execution time should be measured"

Copilot · 2026-02-03T09:08:53Z

test/functional-tests/tests/test_uploadstblogs_error_handling.py

+"""
+
+import pytest
+import time


Import of 'time' is not used.

Suggested change

import time

Copilot · 2026-02-03T09:08:53Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+"""
+
+import pytest
+import time


Import of 'time' is not used.

Suggested change

import time

Copilot · 2026-02-03T09:08:54Z

test/functional-tests/tests/test_uploadstblogs_security.py

+"""
+
+import pytest
+import time


Import of 'time' is not used.

Suggested change

import time

Copilot · 2026-02-03T09:08:54Z

test/functional-tests/tests/uploadstblogs_helper.py

+import os
+import time
+import re
+import json


Import of 'json' is not used.

Suggested change

import json

Copilot · 2026-02-03T09:08:54Z

test/functional-tests/tests/uploadstblogs_helper.py

+    """Get file size in bytes"""
+    try:
+        return os.path.getsize(filepath)
+    except:


Except block directly handles BaseException.

Suggested change

except:

except OSError:

DCM Agent 2.0.1 release

* Create uploadlogsnow.h --------- Co-authored-by: nhanasi <navihansi@gmail.com>

Copilot

Pull request overview

Copilot reviewed 70 out of 71 changed files in this pull request and generated 29 comments.

Comments suppressed due to low confidence (2)

uploadstblogs/src/cleanup_handler.c:455

cleanup_temp_dirs() takes a session parameter but doesn't use it, which will trigger -Wunused-parameter warnings under -Wall on some toolchains. Either remove the parameter or explicitly mark it unused (e.g., (void)session;).
uploadstblogs/src/file_operations.c:399
add_timestamp_to_files() no longer verifies entries are regular files before renaming (the previous stat()/S_ISREG check was removed). Despite the comment "Skip directories", this will also rename subdirectories if present, which can break directory layouts and subsequent file operations. Please restore a safe check (e.g., fstatat()/stat() and S_ISREG, or entry->d_type where reliable) so only regular files are timestamped.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T21:02:41Z

uploadstblogs/src/strategy_selector.c

    // Debug: Print all flag values
    fprintf(stderr, "DEBUG: early_checks() - rrd_flag=%d, dcm_flag=%d, trigger_type=%d\n", 
-            ctx->flags.rrd_flag, ctx->flags.dcm_flag, ctx->flags.trigger_type);
+            ctx->rrd_flag, ctx->dcm_flag, ctx->trigger_type);


early_checks() prints debug output directly to stderr via fprintf, which will always emit in production builds and bypass the project's logging controls. Please remove this or gate it behind a compile-time debug flag / RDK_LOG at an appropriate level.

Copilot · 2026-02-10T21:02:41Z

uploadstblogs/src/path_handler.c

+    // Prepare POST fields: filename first, then additional fields (following common utilities pattern)
+    char post_fields[512] = {0};
+
+    // Construct POST fields with full archive_filepath
+    if (md5_ptr && strlen(md5_ptr) > 0) {
+        snprintf(post_fields, sizeof(post_fields), "filename=%s&md5=%s", archive_filepath, md5_ptr);
+    } else {
+        snprintf(post_fields, sizeof(post_fields), "filename=%s", archive_filepath);
+    }


post_fields is built with snprintf(...) but the return value isn't checked. If archive_filepath is long, this can silently truncate the POST body (e.g., cut off md5=), causing hard-to-debug upload failures. Please check the snprintf return value and fail the upload (or log and return UPLOADSTB_FAILED) when truncation occurs.

Copilot · 2026-02-10T21:02:41Z

uploadstblogs/src/uploadlogsnow.c

+    while ((entry = readdir(dir)) != NULL) {
+        // Skip . and ..
+        if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) {
+            continue;
+        }
+
+        // Skip excluded files/directories
+        if (should_exclude_file(entry->d_name)) {
+            RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, 
+                    "[%s:%d] Excluding file: %s\n", 
+                    __FUNCTION__, __LINE__, entry->d_name);
+            continue;
+        }
+
+        // Construct full paths
+        char src_file[MAX_PATH_LENGTH];
+        char dest_file[MAX_PATH_LENGTH];
+
+        // Check if paths would fit to prevent truncation
+        size_t src_len = strlen(src_path) + 1 + strlen(entry->d_name) + 1;
+        size_t dest_len = strlen(dest_path) + 1 + strlen(entry->d_name) + 1;
+
+        if (src_len > sizeof(src_file) || dest_len > sizeof(dest_file)) {
+            RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
+                    "[%s:%d] Path too long, skipping: %s\n", 
+                    __FUNCTION__, __LINE__, entry->d_name);
+            continue;
+        }
+
+        int src_ret = snprintf(src_file, sizeof(src_file), "%s/%s", src_path, entry->d_name);
+        int dest_ret = snprintf(dest_file, sizeof(dest_file), "%s/%s", dest_path, entry->d_name);
+
+        // Additional safety check for snprintf truncation
+        if (src_ret < 0 || src_ret >= (int)sizeof(src_file) || 
+            dest_ret < 0 || dest_ret >= (int)sizeof(dest_file)) {
+            RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
+                    "[%s:%d] Path formatting failed, skipping: %s\n", 
+                    __FUNCTION__, __LINE__, entry->d_name);
+            continue;
+        }
+
+        // Use file operations utility for copy
+        if (copy_file(src_file, dest_file)) {
+            copied_count++;
+            RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, 
+                    "[%s:%d] Copied: %s\n", __FUNCTION__, __LINE__, entry->d_name);
+        } else {
+            RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
+                    "[%s:%d] Failed to copy: %s\n", 
+                    __FUNCTION__, __LINE__, entry->d_name);
+        }


copy_files_to_dcm_path() uses copy_file() on every readdir() entry, but it never checks whether the entry is a directory. If $LOG_PATH contains subdirectories (common for log trees), those directories will not be copied (and may spam warnings), which diverges from typical cp -rf $LOG_PATH/* script behavior and can omit logs. Please stat()/d_type filter to regular files, or implement recursive directory copy if directories are expected.

Copilot · 2026-02-10T21:02:42Z

uploadstblogs/src/cleanup_handler.c

+    char filepath[512];
+    int result = 0;
+
+    while ((entry = readdir(dir)) != NULL) {
+        if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) {
+            continue;
+        }
+
+        snprintf(filepath, sizeof(filepath), "%s/%s", path, entry->d_name);
+
+        // Try as directory first, then as file (avoids TOCTOU race)
+        result = remove_directory_recursive(filepath);
+        if (result != 0) {
+            // If directory removal failed, try as regular file
+            result = unlink(filepath);
+        }


Several snprintf(...) calls build paths into fixed 512-byte buffers (e.g., filepath/fullpath) without checking for truncation. If truncation occurs, recursive deletion/cleanup may operate on an unintended path fragment or fail in confusing ways. Please check snprintf return values and skip/abort the operation when the path doesn't fit (and log the full intended path length).

Copilot · 2026-02-10T21:02:42Z

configure.ac

+			case "${enableval}" in
+        	 yes) BREAKPAD_CFLAGS="-DINCLUDE_BREAKPAD"
+                    BREAKPAD_LFLAGS="-lbreakpadwrapper";;
+        	 no) AC_MSG_ERROR([breakpad is disabled]) ;;


The new --enable-breakpad option errors out when explicitly set to no (AC_MSG_ERROR([breakpad is disabled])). That makes ./configure --enable-breakpad=no fail unexpectedly; it should behave the same as omitting the flag (breakpad disabled). Please treat no as a valid disable case and only error on unrecognized values.

Suggested change

no) AC_MSG_ERROR([breakpad is disabled]) ;;

no) ;;

Copilot · 2026-02-10T21:02:47Z

test/functional-tests/tests/test_uploadLogsNow.py

+        for path in temp_paths:
+            try:
+                subprocess.run(f"rm -rf {path}", shell=True)
+            except:


Except block directly handles BaseException.

Suggested change

except:

except Exception:

Copilot · 2026-02-10T21:02:47Z

test/functional-tests/tests/test_uploadLogsNow.py

+                with open(filepath, 'w') as f:
+                    f.write(content)
+                created_files.append(filepath)
+            except:


Except block directly handles BaseException.

Copilot · 2026-02-10T21:02:47Z

test/functional-tests/tests/uploadstblogs_helper.py

+            for chunk in iter(lambda: f.read(4096), b""):
+                md5_hash.update(chunk)
+        return md5_hash.hexdigest()
+    except:


Except block directly handles BaseException.

Suggested change

except:

except Exception:

Copilot · 2026-02-10T21:02:48Z

test/functional-tests/tests/test_uploadLogsNow.py

+            except:
+                pass


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except:

pass

except Exception as e:

# Best-effort cleanup: log failure but do not interrupt tests

print(f"Failed to remove temporary path '{path}': {e}")

Copilot · 2026-02-10T21:02:48Z

test/functional-tests/tests/test_uploadLogsNow.py

+            except Exception:
+                pass


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

pass

except Exception as e:

print(f"Error checking temporary archive location {location}: {e}")

Copilot

Pull request overview

Copilot reviewed 70 out of 71 changed files in this pull request and generated 15 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T11:16:46Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+    void TearDown() override {
+        // Clean up test directory if it was created
+        if (!test_log_dir.empty()) {
+            std::string cleanup_cmd = "rm -rf " + test_log_dir;
+            system(cleanup_cmd.c_str());
+        }


TearDown() builds a shell command and calls system("rm -rf " + test_log_dir). Even though the path is currently derived from PID, using system() for cleanup is brittle and can become unsafe if the directory name ever includes unexpected characters. Prefer using direct APIs (e.g., std::filesystem::remove_all or nftw/remove_directory) to delete the temp directory without invoking a shell.

Copilot · 2026-02-11T11:16:47Z

uploadstblogs/src/path_handler.c

        static char clean_path[512];
        strncpy(clean_path, path_start, path_len);
        clean_path[path_len] = '\0';
        path_part = clean_path;
    }


attempt_proxy_fallback() uses a static char clean_path[512] buffer to hold the S3 path without query params. This introduces shared mutable state and breaks the thread-safety guarantee advertised for the public API (multiple concurrent uploads could race and corrupt the URL). Use a stack buffer (or write into proxy_url directly) instead of a function-static buffer.

Copilot · 2026-02-11T11:16:47Z

dcm_parseconf.c

    DCMInfo("DCM_DIFD_CRON: %s\n", pDifdCron);

    if(uploadCheck == 1 && pdcmSetHandle->bRebootFlag == 0) {
-        snprintf(pExBuff, EXECMD_BUFF_SIZE, "nice -n 19 /bin/busybox sh %s/uploadSTBLogs.sh %s 1 1 1 %s %s &",
-                                                 pRDKPath, DCM_LOG_TFTP, pUploadprtl, pUploadURL);
-        dcmUtilsSysCmdExec(pExBuff);
+        DCMInfo("Triggering log upload with reboot flag via library API\n");
+        UploadSTBLogsParams params = {
+            .flag = 1,
+            .dcm_flag = 1,
+            .upload_on_reboot = true,
+            .upload_protocol = pUploadprtl,
+            .upload_http_link = pUploadURL,
+            .trigger_type = TRIGGER_REBOOT,
+            .rrd_flag = false,
+            .rrd_file = NULL
+        };
+#ifndef GTEST_ENABLE
+        int result = uploadstblogs_run(&params);
+        if (result != 0) {
+            DCMError("Log upload (reboot=true) failed: %d\n", result);
+        }
+#endif
    }
    else if (uploadCheck == 0 && pdcmSetHandle->bRebootFlag == 0) {
-        snprintf(pExBuff, EXECMD_BUFF_SIZE, "nice -n 19 /bin/busybox sh %s/uploadSTBLogs.sh %s 1 1 0 %s %s &",
-                                                 pRDKPath, DCM_LOG_TFTP, pUploadprtl, pUploadURL);
-        dcmUtilsSysCmdExec(pExBuff);
+        DCMInfo("Triggering log upload without reboot flag via library API\n");
+        UploadSTBLogsParams params = {
+            .flag = 1,
+            .dcm_flag = 1,
+            .upload_on_reboot = false,
+            .upload_protocol = pUploadprtl,
+            .upload_http_link = pUploadURL,
+            .trigger_type = TRIGGER_SCHEDULED,
+            .rrd_flag = false,
+            .rrd_file = NULL
+        };
+#ifndef GTEST_ENABLE
+        int result = uploadstblogs_run(&params);
+        if (result != 0) {
+            DCMError("Log upload (reboot=false) failed: %d\n", result);
+        }
+#endif
    }


Similar to dcm.c, these code paths previously executed uploadSTBLogs.sh ... & asynchronously. Calling uploadstblogs_run() inline during config parsing can block the DCM agent flow while uploads run (including retries/timeouts), which is a significant behavioral change. Consider running the upload in a detached worker (thread/process) or otherwise ensuring this call cannot stall critical DCM operations.

Copilot · 2026-02-11T11:16:47Z

configure.ac

+        	 no) AC_MSG_ERROR([breakpad is disabled]) ;;
+        	 *) AC_MSG_ERROR([bad value ${enableval} for --enable-breakpad]) ;;
+        	 esac
+        	 ],
+    	    [echo "breakpad is disabled"])


The new --enable-breakpad option errors out when explicitly disabled (--enable-breakpad=no) due to AC_MSG_ERROR([breakpad is disabled]). That makes configure fail for a valid value. Also BREAKPAD_CFLAGS/BREAKPAD_LFLAGS are set but never AC_SUBST’d or added to any *_CFLAGS/LDFLAGS, so enabling breakpad currently has no build effect. Update the option handling to treat no as a normal path (no error) and wire the flags into the build via AC_SUBST/Makefile usage.

Suggested change

no) AC_MSG_ERROR([breakpad is disabled]) ;;

*) AC_MSG_ERROR([bad value ${enableval} for --enable-breakpad]) ;;

esac

],

[echo "breakpad is disabled"])

no) # explicitly disabled: leave BREAKPAD_CFLAGS/BREAKPAD_LFLAGS at their defaults

;;

*) AC_MSG_ERROR([bad value ${enableval} for --enable-breakpad]) ;;

esac

],

[echo "breakpad is disabled"])

AC_SUBST(BREAKPAD_CFLAGS)

AC_SUBST(BREAKPAD_LFLAGS)

Copilot · 2026-02-11T11:16:47Z

dcm.c

+        DCMInfo("\nStart log upload via library API\n");
+
+        // Call uploadstblogs library API instead of shell script
+        UploadSTBLogsParams params = {
+            .flag = 0,
+            .dcm_flag = 1,
+            .upload_on_reboot = false,
+            .upload_protocol = pPrctl,
+            .upload_http_link = pURL,
+            .trigger_type = TRIGGER_SCHEDULED,
+            .rrd_flag = false,
+            .rrd_file = NULL
+        };
+#ifndef GTEST_ENABLE
+        int result = uploadstblogs_run(&params);
+        if (result != 0) {
+            DCMError("Log upload failed with error code: %d\n", result);
+        } else {
+            DCMInfo("Log upload completed successfully\n");
+        }
+#endif


This scheduler callback previously spawned the log upload script asynchronously (background &). It now calls uploadstblogs_run() synchronously, which can block the scheduler/job execution thread for the entire upload duration (network retries, archive creation, etc.), changing runtime behavior and potentially delaying other scheduled jobs. Consider restoring async execution (thread/fork) or explicitly documenting/handling the blocking behavior (timeouts, watchdog, etc.).

Copilot · 2026-02-11T11:16:49Z

test/functional-tests/tests/test_uploadstblogs_security.py

+
+        create_test_log_files(count=1)
+
+        result = run_uploadstblogs()


Variable result is not used.

Suggested change

result = run_uploadstblogs()

run_uploadstblogs()

Copilot · 2026-02-11T11:16:49Z

test/functional-tests/tests/test_uploadstblogs_upload_strategies.py

+        result = run_uploadstblogs(args)
+
+        # Check for telemetry
+        telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker")


Variable telemetry_logs is not used.

Suggested change

telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker")

grep_uploadstb_logs_regex(r"telemetry|marker")

Copilot · 2026-02-11T11:16:49Z

test/functional-tests/tests/test_uploadstblogs_upload_strategies.py

+        result = run_uploadstblogs(args)
+
+        # Check for previous log collection
+        prev_logs = grep_uploadstb_logs_regex(r"previous|PreviousLogs")


Variable prev_logs is not used.

Suggested change

prev_logs = grep_uploadstb_logs_regex(r"previous|PreviousLogs")

grep_uploadstb_logs_regex(r"previous|PreviousLogs")

Copilot · 2026-02-11T11:16:49Z

test/functional-tests/tests/test_uploadstblogs_upload_strategies.py

+        result = run_uploadstblogs(args)
+
+        # Check for log collection
+        collection_logs = grep_uploadstb_logs_regex(r"collect|archive|DCM")


Variable collection_logs is not used.

Suggested change

collection_logs = grep_uploadstb_logs_regex(r"collect|archive|DCM")

grep_uploadstb_logs_regex(r"collect|archive|DCM")

Copilot · 2026-02-11T11:16:50Z

test/functional-tests/tests/test_uploadstblogs_normal_upload.py

+from uploadstblogs_helper import *
+from helper_functions import *


Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

Suggested change

from uploadstblogs_helper import *

from helper_functions import *

import subprocess

from uploadstblogs_helper import *

from helper_functions import grep_uploadstb_logs, grep_uploadstb_logs_regex

DCM Agent release for 2.0.3

shibu-kv and others added 3 commits February 10, 2025 06:57

Merge branch 'release/1.1.0'

b12ff0c

Merge branch 'release/1.2.0'

0397413

Copilot AI review requested due to automatic review settings January 9, 2026 05:09

Abhinavpv28 requested a review from a team as a code owner January 9, 2026 05:09

Copilot started reviewing on behalf of Abhinavpv28 January 9, 2026 05:10 View session

Copilot AI reviewed Jan 9, 2026

View reviewed changes

Abhinavpv28 and others added 2 commits January 12, 2026 13:40

RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scrip…

ffd0993

…ts To C Implementation (#49) * Simplify some pieces of code over engineered by AI models. * Update run_uploadstblogs_l2.sh for L2 coverage --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>

Copilot AI review requested due to automatic review settings January 13, 2026 18:09

Copilot started reviewing on behalf of shibu-kv January 13, 2026 18:09 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

Abhinavpv28 added 2 commits January 14, 2026 08:57

Enable breakpad support for analyzing any crashes

d68ca8c

Copilot AI review requested due to automatic review settings January 21, 2026 18:58

Copilot started reviewing on behalf of shibu-kv January 21, 2026 18:59 View session

Changelog updates for 2.0.0 release

6e7c40d

Copilot AI reviewed Jan 21, 2026

View reviewed changes

shibu-kv and others added 3 commits January 21, 2026 11:18

Merge branch 'release/2.0.0'

99acb75

Merge branch 'release/2.0.0' into develop

8696fdc

RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scrip…

52ad7e1

…ts To C Implementation (#60)

Copilot AI review requested due to automatic review settings February 3, 2026 08:55

Copilot started reviewing on behalf of tdeva14 February 3, 2026 08:55 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

naveenkumarhanasi and others added 4 commits February 3, 2026 19:04

DCM Agent 2.0.1 release changelog updates

c4a4a53

Merge branch 'release/2.0.1'

6af7b08

Merge tag '2.0.1' into develop

835a632

DCM Agent 2.0.1 release

RDK-59919 : [RDKE] Port Ops Support Upload Scripts to Source code (#59)

9a67596

* Create uploadlogsnow.h --------- Co-authored-by: nhanasi <navihansi@gmail.com>

Copilot AI review requested due to automatic review settings February 10, 2026 20:53

Copilot started reviewing on behalf of shibu-kv February 10, 2026 20:53 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

shibu-kv and others added 4 commits February 10, 2026 13:04

Changelog updates for 2.0.2 release

24893c3

Merge branch 'release/2.0.2'

0c95175

Merge branch 'release/2.0.2' into develop

f6aea55

Update context_manager.c (#73)

6097556

Copilot AI review requested due to automatic review settings February 11, 2026 11:06

Copilot started reviewing on behalf of tdeva14 February 11, 2026 11:06 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

naveenkumarhanasi added 3 commits February 11, 2026 16:45

DCM Agent 2.0.3 release changelog updates

45018b7

Merge branch 'release/2.0.3'

4489cc7

Merge tag '2.0.3' into develop

86c4755

DCM Agent release for 2.0.3

		result = subprocess.run(f"grep '{marker}' /opt/logs/telemetry*.log 2>/dev/null",
		shell=True, capture_output=True, text=True)

-    result = subprocess.run(f"grep -c '{pattern}' {log_file} 2>/dev/null || echo 0",
-                          shell=True, capture_output=True, text=True)
-    return int(result.stdout.strip())
+    try:
+        count = 0
+        with open(log_file, "r", encoding="utf-8", errors="ignore") as f:
+            for line in f:
+                if pattern in line:
+                    count += 1
+        return count
+    except (FileNotFoundError, PermissionError, OSError):
+        return 0

	assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"
	assert len(archive_logs) > 0, f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"

		@@ -0,0 +1,70 @@
		####################################################################################
		# If not stated otherwise in this file or this component's Licenses

	error_logs = grep_uploadstb_logs_regex(r"ERROR\|error\|fail.*properties\|invalid")
	error_logs = grep_uploadstb_logs_regex(r"ERROR\|error\|fail.*properties\|invalid")
	assert len(error_logs) > 0, "Configuration error should be logged for corrupted device properties"

	logs = grep_uploadstb_logs_regex(r"no.*file\|empty\|nothing to upload")
	grep_uploadstb_logs_regex(r"no.*file\|empty\|nothing to upload")

	upload_logs = grep_uploadstb_logs_regex(r"upload.*success\|uploading\|HTTP")
	upload_logs = grep_uploadstb_logs_regex(r"upload.*success\|uploading\|HTTP")
	assert len(upload_logs) == 0, "Should not log upload attempts without files"

	error_logs = grep_uploadstb_logs_regex(r"ERROR\|error\|fail.*properties\|invalid")
	grep_uploadstb_logs_regex(r"ERROR\|error\|fail.*properties\|invalid")

	logs = grep_uploadstb_logs_regex(r"size\|large\|limit\|truncate")
	grep_uploadstb_logs_regex(r"size\|large\|limit\|truncate")

		# Check for compression/archive activity
		compression_logs = grep_uploadstb_logs_regex(r"compress\|archive\|tgz")

	event_logs = grep_uploadstb_logs_regex(r"event\|publish\|success")
	grep_uploadstb_logs_regex(r"event\|publish\|success")

	logs = grep_uploadstb_logs_regex(r"strategy\|STRAT_\|upload.*type")
	grep_uploadstb_logs_regex(r"strategy\|STRAT_\|upload.*type")

	size_logs = grep_uploadstb_logs_regex(r"size.*limit\|too large\|exceed")
	grep_uploadstb_logs_regex(r"size.*limit\|too large\|exceed")

	assert len(archive_logs) > 0, "Upload Process should complete and succeed"
	assert len(upload_logs) > 0, "Upload Process should complete and succeed"

	memory_during = check_memory_usage("uploadSTBLogs")
	memory_during = check_memory_usage("uploadSTBLogs")
	assert memory_during > 0, "Process should use memory while running"

	elapsed = time.time() - start_time
	elapsed = time.time() - start_time
	assert elapsed >= 0.0, "Execution time should be measured"

-            except:
-                pass
+            except Exception as e:
+                # Best-effort cleanup: log failure but do not interrupt tests
+                print(f"Failed to remove temporary path '{path}': {e}")

	telemetry_logs = grep_uploadstb_logs_regex(r"telemetry\|marker")
	grep_uploadstb_logs_regex(r"telemetry\|marker")

	prev_logs = grep_uploadstb_logs_regex(r"previous\|PreviousLogs")
	grep_uploadstb_logs_regex(r"previous\|PreviousLogs")

	collection_logs = grep_uploadstb_logs_regex(r"collect\|archive\|DCM")
	grep_uploadstb_logs_regex(r"collect\|archive\|DCM")

Rebase #48

Are you sure you want to change the base?

Rebase #48

Conversation

Abhinavpv28 commented Jan 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

		from uploadstblogs_helper import *
		from helper_functions import *