Feature/l2check #70

Abhinavpv28 · 2026-02-10T10:14:53Z

No description provided.

Update and rename uploadlogsnow.h to uploadlogsnow.c Create uploadlogsnow.h Update uploadstblogs.c Update uploadstblogs_types.h Add uploadlogsnow.c to Makefile sources Update uploadstblogs.c Update uploadlogsnow.c Update file_operations.c Update file_operations.h Update uploadlogsnow.c Update uploadlogsnow.c Update uploadlogsnow.c Update uploadlogsnow.c Update Makefile.am Update Makefile.am Add unit tests for uploadlogsnow functionality Update unit_test.sh Update uploadlogsnow_gtest.cpp Update Makefile.am Refactor remove_directory function to return bool Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scripts 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. Changelog updates for 2.0.0 release Update file_operations.c Update uploadlogsnow.c Update uploadlogsnow.c Update uploadlogsnow.c Update uploadlogsnow_gtest.cpp Update uploadlogsnow_gtest.cpp Update uploadlogsnow_gtest.cpp Update uploadstblogs/unittest/uploadlogsnow_gtest.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update uploadstblogs/src/uploadstblogs.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update uploadstblogs.c Apply suggestion from @Copilot 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> Log message for no files to upload Added logging for cases when no files are found to upload. Update context_manager.c Change greeting from 'Hello World' to 'Goodbye World' Update cov_build.sh to clone rdk_logger Remove rdk_logger directory and clone it from GitHub. Update cov_build.sh Update cov_build.sh Update context_manager.c Update context_manager.c Update context_manager.c L1fix L1 fix L1 fix L1 fix Update context_manager.c Update event_manager.c Update uploadlogsnow_gtest.cpp Remove OS compatibility code from uploadlogsnow_gtest.cpp Removed compatibility code for Windows/Linux builds in uploadlogsnow_gtest.cpp. Update uploadstblogs/src/context_manager.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update unit_test.sh Update uploadstblogs/src/uploadstblogs.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update context_manager.c Update unit_test.sh Refactor include statements in uploadlogsnow.c Updated include paths for better accessibility. Update event_manager.c Update context_manager.c Update context_manager.c Update context_manager.c Update context_manager.c Update uploadlogsnow_gtest.cpp Update uploadlogsnow_gtest.cpp Update uploadlogsnow_gtest.cpp Update uploadstblogs/unittest/uploadlogsnow_gtest.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update uploadstblogs/src/context_manager.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update context_manager.c Create test_uploadLogsNow.py Refactor subprocess calls for consistency Update uploadstblogs_helper.py Update run_uploadstblogs_l2.sh

Rebase

Copilot

Pull request overview

Adds an UploadLogsNow (“uploadlogsnow” CLI argument) execution path to the logupload/uploadstblogs binary, implementing the former shell-script workflow in C and integrating it into unit + functional test runs.

Changes:

Introduces execute_uploadlogsnow_workflow() and wires it into uploadstblogs_execute() when argv[1] == "uploadlogsnow".
Extends file utilities with add_timestamp_to_files_uploadlogsnow() and adds additional snprintf truncation checks in path-building code.
Adds new unit test target (uploadlogsnow_gtest) and a new functional test (test_uploadLogsNow.py) plus runner updates.

Reviewed changes

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

Show a summary per file

File	Description
`uploadstblogs/src/uploadlogsnow.c`	New C implementation of UploadLogsNow workflow (copy → timestamp → archive → upload → cleanup).
`uploadstblogs/include/uploadlogsnow.h`	Public header for UploadLogsNow workflow entry point.
`uploadstblogs/src/uploadstblogs.c`	Detects `uploadlogsnow` CLI arg and routes to custom workflow.
`uploadstblogs/include/uploadstblogs_types.h`	Adds status/temp dir constants and an `uploadlogsnow_mode` flag in `RuntimeContext`.
`uploadstblogs/src/event_manager.c`	Skips IARM events when running in UploadLogsNow mode.
`uploadstblogs/src/file_operations.c`	Adds UploadLogsNow timestamping variant; adds truncation checks; modifies timestamping logic.
`uploadstblogs/include/file_operations.h`	Declares `add_timestamp_to_files_uploadlogsnow()`.
`uploadstblogs/src/context_manager.c`	Adds extended logger init and conditional `rdk_logger.h` include.
`uploadstblogs/src/Makefile.am`	Builds `uploadlogsnow.c` into the library.
`uploadstblogs/unittest/uploadlogsnow_gtest.cpp`	New gtest for UploadLogsNow workflow.
`uploadstblogs/unittest/Makefile.am`	Adds `uploadlogsnow_gtest` test binary target.
`test/functional-tests/tests/test_uploadLogsNow.py`	New functional test suite for UploadLogsNow.
`test/run_uploadstblogs_l2.sh`	Adds UploadLogsNow test step to L2 runner.
`unit_test.sh`	Adds UploadLogsNow unit test execution and pulls `rdk_logger` headers.
`cov_build.sh`	Pulls `rdk_logger` headers in coverage build.
`.github/workflows/code-coverage.yml`	Runs code coverage workflow on PRs to `main` and `develop`.
`test/functional-tests/tests/uploadstblogs_helper.py`	Minor whitespace change.

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

Copilot · 2026-02-10T10:24:03Z

test/functional-tests/tests/test_uploadLogsNow.py

+import pytest
+import time
+import subprocess as sp
+import os
+import tempfile
+import shutil
+from uploadstblogs_helper import *
+from helper_functions import *
+
+
+def run_uploadlogsnow():
+    """Execute uploadlogsnow using the specific binary command"""
+    cmd = "/usr/local/bin/logupload uploadlogsnow"
+    result = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=300)
+    return result


This file imports subprocess as "sp" but calls subprocess.run() in multiple places (run_uploadlogsnow, set_rfc_endpoint, get_rfc_endpoint, cleanup_dcm_temp_files, etc.). This will raise NameError and break the test suite. Either import subprocess (without alias) or consistently use sp.run().

Copilot · 2026-02-10T10:24:03Z

test/functional-tests/tests/test_uploadLogsNow.py

+        """Clean up DCM temporary files and directories"""
+        temp_paths = [
+            "/tmp/DCM",
+            "/tmp/loguploadstatus.txt",


The cleanup/status checks look for /tmp/loguploadstatus.txt and /opt/logs/loguploadstatus.txt, but the C implementation writes STATUS_FILE as /opt/loguploadstatus.txt. This mismatch means the functional test may miss the status output. Align the test paths with the implementation (or update STATUS_FILE if /tmp was intended).

Suggested change

"/tmp/loguploadstatus.txt",

"/opt/loguploadstatus.txt",

Copilot · 2026-02-10T10:24:04Z

test/functional-tests/tests/test_uploadLogsNow.py

+        # Check for context initialization logs
+        init_logs = grep_uploadstb_logs_regex(r"Context.*initializ|initializ.*context|UploadLogsNow.*start")
+        assert len(init_logs) >= 0, "Should find context initialization evidence"
+
+        # Check for device properties loading
+        device_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE|device.*propert|loading.*propert")
+        assert len(device_logs) >= 0, "Should load device properties"
+
+        # Check for path validation/setup
+        path_logs = grep_uploadstb_logs_regex(r"LOG_PATH|DCM_LOG_PATH|path.*valid|directory.*creat")
+        assert len(path_logs) >= 0, "Should validate and setup paths"
+
+        # Check for RFC endpoint configuration reading
+        rfc_logs = grep_uploadstb_logs_regex(r"RFC|LogUploadEndpoint|endpoint.*config")
+        assert len(rfc_logs) >= 0, "Should read RFC configuration"
+


Several assertions in this test use assert len(<logs>) >= 0, which is always true and therefore doesn't validate the expected behavior (it will never fail). Use a stricter condition (e.g., > 0) or assert on specific expected patterns/return codes so the test meaningfully verifies UploadLogsNow behavior.

Copilot · 2026-02-10T10:24:04Z

uploadstblogs/src/file_operations.c

+
+    // Get current timestamp in script format: MM-DD-YY-HH-MMAM/PM-
+    time_t now = time(NULL);
+    struct tm* tm_info = localtime(&now);


add_timestamp_to_files_uploadlogsnow() calls localtime(&now) and then passes the result straight to strftime() without checking for NULL. Other parts of the codebase (e.g., archive_manager.c) explicitly check localtime() failure. Add a NULL check (or use localtime_r) to avoid a potential crash if localtime() fails.

Suggested change

struct tm* tm_info = localtime(&now);

struct tm* tm_info = localtime(&now);

if (tm_info == NULL) {

RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB,

"[%s:%d] localtime() failed while generating timestamp\n",

__FUNCTION__, __LINE__);

return -1;

}

Copilot · 2026-02-10T10:24:04Z

uploadstblogs/src/file_operations.c

    while ((entry = readdir(dir)) != NULL) {
        // Skip directories and special entries
        if (entry->d_name[0] == '.' || 
            strcmp(entry->d_name, "..") == 0 ||
            strncmp(entry->d_name, timestamp, strlen(timestamp)) == 0) {
            continue;
        }

-        char old_path[MAX_PATH_LENGTH];
-        char new_path[MAX_PATH_LENGTH];
+        char old_path[MAX_PATH_LENGTH] = "\0";
+        char new_path[MAX_PATH_LENGTH] = "\0";

-        snprintf(old_path, sizeof(old_path), "%s/%s", dir_path, entry->d_name);
-        snprintf(new_path, sizeof(new_path), "%s/%s%s", dir_path, timestamp, entry->d_name);
-
-        // Skip if not a regular file
-        struct stat st;
-        if (stat(old_path, &st) != 0 || !S_ISREG(st.st_mode)) {
+        int old_ret = snprintf(old_path, sizeof(old_path), "%s/%s", dir_path, entry->d_name);
+        int new_ret = snprintf(new_path, sizeof(new_path), "%s/%s%s", dir_path, timestamp, entry->d_name);
+
+        // Check for snprintf truncation
+        if (old_ret < 0 || old_ret >= (int)sizeof(old_path) ||
+            new_ret < 0 || new_ret >= (int)sizeof(new_path)) {
+            RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
+                    "[%s:%d] Path too long, skipping: %s\n", 
+                    __FUNCTION__, __LINE__, entry->d_name);
            continue;
        }

+        // Rename directly without pre-check to avoid TOCTOU issue
        if (rename(old_path, new_path) == 0) {
            RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, 


add_timestamp_to_files() no longer checks that each directory entry is a regular file before renaming. Since rename() works on directories too, this can unintentionally rename/move subdirectories under DCM_LOG_PATH / PREV_LOG_PATH (e.g., PreviousLogs), which can break later logic. Preserve the previous behavior by stat'ing (or fstatat) and skipping non-regular files/symlinks while still avoiding TOCTOU where possible.

Copilot · 2026-02-10T10:24:08Z

test/functional-tests/tests/test_uploadLogsNow.py

+import subprocess as sp
+import os
+import tempfile
+import shutil


Import of 'shutil' is not used.

Suggested change

import shutil

Copilot · 2026-02-10T10:24:08Z

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-10T10:24:08Z

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-10T10:24:08Z

test/functional-tests/tests/test_uploadLogsNow.py

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


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

Suggested change

except:

except Exception:

# Best-effort cleanup: ignore errors such as missing files or permission issues

Copilot · 2026-02-10T10:24:09Z

test/functional-tests/tests/test_uploadLogsNow.py

+                        if filename.endswith(('.tar.gz', '.tgz')):
+                            print(f"Found archive file: {location}/{filename}")
+                            return True
+            except Exception:


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

Suggested change

except Exception:

except Exception:

# Best-effort scan: ignore filesystem errors when checking temp locations

Comment out debug logging setup line in the script.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 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-10T11:32:43Z

test/functional-tests/tests/test_uploadLogsNow.py

+        """Check upload status files for success indicators"""
+        status_files = [
+            "/tmp/loguploadstatus.txt",
+            "/opt/logs/loguploadstatus.txt"


check_upload_status_success() looks for status files under /tmp and /opt/logs, but the implementation writes STATUS_FILE (currently /opt/loguploadstatus.txt). Align the test’s expected status file path with the code (or make STATUS_FILE configurable for tests) so this verification is meaningful.

Suggested change

"/opt/logs/loguploadstatus.txt"

"/opt/logs/loguploadstatus.txt",

"/opt/loguploadstatus.txt",

Copilot · 2026-02-10T11:32:43Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+        g_execute_upload_cycle_call_count = 0;
+
+        // Create a temporary test directory
+        test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());


SetUp() sets ctx.log_path to a unique /tmp directory, but the directory is never created. Since uploadlogsnow.c uses real opendir/readdir, copy_files_to_dcm_path will fail with ENOENT and tests may pass for the wrong reason. Create test_log_dir on disk (and add at least one dummy file) in SetUp().

Suggested change

test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());

test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());

// Ensure the directory exists on disk

if (mkdir(test_log_dir.c_str(), 0700) != 0 && errno != EEXIST) {

// If directory creation fails for reasons other than "already exists",

// tests may not behave as expected, but we proceed to avoid crashing.

}

// Create at least one dummy log file so opendir/readdir can find entries

std::string dummy_file_path = test_log_dir + "/dummy.log";

FILE* dummy_file = fopen(dummy_file_path.c_str(), "w");

if (dummy_file != nullptr) {

fputs("dummy log content\n", dummy_file);

fclose(dummy_file);

}

Copilot · 2026-02-10T11:32:44Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+int copy_files_to_dcm_path(const char* src_path, const char* dest_path) {
+    g_copy_files_to_dcm_path_call_count++;
+    if (!src_path || !dest_path) return -1;
+    if (g_copy_file_should_fail) return -1;
+    return g_copy_files_return_count;


This test provides a mock copy_files_to_dcm_path(), but in uploadlogsnow.c the implementation is static, so this symbol is never used. As a result, g_copy_files_return_count and g_copy_files_to_dcm_path_call_count don’t affect the workflow. Consider making copy_files_to_dcm_path non-static (and exposing it for mocking) or injecting/mocking directory enumeration another way.

Copilot · 2026-02-10T11:32:44Z

uploadstblogs/src/uploadlogsnow.c

+    FILE* fp = fopen(STATUS_FILE, "w");
+    if (!fp) {
+        RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, 
+                "[%s:%d] Failed to open status file: %s\n", 
+                __FUNCTION__, __LINE__, STATUS_FILE);


write_upload_status() uses fopen(STATUS_FILE, "w"), which follows symlinks. If this runs with elevated privileges, a symlink at STATUS_FILE could redirect writes to an unintended target. Consider open() with O_NOFOLLOW|O_CLOEXEC (and a safe mode) before writing.

Copilot · 2026-02-10T11:32:44Z

uploadstblogs/src/uploadlogsnow.c

+cleanup:
+    // Clean up DCM_LOG_PATH
+    if (!remove_directory(dcm_log_path)) {
+        RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
+                "[%s:%d] Failed to cleanup DCM_LOG_PATH\n", __FUNCTION__, __LINE__);
+    } else {


Cleanup unconditionally calls remove_directory(dcm_log_path). If dcm_log_path can ever point outside a known-safe temp location, this is a dangerous delete. Consider validating the path (e.g., must be under /tmp) and/or only removing a per-run subdirectory created by this workflow.

Copilot · 2026-02-10T11:32:46Z

uploadstblogs/src/file_operations.c

+            strncmp(entry->d_name, timestamp, strlen(timestamp)) == 0) {
+            continue;
+        }
+


This loop says "Skip directories" but the condition only checks the name and timestamp prefix; it doesn’t verify file type. add_timestamp_to_files_uploadlogsnow() may rename directories or symlinks in dir_path. Add a file-type check (e.g., fstatat/lstat with AT_SYMLINK_NOFOLLOW and require S_ISREG) before attempting rename().

Suggested change

/* Ensure we only operate on regular files (do not follow symlinks) */

struct stat st;

if (fstatat(dirfd(dir), entry->d_name, &st, AT_SYMLINK_NOFOLLOW) != 0) {

RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB,

"[%s:%d] Failed to stat entry: %s (errno=%d)\n",

__FUNCTION__, __LINE__, entry->d_name, errno);

error_count++;

continue;

}

if (!S_ISREG(st.st_mode)) {

/* Skip non-regular files such as directories, symlinks, sockets, etc. */

continue;

}

Copilot · 2026-02-10T11:32:46Z

uploadstblogs/src/file_operations.c

+        // Rename directly without pre-check to avoid TOCTOU issue
+        if (rename(old_path, new_path) == 0) {
+            success_count++;
+        } else {
+            RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, 


add_timestamp_to_files_uploadlogsnow() calls rename() without validating the entry is a regular file. If a directory/symlink is present, this will rename it as well. Add a file-type check before renaming to avoid unintended directory moves and potential symlink abuse.

Copilot · 2026-02-10T11:32:46Z

test/functional-tests/tests/test_uploadLogsNow.py

+        init_logs = grep_uploadstb_logs_regex(r"Context.*initializ|initializ.*context|UploadLogsNow.*start")
+        assert len(init_logs) >= 0, "Should find context initialization evidence"
+
+        # Check for device properties loading
+        device_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE|device.*propert|loading.*propert")


These assertions (len(...) >= 0) are always true and won’t catch regressions in initialization/path/RFC behavior. Replace with meaningful expectations (e.g., > 0 for specific patterns or assert on side effects like status file contents) so failures are detected.

Copilot · 2026-02-10T11:32:46Z

test/functional-tests/tests/test_uploadLogsNow.py

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


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

Suggested change

subprocess.run(f"rm -rf {path}", shell=True)

except:

pass

subprocess.run(f"rm -rf {path}", shell=True, check=True)

except subprocess.CalledProcessError as e:

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

print(f"Warning: failed to remove temp path '{path}': {e}")

Copilot · 2026-02-10T11:32:47Z

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 18 out of 18 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

uploadstblogs/src/file_operations.c:399

add_timestamp_to_files() no longer checks that the entry is a regular file before renaming. Since rename() works on directories too, this can end up timestamping directories under the log path (e.g., PreviousLogs), which would break downstream logic that expects those directory names. Consider restoring a file-type check (e.g., lstat/stat + S_ISREG) before renaming, while still avoiding TOCTOU where possible.

    while ((entry = readdir(dir)) != NULL) {
        // Skip directories and special entries
        if (entry->d_name[0] == '.' || 
            strcmp(entry->d_name, "..") == 0 ||
            strncmp(entry->d_name, timestamp, strlen(timestamp)) == 0) {
            continue;
        }

        char old_path[MAX_PATH_LENGTH] = "\0";
        char new_path[MAX_PATH_LENGTH] = "\0";
        
        int old_ret = snprintf(old_path, sizeof(old_path), "%s/%s", dir_path, entry->d_name);
        int new_ret = snprintf(new_path, sizeof(new_path), "%s/%s%s", dir_path, timestamp, entry->d_name);
        
        // Check for snprintf truncation
        if (old_ret < 0 || old_ret >= (int)sizeof(old_path) ||
            new_ret < 0 || new_ret >= (int)sizeof(new_path)) {
            RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB, 
                    "[%s:%d] Path too long, skipping: %s\n", 
                    __FUNCTION__, __LINE__, entry->d_name);
            continue;
        }

        // Rename directly without pre-check to avoid TOCTOU issue
        if (rename(old_path, new_path) == 0) {
            RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, 
                    "[%s:%d] Renamed: %s -> %s\n", 
                    __FUNCTION__, __LINE__, entry->d_name, new_path);
            success_count++;
        } else {

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

Copilot · 2026-02-10T12:08:46Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+    // Reset and test copy failure
+    SetUp(); // Reset all mocks
+    g_copy_file_should_fail = true;
+    result = execute_uploadlogsnow_workflow(&ctx);
+    EXPECT_EQ(-1, result);
+
+    // Reset and test archive creation failure
+    SetUp(); // Reset all mocks
+    g_create_archive_should_fail = true;


Calling SetUp() manually inside a TEST_F bypasses the normal gtest fixture lifecycle (and skips TearDown() between resets), which can leak resources and make failures harder to diagnose. Prefer extracting the mock-state reset logic into a dedicated helper (e.g., ResetMocks()) and call that, or split these into separate TEST_F cases.

Copilot · 2026-02-10T12:08:47Z

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() uses system("rm -rf ...") to remove the temp directory. Even in tests, this is brittle and can be unsafe if the path ever changes (shell interpretation, spaces, etc.). Prefer removing the directory via filesystem APIs (e.g., std::filesystem::remove_all) or calling remove_directory() directly with a validated path.

Copilot · 2026-02-10T12:08:47Z

test/run_l2.sh


 mkdir -p "$RESULT_DIR"
-echo "LOG.RDK.DCM = ALL FATAL ERROR WARNING NOTICE INFO DEBUG" >> /etc/debug.ini
+echo "LOG.RDK.DEFAULT" >> /etc/debug.ini


This writes an invalid debug.ini entry ("LOG.RDK.DEFAULT" without a level assignment). If debug.ini parsing expects KEY = LEVELS, logging configuration may break for the rest of the L2 run. Match the format used elsewhere (e.g., "LOG.RDK.DEFAULT = INFO").

Suggested change

echo "LOG.RDK.DEFAULT" >> /etc/debug.ini

echo "LOG.RDK.DEFAULT = INFO" >> /etc/debug.ini

Copilot · 2026-02-10T12:08:47Z

uploadstblogs/src/uploadlogsnow.c

+    // Check if any files were copied
+    if (copied_files == 0) {
+        RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB, 
+                "[%s:%d] No files found to upload in directory: %s\n", 
+                __FUNCTION__, __LINE__, ctx->log_path);
+        write_upload_status("No files to upload");
+        ret = 0;  // Success, but no files to process
+        goto cleanup;


copy_files_to_dcm_path() only counts successful copies and ignores failures. If all copies fail (permissions, disk full, etc.), copied_files will be 0 and the workflow reports "No files to upload" and returns success, even though files may exist. Consider tracking whether the source directory had entries and/or whether any copy failed, and treat "all copies failed" as an error distinct from "directory is empty".

Copilot · 2026-02-10T12:08:48Z

test/functional-tests/tests/test_uploadLogsNow.py

+        assert len(init_logs) >= 0, "Should find context initialization evidence"
+
+        # Check for device properties loading
+        device_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE|device.*propert|loading.*propert")
+        assert len(device_logs) >= 0, "Should load device properties"
+
+        # Check for path validation/setup
+        path_logs = grep_uploadstb_logs_regex(r"LOG_PATH|DCM_LOG_PATH|path.*valid|directory.*creat")
+        assert len(path_logs) >= 0, "Should validate and setup paths"
+
+        # Check for RFC endpoint configuration reading
+        rfc_logs = grep_uploadstb_logs_regex(r"RFC|LogUploadEndpoint|endpoint.*config")
+        assert len(rfc_logs) >= 0, "Should read RFC configuration"


Assertions like assert len(init_logs) >= 0 will always pass, so these checks don't validate behavior and can hide regressions. Consider asserting > 0 (or a minimum expected count) when logs are required, or asserting on specific expected log lines/status file content to make the test meaningful.

Suggested change

assert len(init_logs) >= 0, "Should find context initialization evidence"

# Check for device properties loading

device_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE|device.*propert|loading.*propert")

assert len(device_logs) >= 0, "Should load device properties"

# Check for path validation/setup

path_logs = grep_uploadstb_logs_regex(r"LOG_PATH|DCM_LOG_PATH|path.*valid|directory.*creat")

assert len(path_logs) >= 0, "Should validate and setup paths"

# Check for RFC endpoint configuration reading

rfc_logs = grep_uploadstb_logs_regex(r"RFC|LogUploadEndpoint|endpoint.*config")

assert len(rfc_logs) >= 0, "Should read RFC configuration"

assert len(init_logs) > 0, "Should find context initialization evidence"

# Check for device properties loading

device_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE|device.*propert|loading.*propert")

assert len(device_logs) > 0, "Should load device properties"

# Check for path validation/setup

path_logs = grep_uploadstb_logs_regex(r"LOG_PATH|DCM_LOG_PATH|path.*valid|directory.*creat")

assert len(path_logs) > 0, "Should validate and setup paths"

# Check for RFC endpoint configuration reading

rfc_logs = grep_uploadstb_logs_regex(r"RFC|LogUploadEndpoint|endpoint.*config")

assert len(rfc_logs) > 0, "Should read RFC configuration"

Copilot · 2026-02-10T12:08:48Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+        // Create a temporary test directory
+        test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());
+
+        // Initialize test context with safe paths
+        memset(&ctx, 0, sizeof(ctx));
+        strncpy(ctx.log_path, test_log_dir.c_str(), sizeof(ctx.log_path) - 1);
+        strcpy(ctx.dcm_log_path, "");
+        ctx.uploadlogsnow_mode = true;
+    }


The test SetUp() only sets ctx.log_path to a /tmp path but never creates that directory or populates it with test files. Since the real uploadlogsnow.c implementation uses opendir/readdir on ctx->log_path, most tests will fail (or pass) due to an early copy failure and won't actually exercise later stages like create_archive() / file_exists() / execute_upload_cycle(). Consider creating the directory + a few dummy files in SetUp() (and cleaning them in TearDown()), or refactoring the production code to make directory traversal mockable.

Copilot · 2026-02-10T12:08:48Z

uploadstblogs/unittest/uploadlogsnow_gtest.cpp

+int copy_files_to_dcm_path(const char* src_path, const char* dest_path) {
+    g_copy_files_to_dcm_path_call_count++;
+    if (!src_path || !dest_path) return -1;
+    if (g_copy_file_should_fail) return -1;
+    return g_copy_files_return_count;
+}


This test defines a mock copy_files_to_dcm_path(), but in uploadlogsnow.c the function is declared static, so this mock will never be used. As a result, g_copy_files_return_count and g_copy_files_to_dcm_path_call_count won't reflect the workflow behavior. Consider either (1) making copy_files_to_dcm_path non-static and declaring it in a header for mocking, or (2) testing via real filesystem setup instead of trying to interpose this symbol.

Copilot · 2026-02-10T12:08:48Z

test/functional-tests/tests/test_uploadLogsNow.py

+import tempfile
+import shutil
+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 helper_functions import *

from helper_functions import remove_lock_file, cleanup_test_log_files, kill_uploadstblogs, clear_uploadstb_logs

Copilot · 2026-02-10T12:08:49Z

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 fail the test

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

Copilot · 2026-02-10T12:08:49Z

test/functional-tests/tests/test_uploadLogsNow.py

+                        if filename.endswith(('.tar.gz', '.tgz')):
+                            print(f"Found archive file: {location}/{filename}")
+                            return True
+            except Exception:


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

Suggested change

except Exception:

except Exception:

# Intentionally ignore errors accessing these temporary locations.

# Failure to read or list these paths should not cause the test to fail;

# other checks above already verify archive processing.

Comment out cloning and copying rdk_logger headers.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

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

Copilot · 2026-02-10T12:37:06Z

uploadstblogs/src/file_operations.c

+            continue;
+        }
+
+        // Rename directly without pre-check to avoid TOCTOU issue


add_timestamp_to_files_uploadlogsnow() renames every non-dot entry without verifying it is a regular file. This can rename directories/symlinks under the DCM temp folder and may break archive creation or cleanup. It should skip non-regular files (same approach as add_timestamp_to_files()).

Suggested change

// Rename directly without pre-check to avoid TOCTOU issue

// Only process regular files (skip directories, symlinks, etc.)

struct stat st;

if (lstat(old_path, &st) != 0) {

RDK_LOG(RDK_LOG_WARN, LOG_UPLOADSTB,

"[%s:%d] Failed to stat %s: %s\n",

__FUNCTION__, __LINE__, old_path, strerror(errno));

continue;

}

if (!S_ISREG(st.st_mode)) {

RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB,

"[%s:%d] Skipping non-regular file: %s\n",

__FUNCTION__, __LINE__, old_path);

continue;

}

Copilot · 2026-02-10T12:37:07Z

uploadstblogs/src/context_manager.c

+    if (rdk_logger_ext_init(&config) != RDK_SUCCESS) {
+        printf("UPLOADSTB : ERROR - Extended logger init failed\n");


init_context() initializes the logger via rdk_logger_ext_init(), but the local g_rdk_logger_enabled flag is never set and the failure path uses printf() instead of the existing logging/fallback pattern. If other logging macros depend on the enabled flag (as in other components), logs may silently downgrade/behave inconsistently. Consider either removing the unused flag entirely or setting it based on init success, and ensure build/config checks cover the rdk_logger_ext_init symbol (not just rdk_logger_init).

Suggested change

if (rdk_logger_ext_init(&config) != RDK_SUCCESS) {

printf("UPLOADSTB : ERROR - Extended logger init failed\n");

if (rdk_logger_ext_init(&config) == RDK_SUCCESS) {

g_rdk_logger_enabled = 1;

} else {

g_rdk_logger_enabled = 0;

fprintf(stderr, "UPLOADSTB : ERROR - Extended logger init failed\n");

Copilot · 2026-02-10T12:37:07Z

uploadstblogs/src/uploadstblogs.c

+    // Check for special "uploadlogsnow" parameter first
+    if (argc >= 2 && strcmp(argv[1], "uploadlogsnow") == 0) {
+        // Set UploadLogsNow-specific parameters
+        ctx->flag = 1;                          // Upload enabled
+        ctx->dcm_flag = 1;                      // Use DCM mode
+        ctx->upload_on_reboot = 1;              // Upload on reboot enabled
+        ctx->trigger_type = TRIGGER_ONDEMAND;   // ONDEMAND trigger (5)
+        ctx->rrd_flag = 0;                      // Not RRD upload
+        ctx->tls_enabled = false;               // Default to HTTP
+        ctx->uploadlogsnow_mode = true;         // Enable UploadLogsNow mode
+
+        RDK_LOG(RDK_LOG_DEBUG, "LOG.RDK.UPLOADSTBLOGS", "UploadLogsNow mode enabled\n");
+        return true;


In parse_args(), the UploadLogsNow branch logs to module string "LOG.RDK.UPLOADSTBLOGS" instead of the rest of the codebase’s LOG_UPLOADSTB ("LOG.RDK.UPLOADSTB"). This can make logs disappear depending on debug.ini configuration. Prefer using LOG_UPLOADSTB for consistency.

Copilot · 2026-02-10T12:37:07Z

test/functional-tests/tests/test_uploadLogsNow.py

+        result = run_uploadlogsnow()
+
+        # Check logs for endpoint usage
+        endpoint_logs = grep_uploadstb_logs_regex(r"mockxconf.*50058")


Variable endpoint_logs is not used.

Suggested change

endpoint_logs = grep_uploadstb_logs_regex(r"mockxconf.*50058")

grep_uploadstb_logs_regex(r"mockxconf.*50058")

Abhinavpv28 and others added 5 commits January 28, 2026 13:09

Merge pull request #61 from rdkcentral/develop

ac3d8fa

Rebase

Update code-coverage.yml

5048713

Update file_operations.c

8a83d88

Merge branch 'develop' into feature/RDK-59919_3

3ec76d5

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

Abhinavpv28 requested a review from a team as a code owner February 10, 2026 10:14

Copilot started reviewing on behalf of Abhinavpv28 February 10, 2026 10:15 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Update context_manager.c

f2f74e4

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

Copilot started reviewing on behalf of Abhinavpv28 February 10, 2026 11:23 View session

Abhinavpv28 added 2 commits February 10, 2026 16:59

Update run_l2.sh

9d22b01

Comment out debug logging in run_uploadstblogs_l2.sh

6c83b03

Comment out debug logging setup line in the script.

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Abhinavpv28 added 2 commits February 10, 2026 17:27

Update logging configuration in run_uploadstblogs_l2.sh

d4f925d

Update logging configuration in run_l2.sh

8293269

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

Copilot started reviewing on behalf of Abhinavpv28 February 10, 2026 11:58 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Disable rdk_logger header installation

9cfc829

Comment out cloning and copying rdk_logger headers.

Copilot AI review requested due to automatic review settings February 10, 2026 12:28

Copilot started reviewing on behalf of Abhinavpv28 February 10, 2026 12:29 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Update context_manager.c

c577df1

	except:
	except Exception:
	# Best-effort cleanup: ignore errors such as missing files or permission issues

	except Exception:
	except Exception:
	# Best-effort scan: ignore filesystem errors when checking temp locations

	"/opt/logs/loguploadstatus.txt"
	"/opt/logs/loguploadstatus.txt",
	"/opt/loguploadstatus.txt",

-        test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());
+        test_log_dir = std::string("/tmp/uploadlogsnow_test_") + std::to_string(getpid());
+        // Ensure the directory exists on disk
+        if (mkdir(test_log_dir.c_str(), 0700) != 0 && errno != EEXIST) {
+            // If directory creation fails for reasons other than "already exists",
+            // tests may not behave as expected, but we proceed to avoid crashing.
+        }
+        // Create at least one dummy log file so opendir/readdir can find entries
+        std::string dummy_file_path = test_log_dir + "/dummy.log";
+        FILE* dummy_file = fopen(dummy_file_path.c_str(), "w");
+        if (dummy_file != nullptr) {
+            fputs("dummy log content\n", dummy_file);
+            fclose(dummy_file);
+        }

+        /* Ensure we only operate on regular files (do not follow symlinks) */
+        struct stat st;
+        if (fstatat(dirfd(dir), entry->d_name, &st, AT_SYMLINK_NOFOLLOW) != 0) {
+            RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB,
+                    "[%s:%d] Failed to stat entry: %s (errno=%d)\n",
+                    __FUNCTION__, __LINE__, entry->d_name, errno);
+            error_count++;
+            continue;
+        }
+        if (!S_ISREG(st.st_mode)) {
+            /* Skip non-regular files such as directories, symlinks, sockets, etc. */
+            continue;
+        }

-                subprocess.run(f"rm -rf {path}", shell=True)
-            except:
-                pass
+                subprocess.run(f"rm -rf {path}", shell=True, check=True)
+            except subprocess.CalledProcessError as e:
+                # Best-effort cleanup: log failure but do not interrupt the tests
+                print(f"Warning: failed to remove temp path '{path}': {e}")

-            except Exception:
-                pass
+            except Exception as e:
+                print(f"Error checking temporary archive location {location}: {e}")

	echo "LOG.RDK.DEFAULT" >> /etc/debug.ini
	echo "LOG.RDK.DEFAULT = INFO" >> /etc/debug.ini

	from helper_functions import *
	from helper_functions import remove_lock_file, cleanup_test_log_files, kill_uploadstblogs, clear_uploadstb_logs

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

-            except Exception:
+            except Exception:
+                # Intentionally ignore errors accessing these temporary locations.
+                # Failure to read or list these paths should not cause the test to fail;
+                # other checks above already verify archive processing.

		if (rdk_logger_ext_init(&config) != RDK_SUCCESS) {
		printf("UPLOADSTB : ERROR - Extended logger init failed\n");

-    if (rdk_logger_ext_init(&config) != RDK_SUCCESS) {
-        printf("UPLOADSTB : ERROR - Extended logger init failed\n");
+    if (rdk_logger_ext_init(&config) == RDK_SUCCESS) {
+        g_rdk_logger_enabled = 1;
+    } else {
+        g_rdk_logger_enabled = 0;
+        fprintf(stderr, "UPLOADSTB : ERROR - Extended logger init failed\n");

	endpoint_logs = grep_uploadstb_logs_regex(r"mockxconf.*50058")
	grep_uploadstb_logs_regex(r"mockxconf.*50058")

Feature/l2check #70

Are you sure you want to change the base?

Feature/l2check #70

Conversation

Abhinavpv28 commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 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 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 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 Feb 10, 2026