RDK-61009 : [RDKE] Port Log Backup Scripts to Source code - #94
RDK-61009 : [RDKE] Port Log Backup Scripts to Source code #94Abhinavpv28 wants to merge 9 commits into
Conversation
This document outlines the low-level design for migrating the backup_logs.sh script to C, detailing data structures, algorithms, and error handling for embedded RDK systems.
There was a problem hiding this comment.
Pull request overview
This PR adds documentation artifacts (requirements, HLD/LLD, and flowcharts) to support migrating backup_logs.sh from shell to a C implementation for embedded RDK targets.
Changes:
- Added functional requirements document for the
backup_logs.sh→ C migration. - Added high-level and low-level design documents describing modules, data structures, and algorithms.
- Added text + Mermaid flowcharts/sequence diagrams for the backup flows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| backup_logs/diagrams/backup_logs_flowcharts.md | Adds text/Mermaid diagrams for main flow, HDD-disabled strategy, sequence, and error handling. |
| backup_logs/backup_logs_requirements.md | Defines functional + non-functional requirements for the planned C port. |
| backup_logs/backup_logs_migration_HLD.md | Describes proposed modular architecture and public interfaces at a high level. |
| backup_logs/backup_logs_LLD.md | Provides detailed pseudo-code/data structures for the planned implementation. |
| int register_cleanup(void (*cleanup_func)(void*), void* resource) { | ||
| cleanup_handler_t* handler = malloc(sizeof(cleanup_handler_t)); | ||
| if (!handler) { | ||
| return -1; | ||
| } | ||
|
|
||
| handler->cleanup_func = cleanup_func; | ||
| handler->resource = resource; | ||
| handler->next = g_cleanup_list; | ||
| g_cleanup_list = handler; |
There was a problem hiding this comment.
The cleanup framework uses malloc/free for each registered handler, which conflicts with the stated goal to minimize/avoid dynamic allocation in embedded environments. Consider a fixed-size handler array/pool (bounded by a max resource count) or make cleanup registration compile-time/static to avoid heap use.
| if (match_txt && (strcasestr(name, ".txt") != NULL)) { | ||
| matches = true; | ||
| } else if (match_log && (strcasestr(name, ".log") != NULL)) { | ||
| matches = true; | ||
| } else if (match_bin && (strcasestr(name, ".bin") != NULL)) { | ||
| matches = true; |
There was a problem hiding this comment.
fileops_find_pattern_optimized uses strcasestr, which is a GNU extension and not portable across all libc/toolchains typically used in embedded builds. Prefer a POSIX approach (fnmatch/glob) or implement a small case-insensitive suffix check locally.
| **Requirements**: | ||
| - Send systemd ready notification upon completion | ||
| - Set systemd status message: "Logs Backup Done..!" | ||
| - Create persistent marker file at `$PERSISTENT_PATH/logFileBackup` |
There was a problem hiding this comment.
This requirement references $PERSISTENT_PATH/logFileBackup, but earlier REQ-001 says the implementation must extract APP_PERSISTENT_PATH. Please align the variable naming (APP_PERSISTENT_PATH vs PERSISTENT_PATH) so the C port reads the correct configuration key and the docs match the actual environment.
| - Create persistent marker file at `$PERSISTENT_PATH/logFileBackup` | |
| - Create persistent marker file at `$APP_PERSISTENT_PATH/logFileBackup` |
| Remove existing last_bootfile | ||
| | | ||
| v | ||
| Check for messages.txt in Previous Logs | ||
| | | ||
| v | ||
| messages.txt exists? |
There was a problem hiding this comment.
The flowchart mentions removing last_bootfile, but the rest of the docs/requirements refer to the last_reboot marker. Please rename this step (and the Mermaid node) to last_reboot for consistency.
| # High-Level Design: backup_logs.sh Migration to C | ||
|
|
||
| ## 1. Overview | ||
|
|
||
| ### 1.1 Purpose | ||
| This document outlines the High-Level Design (HLD) for migrating the existing `backup_logs.sh` shell script to a C implementation. The migration aims to improve performance, reduce memory footprint, and enhance reliability for embedded RDK systems. |
There was a problem hiding this comment.
The PR title suggests the log backup scripts have been ported to source code, but the changes in this PR appear to introduce only requirements/design/diagram documentation (no C implementation or build integration). Please align the PR title/description with the actual scope, or include the corresponding source changes in the same PR.
| file_entry_t files[MAX_FILES_PER_DIR]; | ||
| int count; | ||
| int capacity; |
There was a problem hiding this comment.
The LLD defines file_list_t as file_entry_t files[MAX_FILES_PER_DIR] with MAX_FILES_PER_DIR=10000 and each entry containing multiple MAX_PATH_LEN=4096 buffers. This implies tens/hundreds of MB of static memory, which conflicts with the stated ≤512KB memory requirement and typical embedded constraints; the design should avoid storing full-path arrays per file and instead stream/process entries with small fixed buffers.
| file_entry_t files[MAX_FILES_PER_DIR]; | |
| int count; | |
| int capacity; | |
| /** | |
| * Current file entry being processed. The implementation should | |
| * iterate over directory contents and populate this structure | |
| * one entry at a time instead of pre-allocating all entries. | |
| */ | |
| file_entry_t current_entry; | |
| /** | |
| * Zero-based index of the current entry within the logical list | |
| * of files being processed. | |
| */ | |
| int index; | |
| /** | |
| * Total number of files that are expected to be processed in | |
| * this logical list. This is a logical bound, not a capacity for | |
| * an in-memory array. | |
| */ | |
| int total; |
| // Step 1: Move bak1_ files to base names (bak1_messages.txt -> messages.txt) | ||
| if (fileops_find_pattern(config->prev_log_path, "bak1_*", &file_list) > 0) { | ||
| for (int i = 0; i < file_list.count; i++) { | ||
| // Remove bak1_ prefix | ||
| const char* base_name = file_list.files[i].filename + 5; // Skip "bak1_" |
There was a problem hiding this comment.
backup_rotate_files_hdd_disabled calls fileops_find_pattern(..., "bak1_*"/"bak2_*"/"bak3_*", ...), but the provided pattern-matching algorithm only recognizes *.txt*, *.log*, *.bin*, and bootlog. As written, the rotation steps will never find bak-prefixed files; either extend fileops_find_pattern to support wildcard prefixes (e.g., fnmatch) or use a dedicated prefix match helper.
| // Extract key (trim whitespace) | ||
| size_t key_len = equals - line; | ||
| strncpy(key, line, key_len); | ||
| key[key_len] = '\0'; | ||
| // Trim trailing whitespace from key | ||
| while (key_len > 0 && isspace(key[key_len-1])) { | ||
| key[--key_len] = '\0'; | ||
| } | ||
|
|
||
| // Extract value (handle quotes and expansion) | ||
| char* value_start = equals + 1; | ||
| // Skip leading whitespace | ||
| while (*value_start && isspace(*value_start)) { | ||
| value_start++; | ||
| } | ||
|
|
||
| // Handle quoted values | ||
| if (*value_start == '"' || *value_start == '\'') { | ||
| char quote = *value_start; | ||
| value_start++; | ||
| char* quote_end = strchr(value_start, quote); | ||
| if (quote_end) { | ||
| size_t value_len = quote_end - value_start; | ||
| strncpy(value, value_start, value_len); | ||
| value[value_len] = '\0'; | ||
| } else { | ||
| return -1; // Unterminated quote | ||
| } | ||
| } else { | ||
| // Unquoted value - take until newline or comment | ||
| strcpy(value, value_start); | ||
| char* comment = strchr(value, '#'); |
There was a problem hiding this comment.
config_parse_shell_variable uses strncpy/strcpy without any knowledge of the destination buffer sizes, so long lines can overflow key/value. The parsing helper should take explicit key_size/value_size parameters and use bounded copies (e.g., snprintf/strlcpy if available) while validating truncation.
|
|
||
| // Pre-allocate destination paths to avoid repeated allocations | ||
| char dest_paths[MAX_FILES_PER_DIR][MAX_PATH_LEN]; | ||
|
|
||
| // Prepare all destination paths first | ||
| for (int i = 0; i < total_files; i++) { | ||
| snprintf(dest_paths[i], sizeof(dest_paths[i]), "%s/%s", | ||
| dest_dir, file_list->files[i].filename); | ||
| } | ||
|
|
||
| // Execute moves in batch with progress tracking | ||
| for (int i = 0; i < total_files; i++) { | ||
| if (fileops_move_safe(file_list->files[i].source_path, dest_paths[i]) == 0) { | ||
| success_count++; | ||
| } else { | ||
| LOG_WARN("Failed to move file %d of %d: %s", | ||
| i + 1, total_files, file_list->files[i].filename); | ||
| } | ||
|
|
||
| // Report progress every 100 files for large operations | ||
| if (total_files > 100 && (i + 1) % 100 == 0) { | ||
| LOG_INFO("Moved %d of %d files (%d%%)", success_count, i + 1, | ||
| ((i + 1) * 100) / total_files); | ||
| } | ||
| } | ||
|
|
||
| LOG_INFO("Batch move completed: %d of %d files successful", | ||
| success_count, total_files); | ||
|
|
There was a problem hiding this comment.
fileops_batch_move allocates dest_paths[MAX_FILES_PER_DIR][MAX_PATH_LEN] on the stack, which is enormous (up to ~40MB) and will overflow typical embedded stacks. Prefer computing the destination path per-iteration into a single reusable buffer (or a small fixed pool) instead of preallocating all paths.
| // Pre-allocate destination paths to avoid repeated allocations | |
| char dest_paths[MAX_FILES_PER_DIR][MAX_PATH_LEN]; | |
| // Prepare all destination paths first | |
| for (int i = 0; i < total_files; i++) { | |
| snprintf(dest_paths[i], sizeof(dest_paths[i]), "%s/%s", | |
| dest_dir, file_list->files[i].filename); | |
| } | |
| // Execute moves in batch with progress tracking | |
| for (int i = 0; i < total_files; i++) { | |
| if (fileops_move_safe(file_list->files[i].source_path, dest_paths[i]) == 0) { | |
| success_count++; | |
| } else { | |
| LOG_WARN("Failed to move file %d of %d: %s", | |
| i + 1, total_files, file_list->files[i].filename); | |
| } | |
| // Report progress every 100 files for large operations | |
| if (total_files > 100 && (i + 1) % 100 == 0) { | |
| LOG_INFO("Moved %d of %d files (%d%%)", success_count, i + 1, | |
| ((i + 1) * 100) / total_files); | |
| } | |
| } | |
| LOG_INFO("Batch move completed: %d of %d files successful", | |
| success_count, total_files); | |
| if (total_files > MAX_FILES_PER_DIR) { | |
| LOG_WARN("fileops_batch_move: file count %d exceeds MAX_FILES_PER_DIR (%d); limiting to first %d files", | |
| total_files, MAX_FILES_PER_DIR, MAX_FILES_PER_DIR); | |
| total_files = MAX_FILES_PER_DIR; | |
| } | |
| char dest_path[MAX_PATH_LEN]; | |
| // Execute moves in batch with progress tracking | |
| for (int i = 0; i < total_files; i++) { | |
| int written = snprintf(dest_path, sizeof(dest_path), "%s/%s", | |
| dest_dir, file_list->files[i].filename); | |
| if (written < 0 || written >= (int)sizeof(dest_path)) { | |
| LOG_WARN("Destination path too long for file: %s; skipping", | |
| file_list->files[i].filename); | |
| continue; | |
| } | |
| if (fileops_move_safe(file_list->files[i].source_path, dest_path) == 0) { | |
| success_count++; | |
| } else { | |
| LOG_WARN("Failed to move file %d of %d: %s", | |
| i + 1, total_files, file_list->files[i].filename); | |
| } | |
| // Report progress every 100 files for large operations | |
| if (total_files > 100 && (i + 1) % 100 == 0) { | |
| LOG_INFO("Moved %d of %d files (%d%%)", success_count, i + 1, | |
| ((i + 1) * 100) / total_files); | |
| } | |
| } | |
| LOG_INFO("Batch move completed: %d of %d files successful", | |
| success_count, total_files); |
| void signal_handler(int sig) { | ||
| LOG_INFO("Received signal %d, cleaning up resources", sig); | ||
| execute_all_cleanup(); | ||
| exit(sig); | ||
| } |
There was a problem hiding this comment.
The signal handler calls LOG_INFO, runs cleanup routines, and then calls exit(). These operations are not async-signal-safe and can deadlock or corrupt state when invoked from a signal context. The design should limit the handler to setting a volatile sig_atomic_t flag (or writing to a self-pipe) and perform cleanup from the main loop.
No description provided.