Feature/rdk 59919 2

[Abhinavpv28]

No description provided.

[Copilot]

Pull request overview

This pull request adds UploadLogsNow functionality to the logupload binary, migrating shell script functionality to C code. The implementation provides a special mode that copies log files to a temporary directory, creates an archive, and uploads it using the ONDEMAND strategy.

Changes:

• New module for UploadLogsNow workflow with dedicated implementation and tests
• Integration into main uploadstblogs.c with special parameter handling
• Extended file operations with uploadlogsnow-specific timestamp handling

Reviewed changes

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

Show a summary per file

File	Description
uploadstblogs/unittest/uploadlogsnow_gtest.cpp	New comprehensive unit test suite with 20+ test cases covering success, failure, and edge case scenarios
uploadstblogs/unittest/Makefile.am	Build configuration updated to include new uploadlogsnow_gtest binary
uploadstblogs/src/uploadstblogs.c	Added uploadlogsnow mode detection and workflow invocation in parse_args and execute functions
uploadstblogs/src/uploadlogsnow.c	Core implementation of uploadlogsnow workflow including file copying, archiving, and upload
uploadstblogs/src/file_operations.c	New add_timestamp_to_files_uploadlogsnow function with special file exclusion logic
uploadstblogs/src/Makefile.am	Build configuration updated to compile uploadlogsnow.c into library
uploadstblogs/include/uploadstblogs_types.h	Added uploadlogsnow_mode boolean flag to RuntimeContext structure
uploadstblogs/include/uploadlogsnow.h	New public API header declaring execute_uploadlogsnow_workflow function
uploadstblogs/include/file_operations.h	Added declaration for add_timestamp_to_files_uploadlogsnow function
unit_test.sh	Added uploadlogsnow_gtest to test execution list

---

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

[Copilot]

Inconsistent cleanup behavior: The cleanup section at lines 290-299 is only reached via goto from error paths, but when the workflow succeeds (ret = 0 at line 287), the code falls through to cleanup without using goto. This means the cleanup always runs, but the control flow is inconsistent. Consider removing the cleanup label and always executing the cleanup code, or use goto for all paths including success.

[Copilot]

Missing null pointer check for ctx parameter. The function should validate that ctx is not NULL before accessing ctx->dcm_log_path and other fields. While test case ExecuteWorkflow_NullContext expects -1 to be returned, there's no actual null check in the implementation that would prevent a segmentation fault.

Suggested change

	"[%s:%d] UploadLogsNow workflow execution started\n", __FUNCTION__, __LINE__);
	"[%s:%d] UploadLogsNow workflow execution started\n", __FUNCTION__, __LINE__);
	/* Validate context pointer before dereferencing */
	if (ctx == NULL) {
	RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB,
	"[%s:%d] Invalid runtime context: ctx is NULL\n",
	__FUNCTION__, __LINE__);
	write_upload_status("Failed");
	return -1;
	}

[Copilot]

Ambiguous naming: The variable g_status_file_content suggests it contains the full content of the status file, but it only stores the content from the most recent fprintf call (line 110), not accumulated content. Consider renaming to g_last_status_message or g_status_line to better reflect its actual behavior.

[Copilot]

Unclear comment: The comment states "// Should fail but continue workflow" but the expectation is EXPECT_EQ(-1, result), which means the workflow is expected to fail completely, not continue. Either the comment is incorrect and should say "Should fail completely" or the test expectation is wrong.

Suggested change

	// Should fail but continue workflow
	// Workflow should fail when file copy fails

[Copilot]

Incorrect comment: Line 491 has a closing comment "// namespace" but no namespace was opened in this file. This appears to be a copy-paste error from another test file. Remove this comment.

Suggested change

} // namespace

[Copilot]

Ineffective assertion: Using EXPECT_STRNE(g_status_file_content, "") to check if status contains "Failed" is not a proper validation. This only checks that the string is not empty, but doesn't verify that it actually contains "Failed". Use EXPECT_THAT(g_status_file_content, HasSubstr("Failed")) or similar to properly verify the expected content.

Suggested change

	EXPECT_STRNE(g_status_file_content, ""); // Status should contain "Failed"
	EXPECT_THAT(g_status_file_content, ::testing::HasSubstr("Failed"));

[Copilot]

Inefficient string operations: The function iterates through the entire filename length twice (once for AM pattern at lines 560-572, once for PM pattern at lines 575-587) using nested loops. Consider combining these checks or using strstr() for pattern matching to improve performance, especially for files with long names.

Suggested change

	// Check for existing AM timestamp pattern: .*-[0-9][0-9]AM-.*
	if (strlen(filename) > 6) {
	for (size_t i = 0; i < strlen(filename) - 6; i++) {
	if (filename[i] == '-' &&
	isdigit(filename[i+1]) && isdigit(filename[i+2]) &&
	filename[i+3] == 'A' && filename[i+4] == 'M' && filename[i+5] == '-') {
	should_skip = 1;
	RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB,
	"[%s:%d] Processing file...%s\n",
	__FUNCTION__, __LINE__, filename);
	break;
	}
	}
	}
	// Check for existing PM timestamp pattern: .*-[0-9][0-9]PM-.*
	if (!should_skip && strlen(filename) > 6) {
	for (size_t i = 0; i < strlen(filename) - 6; i++) {
	if (filename[i] == '-' &&
	isdigit(filename[i+1]) && isdigit(filename[i+2]) &&
	filename[i+3] == 'P' && filename[i+4] == 'M' && filename[i+5] == '-') {
	should_skip = 1;
	RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB,
	"[%s:%d] Processing file...%s\n",
	__FUNCTION__, __LINE__, filename);
	break;
	}
	}
	}
	// Check for reboot log pattern: reboot.log
	if (!should_skip && strcmp(filename, "reboot.log") == 0) {
	size_t filename_len = strlen(filename);
	// Check for existing AM/PM timestamp pattern: .*-[0-9][0-9](AM|PM)-.*
	if (filename_len > 6) {
	for (size_t i = 0; i < filename_len - 6; i++) {
	if (filename[i] == '-' &&
	isdigit((unsigned char)filename[i+1]) &&
	isdigit((unsigned char)filename[i+2]) &&
	filename[i+5] == '-' &&
	((filename[i+3] == 'A' && filename[i+4] == 'M') ||
	(filename[i+3] == 'P' && filename[i+4] == 'M'))) {
	should_skip = 1;
	RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB,
	"[%s:%d] Processing file...%s\n",
	__FUNCTION__, __LINE__, filename);
	break;
	}
	}
	}
	// Check for reboot log pattern: reboot.log
	if (!should_skip && strcmp(filename, "reboot.log") == 0) {
	if (!should_skip && strcmp(filename, "reboot.log") == 0) {

[Copilot]

Misleading log message: The message "Processing file..." is logged when files are being SKIPPED due to existing timestamps or special patterns. The message should indicate that the file is being skipped, not processed. Consider changing to "Skipping file with existing timestamp: %s" or similar.

[Copilot]

Incorrect test expectation: The test expects result to be -1 (EXPECT_EQ(-1, result)) when copy fails, but according to the mock implementation at line 158, copy_file returns 0 on failure and 1 on success. The copy_files_to_dcm_path function at line 209 checks if copied_files < 0, but copy_file never returns negative values. This means the test may pass incorrectly or the implementation doesn't properly detect copy failures.

Suggested change

	// Should fail but continue workflow
	EXPECT_EQ(-1, result);
	// Workflow should still succeed even if file copies fail
	EXPECT_EQ(0, result);

[rdkcmf-jenkins]

Coverity Issue - Time of check time of use

Calling function "stat" to perform check on "old_path".

Low Impact, CWE-367
TOCTOU

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

---

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

[Copilot]

The function signature for remove_directory is incorrect. According to file_operations.h, remove_directory returns bool, not int. The declaration should be: bool remove_directory(const char* path); Additionally, the mock implementation at line 168-170 returns int values (0 or 1) instead of bool values and has inverted logic that conflicts with the usage in uploadlogsnow.c.

[Copilot]

The function execute_uploadlogsnow_workflow does not validate that the ctx parameter is non-NULL before dereferencing it. This can lead to a segmentation fault if called with a NULL pointer. Add a NULL check at the beginning of the function to prevent undefined behavior.

Suggested change

	if (ctx == NULL) {
	RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB,
	"[%s:%d] RuntimeContext pointer is NULL\n", __FUNCTION__, __LINE__);
	write_upload_status("Failed");
	return -1;
	}

[Copilot]

The loop condition uses strlen(filename) repeatedly, which is inefficient. Each call to strlen traverses the entire string. Consider storing the length in a variable before the loop to improve performance, especially for longer filenames.

[Copilot]

The same inefficient pattern with strlen(filename) is repeated. Consider storing the length in a variable before this loop as well for better performance.

[Copilot]

The mock implementation has incorrect return type and inverted logic. It should return bool (not int) to match the actual function signature in file_operations.h. The correct implementation should be: return g_remove_directory_should_fail ? false : true; This mismatch causes the calling code in uploadlogsnow.c line 292 to incorrectly interpret the return value.

[Copilot]

The comment says "Should fail but continue workflow" but the expectation is that the result should be -1, indicating failure. If all copy operations fail, the workflow should indeed fail (return -1), which is the expected behavior. However, the comment is misleading because it suggests the workflow continues despite failure. Consider clarifying the comment to say "Should fail when all file copy operations fail" to better reflect the actual expectation.

[Copilot]

The test expects result to be -1 when the empty log path is provided, but there is no validation in execute_uploadlogsnow_workflow to check for an empty log_path before using it. This test may not be validating the actual behavior. Either the implementation needs to add this validation, or the test expectation should be adjusted.

[Copilot]

The function is missing validation that ctx->log_path is not empty before attempting to use it. An empty log_path could lead to errors in subsequent operations. Add a check to ensure ctx->log_path is not empty and return an error if it is.

[Copilot]

The function returns the count of copied files (copied_count), but when no files are copied (copied_count = 0), it returns 0 which could be confused with success. The calling code at line 209 treats negative values as failure but doesn't distinguish between "0 files copied" and "successful operation with files copied". Consider returning -1 if no files were copied when files exist in the directory, or document that 0 is a valid success case for empty directories.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.

---

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

[Copilot]

The test expects the function to return -1 for null context, but without reviewing the actual implementation of execute_uploadlogsnow_workflow, we cannot verify this behavior. Based on reviewing uploadlogsnow.c, there is no null check at the start of the function, so this test would actually fail with a segmentation fault rather than return -1. This test reveals a bug in the implementation.

[Copilot]

STATUS_FILE and DCM_TEMP_DIR are defined in both uploadstblogs.c and uploadlogsnow.c with the same values. These constants should be defined once in a shared header file to avoid duplication and potential inconsistencies if one is updated but not the other.

Suggested change

	#define LOG_UPLOADSTB "LOG.RDK.UPLOADSTB"
	#define STATUS_FILE "/opt/loguploadstatus.txt"
	#define DCM_TEMP_DIR "/tmp/DCM"
	#define LOG_UPLOADSTB "LOG.RDK.UPLOADSTB"
	#ifndef STATUS_FILE
	#define STATUS_FILE "/opt/loguploadstatus.txt"
	#endif
	#ifndef DCM_TEMP_DIR
	#define DCM_TEMP_DIR "/tmp/DCM"
	#endif

[Copilot]

The mock time value is hardcoded to 1642780800 (January 21, 2022), but this is inconsistent with the copyright year of 2026 used elsewhere in the file. While this doesn't affect functionality, consider using a more recent or clearly documented test timestamp for consistency.

Suggested change

	static time_t mock_time_value = 1642780800; // 2022-01-21 12:00:00
	static char mock_ctime_buffer[32] = "Fri Jan 21 12:00:00 2022\n";
	static time_t mock_time_value = 1642780800; // Fixed test timestamp: 2022-01-21 12:00:00 (arbitrary, independent of copyright year)
	static char mock_ctime_buffer[32] = "Fri Jan 21 12:00:00 2022\n"; // Must remain consistent with mock_time_value for deterministic tests

[Copilot]

Test name suggests testing empty log path, but the test only verifies memset of ctx.log_path to 0, which may not represent an empty string properly. While memset to 0 does create an empty string, the test should verify that an empty string "" is actually what's being tested by checking that ctx.log_path[0] == '\0' after memset, or simply using strcpy(ctx.log_path, "") for clarity.

[Copilot]

The conditional Windows compatibility code defines DIR and dirent structures, but these definitions are incomplete stubs that will not work correctly with the mock implementations. The DIR type is just a struct with a dummy int, and dirent only has d_name. While this may work for basic compilation on Windows, it suggests this test is not actually intended to run on Windows. Consider either removing the Windows compatibility block or providing a complete mock implementation, or adding a comment explaining that Windows builds are not fully supported for these tests.

Suggested change

	// Define compatibility for Windows/Linux builds
	#ifndef _WIN32
	#include <dirent.h>
	#else
	// Windows compatibility
	typedef struct {
	int dummy;
	} DIR;
	struct dirent {
	char d_name[256];
	};
	// Note: this test relies on POSIX dirent APIs and is not supported on Windows.
	#ifdef _WIN32
	#error "uploadlogsnow_gtest is not supported on Windows (_WIN32)."

[Copilot]

The code sets the last character to null terminator after using strncpy, which is good practice. However, this means the effective buffer size is sizeof(session.archive_file) - 1, not sizeof(session.archive_file). If the full_archive_path length equals sizeof(session.archive_file) - 1, it will be truncated by one character when copied. The strncpy on line 273 should use sizeof(session.archive_file) - 1 as the size parameter to match the -1 offset used when setting the null terminator, or verify that full_archive_path fits within the buffer before copying.

[Copilot]

The extern "C" declarations for standard library functions like fopen, fclose, fprintf, etc., will conflict with the actual system implementations. This approach of mocking standard library functions by redeclaring them in an extern "C" block is problematic because it creates undefined behavior - you cannot reliably override standard library functions this way in C++. Consider using a proper mocking framework like GMock with dependency injection, or wrapping these system calls in a testable interface layer.

Suggested change

	// Mock system functions that uploadlogsnow depends on
	extern "C" {
	// Mock file system functions
	FILE* fopen(const char* filename, const char* mode);
	int fclose(FILE* stream);
	int fprintf(FILE* stream, const char* format, ...);
	size_t fread(void* ptr, size_t size, size_t nmemb, FILE* stream);
	size_t fwrite(const void* ptr, size_t size, size_t nmemb, FILE* stream);
	int stat(const char* path, struct stat* buf);
	DIR* opendir(const char* name);
	struct dirent* readdir(DIR* dirp);
	int closedir(DIR* dirp);
	time_t time(time_t* tloc);
	char* ctime(const time_t* timep);
	int snprintf(char* str, size_t size, const char* format, ...);
	// Mock project-specific functions that uploadlogsnow depends on
	extern "C" {

[Copilot]

The function returns -1 without first checking if ctx is NULL. If ctx is NULL, accessing ctx->log_path on line 208 will cause a segmentation fault. Add a NULL check at the beginning of the function before any ctx member access.

[Copilot]

The documentation says the function "returns 0 on success, negative value on failure", but should be more specific about what negative value is returned. Based on the implementation, it always returns -1 on failure. Update documentation to state "returns 0 on success, -1 on failure" for clarity.

Suggested change

	* @return 0 on success, negative value on failure
	* @return 0 on success, -1 on failure

[Copilot]

The session variable is stack-allocated and only initialized with memset to 0. While session.strategy is set to STRAT_ONDEMAND, other fields like session.primary are not explicitly initialized. After reviewing the code, decide_paths is called to initialize session.primary, but this dependency is fragile. Consider adding a comment explaining that decide_paths must be called to properly initialize the session, or initialize session.primary explicitly.

Suggested change

	// Follow RRD upload pattern: decide_paths() then execute_upload_cycle()
	/*
	* NOTE: decide_paths() MUST be called before execute_upload_cycle() for this
	* session instance. The session struct is zero-initialized and only some
	* fields (e.g. strategy, archive_file) are set explicitly above; other
	* fields such as session.primary are initialized inside decide_paths().
	* Any future refactoring must preserve this ordering or ensure that those
	* fields are otherwise initialized before use.
	*/