Rebase

[Abhinavpv28]

No description provided.

[Copilot]

Pull request overview

Updates device-compatibility documentation/messaging, extends uploadSTBLogs trigger handling for memcapture, and introduces a new backup_logs C implementation (plus unit/functional test coverage and build-system wiring).

Changes:

• Replace “PLATCO-only” wording with “TV-only” across usbLogUpload code/comments/docs.
• Add TRIGGER_MEMCAPTURE support to uploadSTBLogs argument parsing and adjust cleanup behavior for that trigger.
• Add the new backup_logs module (source + headers), install its special_files.conf, and add unit/functional tests + Autotools integration.

Reviewed changes

Copilot reviewed 73 out of 73 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
usbLogUpload/src/usb_log_validation.c	Update device-compatibility comment to TV-only wording.
usbLogUpload/include/usb_log_validation.h	Update API doc comment to TV-only wording.
usbLogUpload/docs/usb-log-upload-requirements.md	Update requirements to state TV-only support.
usbLogUpload/docs/usb-log-upload-flowcharts.md	Update flowcharts to TV-only device check.
usbLogUpload/docs/shared-functions-analysis.md	Update analysis doc to TV-only requirement wording.
usbLogUpload/README.md	Update README device validation wording to TV-only.
uploadstblogs/src/uploadstblogs.c	Parse new MEMCAPTURE trigger type.
uploadstblogs/src/cleanup_handler.c	Skip archive removal for memcapture trigger runs.
uploadstblogs/include/uploadstblogs_types.h	Add TRIGGER_MEMCAPTURE enum value.
test/run_uploadstblogs_l2.sh	License header wording update (Licenses.txt → LICENSE).
test/run_l2.sh	License header wording update (Licenses.txt → LICENSE).
test/functional-tests/tests/uploadstblogs_helper.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_upload_strategies.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_security.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_retry_logic.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_resource_management.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_normal_upload.py	License header wording update.
test/functional-tests/tests/test_uploadstblogs_error_handling.py	License header wording update.
test/functional-tests/tests/test_uploadLogsNow.py	License header wording update.
test/functional-tests/tests/test_start_dcm-agent.py	License header wording update.
test/functional-tests/tests/test_log_upload_onreboot_true_case.py	License header wording update.
test/functional-tests/tests/test_log_upload_onreboot_false_case.py	License header wording update.
test/functional-tests/tests/test_log_upload_onreboot_MM_case.py	License header wording update.
test/functional-tests/tests/test_log_upload_cron_NULL_case.py	License header wording update.
test/functional-tests/tests/test_existence_of_dcmsettingsFile.py	License header wording update.
test/functional-tests/tests/test_bootup_sequence.py	License header wording update.
test/functional-tests/tests/test_backuplogs_system_integration.py	Add backup_logs integration/lifecycle functional tests.
test/functional-tests/tests/test_backuplogs_special_files.py	Add backup_logs special-files functional tests.
test/functional-tests/tests/test_backuplog_config_manager.py	Add backup_logs config-manager functional tests (new).
test/functional-tests/tests/test_backup_engine.py	Add backup_logs engine/strategy functional tests.
test/functional-tests/tests/helper_functions.py	License header wording update.
test/functional-tests/tests/backup_logs_helper.py	Add shared helper utilities for backup_logs functional tests.
test/functional-tests/features/uploadstblogs_upload_strategies.feature	License header wording update.
test/functional-tests/features/uploadstblogs_security.feature	License header wording update.
test/functional-tests/features/uploadstblogs_retry_logic.feature	License header wording update.
test/functional-tests/features/uploadstblogs_resource_management.feature	License header wording update.
test/functional-tests/features/uploadstblogs_normal_upload.feature	License header wording update.
test/functional-tests/features/uploadstblogs_error_handling.feature	License header wording update.
test/functional-tests/features/dcm-agent_start.feature	License header wording update.
test/functional-tests/features/dcm-agent_logupload_Uploadonreboot_true.feature	License header wording update.
test/functional-tests/features/dcm-agent_logupload_Uploadonreboot_false.feature	License header wording update.
test/functional-tests/features/dcm-agent_logupload_Uploadonreboot_MMenabled.feature	License header wording update.
test/functional-tests/features/dcm-agent_cron_NULL_check.feature	License header wording update.
test/functional-tests/features/dcm-agent_check_file_existence.feature	License header wording update.
test/functional-tests/features/dcm-agent_bootup_sequence.feature	License header wording update.
test/functional-tests/features/backup_logs_sys_integration.feature	Add backup_logs system-integration BDD feature file.
test/functional-tests/features/backup_logs_special_files.feature	Add backup_logs special-files BDD feature file.
test/functional-tests/features/backup_logs_engine.feature	Add backup_logs engine/strategy BDD feature file.
test/functional-tests/features/backup_logs_config_manager.feature	Add backup_logs config-manager BDD feature file.
special_files.conf	Add default special-files config to be installed under /etc/backup_logs/.
configure.ac	Include backup_logs/Makefile in Autotools outputs.
Makefile.am	Add backup_logs to build + install special_files.conf to sysconfdir.
CHANGELOG.md	Add release notes entries for recent versions/PRs.
backup_logs/src/sys_integration.c	Implement systemd notification wrapper logic for backup_logs.
backup_logs/src/special_files.c	Implement parsing/execution of “special files” operations.
backup_logs/src/config_manager.c	Implement backup_logs config loading from property files.
backup_logs/src/backup_logs.c	Implement backup_logs init/execute/cleanup/main orchestration.
backup_logs/src/backup_engine.c	Implement backup strategies and file-rotation/move/copy logic.
backup_logs/include/sys_integration.h	Public header for systemd notification helper.
backup_logs/include/special_files.h	Public header for special-files manager.
backup_logs/include/config_manager.h	Public header for backup_logs configuration APIs.
backup_logs/include/backup_types.h	Shared types/constants for backup_logs module.
backup_logs/include/backup_logs.h	Public header for backup_logs top-level APIs.
backup_logs/include/backup_engine.h	Public header for backup strategy/engine APIs.
backup_logs/unittest/configure.ac	Add Autotools config for backup_logs unit tests.
backup_logs/unittest/Makefile.am	Build/test wiring for backup_logs gtest binaries.
backup_logs/unittest/mocks/config_manager_mocks.h	Add mocks header for config-manager unit tests.
backup_logs/unittest/sys_integration_gtest.cpp	Add unit tests for systemd notification function.
backup_logs/unittest/special_files_gtest.cpp	Add unit tests for special-files parsing/execution.
backup_logs/unittest/config_manager_gtest.cpp	Add unit tests for config manager behavior.
backup_logs/unittest/backup_logs_gtest.cpp	Add unit tests for backup_logs orchestration.
backup_logs/unittest/backup_engine_gtest.cpp	Add unit tests for backup engine strategies and helpers.
backup_logs/Makefile.am	Build/link the backup_logs binary.

[Copilot]

backup_logs/include/config_manager.h declares several public functions (e.g., special_files_config_load, config_parse_environment, config_load_device_properties, etc.) that do not have implementations anywhere in the codebase. Either implement these functions or remove them from the header to avoid exposing a misleading/unused API surface.

Suggested change

	/**
	* @brief Load special files configuration
	*
	* @param config Special files configuration structure
	* @param config_file Path to configuration file
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_config_load(special_files_config_t* config, const char* config_file);
	/**
	* @brief Validate special files configuration
	*
	* @param config Special files configuration to validate
	* @return int BACKUP_SUCCESS if valid, error code if invalid
	*/
	int special_files_config_validate(const special_files_config_t* config);
	/**
	* @brief Free special files configuration resources
	*
	* @param config Special files configuration to free
	*/
	void special_files_config_free(special_files_config_t* config);
	/**
	* @brief Execute special files operations
	*
	* @param config Special files configuration
	* @param backup_config Main backup configuration for variable substitution
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_execute_operations(const special_files_config_t* config,
	const backup_config_t* backup_config);
	/**
	* @brief Parse environment variables and paths
	*
	* @param config Backup configuration to update with parsed values
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_parse_environment(backup_config_t* config);
	/**
	* @brief Load device properties
	*
	* @param config Backup configuration to update
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_load_device_properties(backup_config_t* config);
	/**
	* @brief Load include properties
	*
	* @param config Backup configuration to update
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_load_include_properties(backup_config_t* config);

[Copilot]

The newly added if (ctx->trigger_type != TRIGGER_MEMCAPTURE) block introduces inconsistent brace placement and indentation (the nested block contents are not indented). Please reformat this block to match the surrounding style to keep finalize() readable and avoid future merge conflicts.

[Copilot]

__wrap_sys_send_systemd_notification is declared to return int but it never returns a value. This will typically fail the build under common warning settings and also makes test behavior undefined. Return an explicit status code (e.g., BACKUP_SUCCESS) from this wrapper.

Suggested change

	strncpy(mock_control.sys_send_systemd_notification_last_message, message,
	sizeof(mock_control.sys_send_systemd_notification_last_message) - 1);
	mock_control.sys_send_systemd_notification_last_message[sizeof(mock_control.sys_send_systemd_notification_last_message) - 1] = '\0';
	}
	strncpy(mock_control.sys_send_systemd_notification_last_message, message,
	sizeof(mock_control.sys_send_systemd_notification_last_message) - 1);
	mock_control.sys_send_systemd_notification_last_message[sizeof(mock_control.sys_send_systemd_notification_last_message) - 1] = '\0';
	}
	return BACKUP_SUCCESS;

[Copilot]

config_load() treats any HDD_ENABLED value other than the literal string "false" as enabled (true). This means values like "invalid" or an empty string will incorrectly enable the HDD strategy. Parse explicitly for "true"/"false" (and otherwise default to false and log a warning) so behavior matches the documented expectations in the repo’s backup_logs config feature tests.

[Copilot]

This unit test sets LOG_PATH to "/var/logs" via getIncludePropertyData, but then asserts that test_config.log_path is still the default "/opt/logs". With the current config_load() implementation, the property value should be applied, so this assertion will fail. Update the expected values (including derived paths) to match the configured LOG_PATH.

Suggested change

	EXPECT_STREQ(test_config.log_path, "/opt/logs");
	EXPECT_STREQ(test_config.prev_log_path, "/opt/logs/PreviousLogs");
	EXPECT_STREQ(test_config.prev_log_backup_path, "/opt/logs/PreviousLogs_backup");
	EXPECT_STREQ(test_config.log_path, "/var/logs");
	EXPECT_STREQ(test_config.prev_log_path, "/var/logs/PreviousLogs");
	EXPECT_STREQ(test_config.prev_log_backup_path, "/var/logs/PreviousLogs_backup");

[Copilot]

Pull request overview

Copilot reviewed 88 out of 88 changed files in this pull request and generated 7 comments.

[Copilot]

isxdigit() is undefined for negative char values unless the argument is cast to unsigned char (or EOF). To keep the test portable across platforms where char is signed, cast sha256_output[i] to (unsigned char) before calling isxdigit().

[Copilot]

The implementation notes here claim the SHA256 hex conversion uses a nibble lookup table to avoid per-byte snprintf overhead, but the current md5_utils.c implementation uses snprintf in a loop. Either update the implementation to match this design note, or adjust the documentation to reflect the actual conversion approach.

[Copilot]

move_log_files_by_pattern() returns BACKUP_ERROR_FILESYSTEM when no files were moved. An empty source directory (or no matching files) is not necessarily an error and this return value is inconsistent with the function’s “success/error” contract in the header. Consider returning BACKUP_SUCCESS when no matches are found and reserving BACKUP_ERROR_FILESYSTEM for actual I/O/copy failures (optionally returning the moved_count via an output parameter if needed).

[Copilot]

Now that backup_logs/Makefile is added to AC_CONFIG_FILES, configure.ac should also check for the new module’s required build dependencies (e.g., systemd headers/lib for sd_notify, secure_wrapper, fwutils/rdkloggers as needed) and fail with a clear AC_MSG_ERROR or make them optional via --enable/--disable. Otherwise ./configure can succeed but the build can fail later at compile/link time.

Suggested change

	# backup_logs is included in AC_CONFIG_FILES, so validate its required
	# build dependencies during configure to avoid later compile/link failures.
	AC_CHECK_HEADERS([systemd/sd-daemon.h], [],
	[AC_MSG_ERROR([backup_logs requires systemd development headers (missing systemd/sd-daemon.h)])])
	AC_CHECK_LIB([systemd], [sd_notify], [],
	[AC_MSG_ERROR([backup_logs requires libsystemd with sd_notify support])])

[Copilot]

config_load() uses 32-byte temporary buffers (log_path_buf/app_persistent_path_buf/hdd_enabled_buf) for values that are later copied into PATH_MAX-sized fields. If the property values exceed 31 chars they will be silently truncated, leading to incorrect paths and hard-to-debug filesystem failures. Use PATH_MAX-sized buffers (or sizeof(config->log_path)/sizeof(config->persistent_path)) for these property reads and treat truncation as an error/warning.

[Copilot]

HDD_ENABLED parsing treats any value other than the literal string "false" as enabled (true). This makes invalid/empty values evaluate to true, which contradicts the typical expectation (and the new functional tests/features that describe invalid values defaulting to false). Consider explicitly accepting "true" (case-insensitive) and defaulting to false for missing/invalid values, logging a warning when the value is unrecognized.

[Copilot]

backup_logs/include/config_manager.h declares several public APIs (e.g., special_files_config_load/special_files_config_validate/special_files_config_free/etc.) but there are no corresponding implementations in backup_logs/src (search only finds these declarations in this header/docs). Either implement these functions or remove them from the public header to avoid unresolved-symbol/link failures for any consumer that includes the header and calls them.

Suggested change

	* @brief Load special files configuration
	*
	* @param config Special files configuration structure
	* @param config_file Path to configuration file
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_config_load(special_files_config_t* config, const char* config_file);
	/**
	* @brief Validate special files configuration
	*
	* @param config Special files configuration to validate
	* @return int BACKUP_SUCCESS if valid, error code if invalid
	*/
	int special_files_config_validate(const special_files_config_t* config);
	/**
	* @brief Free special files configuration resources
	*
	* @param config Special files configuration to free
	*/
	void special_files_config_free(special_files_config_t* config);
	/**
	* @brief Execute special files operations
	*
	* @param config Special files configuration
	* @param backup_config Main backup configuration for variable substitution
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_execute_operations(const special_files_config_t* config,
	const backup_config_t* backup_config);
	/**

[Copilot]

Pull request overview

Copilot reviewed 92 out of 92 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

uploadstblogs/src/file_operations.c:563

• add_timestamp_to_files_uploadlogsnow() calls strftime() without checking its return value. If strftime() returns 0, the timestamp buffer contents are unspecified, and strlen(timestamp)/strncmp() logic can end up skipping all files (or behaving unpredictably). Mirror add_timestamp_to_files(): check the strftime() result and return an error (or fallback) when formatting fails.

[Copilot]

backup_logs/include/config_manager.h declares several public functions (e.g., special_files_config_load/validate/free, config_parse_environment, config_load_device_properties, etc.) that do not have implementations anywhere in backup_logs. This creates a misleading public API surface and will produce linker errors if any new code starts using these declarations. Either implement these functions or remove the unused declarations from the header so it only exposes supported APIs.

Suggested change

	* @brief Load special files configuration
	*
	* @param config Special files configuration structure
	* @param config_file Path to configuration file
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_config_load(special_files_config_t* config, const char* config_file);
	/**
	* @brief Validate special files configuration
	*
	* @param config Special files configuration to validate
	* @return int BACKUP_SUCCESS if valid, error code if invalid
	*/
	int special_files_config_validate(const special_files_config_t* config);
	/**
	* @brief Free special files configuration resources
	*
	* @param config Special files configuration to free
	*/
	void special_files_config_free(special_files_config_t* config);
	/**
	* @brief Execute special files operations
	*
	* @param config Special files configuration
	* @param backup_config Main backup configuration for variable substitution
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_execute_operations(const special_files_config_t* config,
	const backup_config_t* backup_config);
	/**
	* @brief Parse environment variables and paths
	*
	* @param config Backup configuration to update with parsed values
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_parse_environment(backup_config_t* config);
	/**
	* @brief Load device properties
	*
	* @param config Backup configuration to update
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_load_device_properties(backup_config_t* config);
	/**
	* @brief Load include properties
	*
	* @param config Backup configuration to update
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int config_load_include_properties(backup_config_t* config);
	* @brief Execute special files operations
	*
	* @param config Special files configuration
	* @param backup_config Main backup configuration for variable substitution
	* @return int BACKUP_SUCCESS on success, error code on failure
	*/
	int special_files_execute_operations(const special_files_config_t* config,
	const backup_config_t* backup_config);

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

uploadstblogs/src/file_operations.c:563

• In add_timestamp_to_files_uploadlogsnow(), the return value of strftime() is not checked. If it returns 0 (e.g., unexpected format failure), timestamp will be empty and the subsequent prefix checks/renames will behave incorrectly. Capture the return value and treat 0 as an error (log and return -1) before using timestamp/g_timestamp_prefix.

[Copilot]

sys_send_systemd_notification() ignores the return value of snprintf() when building the sd_notify payload. If the message is long, this can silently truncate the state string. Check snprintf()'s return value (negative/truncation) and either truncate explicitly with a log or return an error so callers can react.

[Copilot]

emit_upload_aborted() currently sends LOG_UPLOAD_FAILED, but event_manager.c defines a distinct LOG_UPLOAD_ABORTED state (mapped to sysstate=2). This changes the semantics of an aborted upload into a failure and will make downstream consumers misinterpret the reason. Update emit_upload_aborted() to emit the aborted event code (and keep maintenance signaling consistent with the aborted case).

[Copilot]

calculate_file_sha256() converts the digest to hex using per-byte snprintf() in a loop. On embedded targets this is relatively expensive and also conflicts with the LLD note claiming a lookup-table conversion. Consider replacing this with a simple hex lookup table (2 chars per byte) and write directly into sha256_hex, and optionally assert sha256_len == 32 before formatting.

[Copilot]

backup_engine_gtest builds with -DUTILS_SUCCESS=1, while the other unit tests (and typical convention) treat UTILS_SUCCESS as 0. This can invert success/failure handling in the code under test and produce misleading results. Align UTILS_SUCCESS across all backup_logs unit-test targets to the actual value used in production headers.

Suggested change

	-DUTILS_SUCCESS=1
	-DUTILS_SUCCESS=0

[Copilot]

Pull request overview

Copilot reviewed 136 out of 136 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (11)

uploadstblogs/unittest/Makefile.am:48

• archive_manager_gtest.cpp uses dlsym/RTLD_NEXT, which typically requires linking with -ldl on glibc. COMMON_LDADD currently doesn’t include -ldl, so archive_manager_gtest may fail to link.
  uploadstblogs/src/cleanup_handler.c:255
• In cleanup_old_archives(), the subdirectory path is built with snprintf() but the return value isn’t checked. If the path is truncated, the recursion will run on an incorrect path (and could delete the wrong files).
  uploadstblogs/src/strategy_handler.c:76
• cleanup_old_archives() return value is ignored. If cleanup fails (e.g., ctx->log_path missing/permission denied), the workflow continues silently, which makes failures hard to diagnose.
  uploadstblogs/unittest/archive_manager_gtest.cpp:707
• This main() sets GTest JSON output to /tmp/Gtest_Report/, but it never ensures the directory exists. Also, buffer is unused and can trigger warnings under stricter builds.
  backup_logs/src/config_manager.c:40
• config_load() uses 32-byte temporary buffers for LOG_PATH and APP_PERSISTENT_PATH. These paths can easily exceed 31 characters, causing truncation and incorrect derived paths.

int config_load(backup_config_t* config) {
    char log_path_buf[32] = {0};
    char hdd_enabled_buf[32] = {0};
    char app_persistent_path_buf[32] = {0};
    

uploadstblogs/docs/lld/uploadSTBLogs_LLD.md:202

• The LLD claims the SHA256-to-hex conversion uses a nibble lookup table to avoid per-byte snprintf overhead, but the actual implementation in md5_utils.c uses snprintf in a loop. The doc should match the implementation (or the implementation should be updated).
  LICENSE:224
• The repository LICENSE file is Apache-2.0, but a generic MIT template (with placeholder "YEAR COPYRIGHT HOLDER") has been appended. This is not a valid license notice and can create legal ambiguity; third-party MIT components should be documented via NOTICE (or a dedicated third-party licenses file), not by appending a template to LICENSE.

                            MIT License

Copyright (c) YEAR COPYRIGHT HOLDER

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

test/functional-tests/tests/test_uploadstblogs_sync_gates.py:43

• This test uses subprocess.run(...), but the module only imports subprocess as sp. As written it will raise NameError before any assertions run.
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:116
• This test expects the literal log line "bacukup_logs sentinel detected. Proceeding." but that string is not present anywhere in uploadstblogs sources, so the assertion will always fail. Either update the production log message or change the test to validate behavior via existing logs/phase progress.
  configure.ac:137
• backup_logs is added as a new subproject, and backup_logs/src/sys_integration.c links against libsystemd (-lsystemd), but configure.ac doesn’t check for libsystemd headers/libs or provide a conditional build. This can break cross-compilation/builds where systemd isn’t available.
  uploadstblogs/src/event_manager.c:191
• emit_upload_aborted() is called when uploads are intentionally skipped (e.g., RFC/reboot-reason gating), but it now sends LOG_UPLOAD_FAILED. That will incorrectly report a failure (and may trigger error handling) for a deliberate abort/skip. It should emit LOG_UPLOAD_ABORTED (as before) to preserve semantics.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 136 out of 136 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (12)

test/functional-tests/tests/test_uploadstblogs_sync_gates.py:42

• This test file imports subprocess as sp, but calls subprocess.run(...) which is undefined and will raise a NameError at runtime.
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:105
• subprocess.run(...) is used again here, but subprocess is not imported (only import subprocess as sp). This will fail the test run.
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:115
• The expected log string is misspelled as "bacukup_logs..." but the implementation logs "backup_logs sentinel detected. Proceeding." (uploadstblogs/src/strategies.c:899). As written, this assertion will always fail.
  test/functional-tests/tests/test_uploadstblogs_sync_gate_backup_logs.py:124
• The expected log string is misspelled as "bacukup_logs..." but the implementation logs "backup_logs sentinel detected. Proceeding." (uploadstblogs/src/strategies.c:899). This assertion will fail even when the gate passes.
  test/functional-tests/tests/test_uploadstblogs_sync_gate_backup_logs.py:166
• The expected log string is misspelled as "bacukup_logs..." but the implementation logs "backup_logs sentinel detected. Proceeding." (uploadstblogs/src/strategies.c:899). This check should use the correct string.
  uploadstblogs/src/event_manager.c:191
• emit_upload_aborted() now emits LOG_UPLOAD_FAILED, but the event mapping explicitly supports LOG_UPLOAD_ABORTED (and callers use this function for abort flows). This makes aborts indistinguishable from failures for IARM consumers, and leaves the LOG_UPLOAD_ABORTED mapping unused.
  uploadstblogs/src/cleanup_handler.c:255
• When recursing into a subdirectory, the snprintf() result is not checked. If the path is truncated, cleanup_old_archives() may recurse into an unintended path (or fail to clean the intended one). The regular-file branch already guards against this; the directory branch should too.
  uploadstblogs/unittest/archive_manager_gtest.cpp:703
• GTest JSON output is directed to /tmp/Gtest_Report/, but that directory is never created. When fopen() is delegated to the real libc implementation, report creation will fail if the directory is missing.
  usbLogUpload/unittest/usb_log_main_gtest.cpp:55
• This test is labeled "valid input" but it no longer asserts success; it only asserts the function did not return USB_NOT_MOUNTED. Renaming the test (and its brief) would avoid overstating what it verifies.
  LICENSE:207
• The MIT license stanza still contains the template placeholder "YEAR COPYRIGHT HOLDER", which makes the licensing text incomplete/incorrect.

                            MIT License

Copyright (c) YEAR COPYRIGHT HOLDER

uploadstblogs/docs/lld/uploadSTBLogs_LLD.md:202

• The LLD claims the SHA256 hex conversion uses a nibble lookup table, but the current implementation in uploadstblogs/src/md5_utils.c uses per-byte snprintf(). Either update the implementation or adjust this note to match reality.
  backup_logs/src/config_manager.c:96
• HDD_ENABLED parsing currently treats any value other than literal "false" as enabled (e.g., "invalid" or empty string becomes true). That contradicts the expected boolean semantics and will select the wrong backup strategy.

    /* Check HDD_ENABLED like shell script */
    if (getDevicePropertyData("HDD_ENABLED", hdd_enabled_buf, sizeof(hdd_enabled_buf)) == UTILS_SUCCESS) {
        config->hdd_enabled = (strcmp(hdd_enabled_buf, "false") != 0);
        RDK_LOG(RDK_LOG_INFO, LOG_BACKUP_LOGS, "HDD_ENABLED loaded from properties: %s (evaluated to %s)\n", 
                hdd_enabled_buf, config->hdd_enabled ? "true" : "false");
    } else {
        config->hdd_enabled = false;  /* Default to false if not found */
        RDK_LOG(RDK_LOG_WARN, LOG_BACKUP_LOGS, "HDD_ENABLED not found in properties, using default: false\n");
    }

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 137 out of 137 changed files in this pull request and generated 1 comment.

Suppressed comments (11)

uploadstblogs/src/event_manager.c:190

• emit_upload_aborted() is emitting LOG_UPLOAD_FAILED even though LOG_UPLOAD_ABORTED is defined; this makes an intentional abort indistinguishable from a real failure (and conflicts with the function name).
  uploadstblogs/src/event_manager.c:435
• emit_folder_missing_error() is used when PREV_LOG_PATH is missing (uploadstblogs/src/validation.c notes this should map to MAINT_LOGUPLOAD_ERROR). Emitting MAINT_LOGUPLOAD_COMPLETE here reports success for an error condition.
  uploadstblogs/src/file_operations.c:567
• add_timestamp_to_files_uploadlogsnow() should check strftime() return value (it can return 0 on buffer too small or other failure), similar to add_timestamp_to_files().
  uploadstblogs/docs/lld/uploadSTBLogs_LLD.md:201
• The LLD states the SHA256 hex conversion uses a nibble lookup table to avoid per-byte snprintf overhead, but the implementation in uploadstblogs/src/md5_utils.c currently uses snprintf per byte. This is a doc/implementation mismatch.
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:42
• This test uses subprocess.run() but only imports subprocess as sp; as written it will raise NameError and fail immediately.
  LICENSE:206
• The MIT license text includes the placeholder "YEAR COPYRIGHT HOLDER", which should be replaced with the actual copyright holder/year to avoid an invalid/ambiguous LICENSE file.

Copyright (c) YEAR COPYRIGHT HOLDER

uploadstblogs/unittest/archive_manager_gtest.cpp:703

• archive_manager_gtest.cpp main() declares and memset()s a 'buffer' that is never used. This can trigger -Wunused-variable in some toolchains and breaks -Werror builds.
  backup_logs/src/config_manager.c:39
• The path buffers are only 32 bytes, which can truncate valid LOG_PATH/APP_PERSISTENT_PATH values and lead to incorrect derived paths. Use PATH_MAX (or sizeof(destination)) for these temporary buffers.

    char log_path_buf[32] = {0};
    char hdd_enabled_buf[32] = {0};
    char app_persistent_path_buf[32] = {0};

backup_logs/src/config_manager.c:92

• HDD_ENABLED parsing treats any value other than literal "false" as true (including "invalid"), but the added feature specs expect invalid values to default to false with a warning.

    if (getDevicePropertyData("HDD_ENABLED", hdd_enabled_buf, sizeof(hdd_enabled_buf)) == UTILS_SUCCESS) {
        config->hdd_enabled = (strcmp(hdd_enabled_buf, "false") != 0);
        RDK_LOG(RDK_LOG_INFO, LOG_BACKUP_LOGS, "HDD_ENABLED loaded from properties: %s (evaluated to %s)\n", 
                hdd_enabled_buf, config->hdd_enabled ? "true" : "false");

uploadstblogs/src/Makefile.am:23

• This change adds -lparsejson to the link line, but configure.ac does not check for the presence of the parsejson library/header. This can lead to configure succeeding but builds failing later on some targets.
  configure.ac:136
• backup_logs is now part of the build (AC_CONFIG_FILES includes backup_logs/Makefile), but configure.ac does not check for libsystemd headers/libs even though backup_logs links -lsystemd and includes <systemd/sd-daemon.h>. This can produce late build failures on platforms without systemd dev packages.

[Copilot]

Pull request overview

Copilot reviewed 137 out of 137 changed files in this pull request and generated no new comments.

Suppressed comments (11)

test/functional-tests/tests/test_uploadstblogs_sync_gates.py:42

• subprocess is not imported in this file (only subprocess as sp), so these calls will raise NameError at runtime. Use sp.run(...) consistently here.
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:104
• Same issue as above: this uses subprocess.run(...) but the module isn’t imported under that name. This will fail at runtime unless changed to sp.run(...) (or import subprocess).
  test/functional-tests/tests/test_uploadstblogs_sync_gates.py:115
• The expected log string has a typo (bacukup_logs). The implementation logs backup_logs sentinel detected. Proceeding. (see uploadstblogs/src/strategies.c). As written, this assertion will fail even when the gate passes.
  test/functional-tests/tests/test_uploadstblogs_sync_gate_backup_logs.py:124
• The expected log string contains a typo (bacukup_logs) and does not match the actual implementation log (backup_logs sentinel detected. Proceeding.). This will make the test fail even when the behavior is correct.
  uploadstblogs/src/event_manager.c:190
• emit_upload_aborted() currently logs an unrelated message and emits LOG_UPLOAD_FAILED, even though the module defines LOG_UPLOAD_ABORTED and the function name indicates an abort (not a failure). This changes the external event semantics for abort scenarios.
  uploadstblogs/src/event_manager.c:435
• emit_folder_missing_error() is called when PREV_LOG_PATH is missing (validation.c comments that the script sends MAINT_LOGUPLOAD_ERROR=5 for this). Emitting MAINT_LOGUPLOAD_COMPLETE here makes a missing required folder look like success.
  uploadstblogs/src/cleanup_handler.c:255
• When recursing into subdirectories, the snprintf result isn’t checked. If the path is truncated, the recursion may walk an unintended directory path. The regular-file branch already handles truncation; the directory branch should as well.
  uploadstblogs/src/file_operations.c:567
• strftime() can return 0 (buffer too small / format failure). In that case timestamp will be an empty string and the rename logic will proceed with an empty prefix. The non-UploadLogsNow path already checks strftime’s return; this one should too.
  uploadstblogs/docs/lld/uploadSTBLogs_LLD.md:202
• This doc says the SHA256 hex conversion uses a nibble lookup table to avoid per-byte snprintf, but the current implementation in uploadstblogs/src/md5_utils.c uses snprintf in a loop. Either implement the lookup-table approach or update this note to match reality.
  LICENSE:207
• The MIT license text added here still contains the template placeholder YEAR COPYRIGHT HOLDER, which makes the LICENSE file legally ambiguous/incomplete. Replace the placeholders with the correct values (or remove this MIT section if it’s not intended to apply to the repository contents).

                            MIT License

Copyright (c) YEAR COPYRIGHT HOLDER

backup_logs/src/config_manager.c:39

• The property value buffers are only 32 bytes. Realistic LOG_PATH / APP_PERSISTENT_PATH values can exceed that, causing truncation (and potentially incorrect derived paths). Use PATH_MAX (or sizeof(config->log_path)) for these property buffers.

    char log_path_buf[32] = {0};
    char hdd_enabled_buf[32] = {0};
    char app_persistent_path_buf[32] = {0};

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.