RDK-60497 : Port USB Log Upload Scripts to Source code #64
Conversation
RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scrip…
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive documentation for porting USB Log Upload functionality from shell scripts to C code. The documentation includes requirements specification, high-level design, and detailed flowcharts to guide the implementation of the usbLogUpload feature for embedded systems.
Changes:
- Added requirements document specifying functional requirements, dependencies, constraints, and error handling for USB log upload
- Added high-level design document detailing architecture, module breakdown, data structures, and implementation strategy
- Added flowcharts and diagrams document with detailed process flows, sequence diagrams, and component interactions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| usbLogUpload/docs/usb-log-upload-requirements.md | Defines functional requirements, inputs/outputs, dependencies, constraints, and security considerations for USB log transfer functionality |
| usbLogUpload/docs/usb-log-upload-hld.md | Provides comprehensive high-level design including architecture overview, module breakdown, data structures, interfaces, and implementation strategies |
| usbLogUpload/docs/usb-log-upload-flowcharts.md | Contains detailed flowcharts, sequence diagrams, and component interaction diagrams illustrating the USB log upload process flow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Error Cases**: | ||
| - Exit code 2: USB not mounted | ||
| - Exit code 3: Writing error to USB | ||
| - Exit code 4: Invalid usage or unsupported device |
There was a problem hiding this comment.
The exit code comment states "Exit code 4: Invalid usage or unsupported device", but this doesn't match the flowcharts which show "Exit Code 4: Invalid Usage" and "Exit Code 4: Unsupported Device" as separate conditions. This should be clarified to specify whether exit code 4 is returned for both cases or if they should have distinct codes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ``` | ||
| ALGORITHM: generate_archive_filename() | ||
| INPUT: None (uses system resources) | ||
| OUTPUT: Formatted filename string |
There was a problem hiding this comment.
The timestamp format specified as "mm-dd-yy-HH-MMAM/PM" at line 284 appears to be ambiguous. The format uses "MM" twice (once for month "mm" and once for minutes "MM") with inconsistent casing. This should be clarified to avoid confusion during implementation. Consider using a standard format notation like "MM-DD-YY-hh-mmAM/PM" where MM=month, DD=day, YY=year, hh=hour, mm=minute.
| // Error codes enumeration | ||
| typedef enum { | ||
| USB_LOG_SUCCESS = 0, | ||
| USB_LOG_ERROR_INVALID_ARGS = 4, | ||
| USB_LOG_ERROR_USB_NOT_MOUNTED = 2, | ||
| USB_LOG_ERROR_WRITE_FAILED = 3, | ||
| USB_LOG_ERROR_UNSUPPORTED_DEVICE = 1, | ||
| USB_LOG_ERROR_MEMORY_ALLOCATION = 5, | ||
| USB_LOG_ERROR_CONFIG_LOAD = 6, | ||
| USB_LOG_ERROR_SYSTEM_COMMAND = 7 |
There was a problem hiding this comment.
The error codes enumeration defines USB_LOG_ERROR_INVALID_ARGS with value 1, but according to the requirements document (line 33) and flowcharts, invalid usage should return exit code 4, not 1. This inconsistency between the error enum values and the documented exit codes needs to be resolved. Either update the enum to match the exit codes, or clarify that these are internal error codes that get mapped to exit codes.
| // Error codes enumeration | |
| typedef enum { | |
| USB_LOG_SUCCESS = 0, | |
| USB_LOG_ERROR_INVALID_ARGS = 4, | |
| USB_LOG_ERROR_USB_NOT_MOUNTED = 2, | |
| USB_LOG_ERROR_WRITE_FAILED = 3, | |
| USB_LOG_ERROR_UNSUPPORTED_DEVICE = 1, | |
| USB_LOG_ERROR_MEMORY_ALLOCATION = 5, | |
| USB_LOG_ERROR_CONFIG_LOAD = 6, | |
| USB_LOG_ERROR_SYSTEM_COMMAND = 7 | |
| // Error codes enumeration (values correspond to process exit codes defined in the requirements document) | |
| typedef enum { | |
| USB_LOG_SUCCESS = 0, // Exit code 0: success | |
| USB_LOG_ERROR_UNSUPPORTED_DEVICE = 1, // Exit code 1 | |
| USB_LOG_ERROR_USB_NOT_MOUNTED = 2, // Exit code 2 | |
| USB_LOG_ERROR_WRITE_FAILED = 3, // Exit code 3 | |
| USB_LOG_ERROR_INVALID_ARGS = 4, // Exit code 4: invalid usage / arguments | |
| USB_LOG_ERROR_MEMORY_ALLOCATION = 5, // Exit code 5 | |
| USB_LOG_ERROR_CONFIG_LOAD = 6, // Exit code 6 | |
| USB_LOG_ERROR_SYSTEM_COMMAND = 7 // Exit code 7 |
| - `char *generate_archive_filename(void)` | ||
| - `int compress_logs_to_usb(const char *temp_dir, const char *usb_log_path)` | ||
|
|
||
| > **Note:** The `generate_archive_filename()` function is defined and owned by the Archive Manager |
There was a problem hiding this comment.
In the archive name generation algorithm, the function "generate_archive_filename()" is shown at line 103, but in the algorithm description at line 279, it's referred to as "generate_archive_filename()". However, in section 3.1.4 line 103, the function is listed under Archive Manager Module. Ensure consistency in function naming and module ownership across all references in the document.
| - `char *generate_archive_filename(void)` | |
| - `int compress_logs_to_usb(const char *temp_dir, const char *usb_log_path)` | |
| > **Note:** The `generate_archive_filename()` function is defined and owned by the Archive Manager | |
| - `char *generate_archive_name(void)` | |
| - `int compress_logs_to_usb(const char *temp_dir, const char *usb_log_path)` | |
| > **Note:** The `generate_archive_name()` function is defined and owned by the Archive Manager |
| Main->>FileMgr: generate_archive_filename() | ||
| FileMgr-->>Main: archive_filename |
There was a problem hiding this comment.
In the Main Process Sequence diagram, line 189 shows "generate_archive_filename()" being called on FileMgr (File Manager), but according to the HLD document section 3.1.4 (line 103), this function belongs to the Archive Manager Module, not the File Manager. This inconsistency should be corrected to show the call going to ArchMgr instead of FileMgr.
| Main->>FileMgr: generate_archive_filename() | |
| FileMgr-->>Main: archive_filename | |
| Main->>ArchMgr: generate_archive_filename() | |
| ArchMgr-->>Main: archive_filename |
| **Temporary directory requirements** | ||
|
|
||
| - `TEMP_DIR_PREFIX` defines the base directory used for staging temporary log files prior to archival. | ||
| - The implementation MUST, before first use: |
There was a problem hiding this comment.
The temporary directory requirements specify that the directory should be created with mode 0700 (owner-only access) for security. However, the constant definition doesn't include a corresponding MAX_PERMISSIONS or TEMP_DIR_MODE constant. Consider adding a constant like TEMP_DIR_MODE with value 0700 to ensure consistent implementation.
|
|
||
| // Configuration interface | ||
| int usb_log_load_config(usb_log_config_t *config); | ||
| int usb_log_get_property(const char *name, char *value, size_t value_size); |
There was a problem hiding this comment.
The function signature at line 332 uses mode_t for the mode parameter, but this type is not defined in the constants section. Consider adding a note in the data structures or constants section about platform-specific types like mode_t to ensure implementers understand these are POSIX standard types.
| - **Error Cases**: | ||
| - Exit code 2: USB not mounted | ||
| - Exit code 3: Writing error to USB | ||
| - Exit code 4: Invalid usage or unsupported device |
There was a problem hiding this comment.
The requirements document states "Exit code 4: Invalid usage or unsupported device" (line 33), but the flowchart shows two separate exit points with code 4: one for "Invalid Usage" (line 16) and one for "Unsupported Device" (line 24). While both use exit code 4, it would be helpful to clarify in the requirements or error handling documentation whether there's a way to distinguish between these two failure modes through error messages, as they represent different root causes.
| - Exit code 4: Invalid usage or unsupported device | |
| - Exit code 4: Invalid usage or unsupported device (distinguished via error messages/log entries: "Invalid usage" vs "Unsupported device") |
|
|
||
| ### Core Functionality | ||
| 1. **USB Log Transfer**: Transfer system logs from embedded device to external USB storage | ||
| 2. **Device Validation**: Verify device compatibility (currently PLATCO devices only) |
There was a problem hiding this comment.
The requirements document mentions "PLATCO devices only" at line 10, but there's no explanation of what PLATCO stands for or what makes a device qualify as PLATCO. This could be confusing for implementers who are not familiar with the device classification. Consider adding a brief explanation or reference to device type documentation.
| 2. **Device Validation**: Verify device compatibility (currently PLATCO devices only) | |
| 2. **Device Validation**: Verify device compatibility (currently limited to devices classified as **PLATCO**; see internal device-type/classification documentation for the definition of PLATCO devices and supported models) |
|
|
||
| ValidateArgs -->|Yes| LoadConfig[Load System Configuration] | ||
| LoadConfig --> ConfigOK{Configuration Loaded?} | ||
| ConfigOK -->|No| Exit6[Exit Code 6: Config Error] |
There was a problem hiding this comment.
The flowchart shows "Exit Code 6: Config Error" at line 20, but this exit code (6) is not documented in the requirements document (usb-log-upload-requirements.md lines 30-33), which only lists exit codes 0, 2, 3, and 4. Exit code 6 is defined in the HLD error enum as USB_LOG_ERROR_CONFIG_LOAD. This exit code should be added to the requirements document's "Error Cases" section for completeness.
| ConfigOK -->|No| Exit6[Exit Code 6: Config Error] | |
| ConfigOK -->|No| Exit4_Config[Exit Code 4: Config Error] |
…-agent into feature/RDK-60497
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 22 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
usbLogUpload/include/usb_log_main.h
Outdated
| void print_usage(const char *program_name); | ||
|
|
There was a problem hiding this comment.
The print_usage function is declared in the header but is never defined or implemented. This will cause a linker error when the test code attempts to call this function.
| void print_usage(const char *program_name); | |
| static inline void print_usage(const char *program_name) | |
| { | |
| const char *name = (program_name != NULL) ? program_name : "usb_log_upload"; | |
| fprintf(stderr, "Usage: %s <usb_mount_point>\n", name); | |
| fprintf(stderr, "Upload logs to the specified USB mount point.\n"); | |
| fprintf(stderr, "Exit codes:\n"); | |
| fprintf(stderr, " %d - Success\n", USB_LOG_SUCCESS); | |
| fprintf(stderr, " %d - General error\n", USB_LOG_ERROR_GENERAL); | |
| fprintf(stderr, " %d - USB not mounted\n", USB_LOG_ERROR_USB_NOT_MOUNTED); | |
| fprintf(stderr, " %d - Write error\n", USB_LOG_ERROR_WRITE_ERROR); | |
| fprintf(stderr, " %d - Invalid usage\n", USB_LOG_ERROR_INVALID_USAGE); | |
| } |
| #include <string.h> | ||
| #include <unistd.h> | ||
| #include <sys/stat.h> | ||
| #include "../uploadstblogs/include/file_operations.h" |
There was a problem hiding this comment.
The include path uses a relative path to access headers from a sibling directory. This assumes a specific directory structure and will break if the directories are reorganized. Consider using a more robust include path configuration through the build system or defining a proper include directory structure.
| #include "../uploadstblogs/include/file_operations.h" | |
| #include "file_operations.h" |
| /* RDK utility constants */ | ||
| #ifndef UTILS_SUCCESS | ||
| #define UTILS_SUCCESS 1 | ||
| #endif | ||
| #ifndef UTILS_FAIL | ||
| #define UTILS_FAIL -1 | ||
| #endif | ||
|
|
There was a problem hiding this comment.
The UTILS_SUCCESS and UTILS_FAIL macros are defined in both usb_log_utils.c and usb_log_utils.h. This creates redundancy and potential inconsistency. These definitions should exist only in the header file to maintain a single source of truth and avoid redefinition issues.
| /* RDK utility constants */ | |
| #ifndef UTILS_SUCCESS | |
| #define UTILS_SUCCESS 1 | |
| #endif | |
| #ifndef UTILS_FAIL | |
| #define UTILS_FAIL -1 | |
| #endif |
| TEST_F(UsbLogValidationTest, UsbMountPointValidTest) { | ||
| // TODO: Test validate_usb_mount_point with valid path | ||
| const char* valid_path = "/tmp"; | ||
| EXPECT_EQ(validate_usb_mount_point(valid_path), 0); | ||
| } |
There was a problem hiding this comment.
The test expects validate_usb_mount_point to return 0 for "/tmp", which assumes /tmp exists and is accessible. While /tmp typically exists on most systems, the test should document this assumption or use a more controlled test environment. Additionally, the test should verify the actual validation logic rather than relying on system state.
| rmdir(test_usb_path); | ||
| rmdir(test_temp_path); |
There was a problem hiding this comment.
The TearDown method attempts to remove directories that may not have been created during the test. The rmdir calls will fail if the directories don't exist, but the code doesn't check the return value or handle errors. Consider checking if directories exist before attempting to remove them, or use a more robust cleanup mechanism that doesn't generate spurious errors.
| rmdir(test_usb_path); | |
| rmdir(test_temp_path); | |
| if (test_usb_path != nullptr && access(test_usb_path, F_OK) == 0) { | |
| rmdir(test_usb_path); | |
| } | |
| if (test_temp_path != nullptr && access(test_temp_path, F_OK) == 0) { | |
| rmdir(test_temp_path); | |
| } |
| #endif | ||
|
|
||
| /* RDK Logging component name for USB Log Upload */ | ||
| #define LOG_USB_UPLOAD "LOG.RDK.USBLOGUPLOAD" |
There was a problem hiding this comment.
The LOG_USB_UPLOAD macro is defined in both usb_log_utils.c and usb_log_utils.h. This creates redundancy and potential inconsistency if the values differ. The definition should exist only in the header file to maintain a single source of truth.
| #define LOG_USB_UPLOAD "LOG.RDK.USBLOGUPLOAD" |
| TEST_F(UsbLogMainTest, ExecuteWithInvalidInputTest) { | ||
| // TODO: Test usb_log_upload_execute with NULL input | ||
| EXPECT_NE(usb_log_upload_execute(nullptr), USB_LOG_SUCCESS); | ||
| } |
There was a problem hiding this comment.
The test is calling usb_log_upload_execute with nullptr (NULL pointer), but there's no null check implementation in the function. Since the function is currently a stub returning success, this test will pass even though the actual implementation would need to properly handle and reject null input. The test should be updated once the function is implemented to verify proper null pointer handling.
| TEST_F(UsbLogMainTest, PrintUsageTest) { | ||
| // TODO: Test print_usage function | ||
| EXPECT_NO_THROW(print_usage("test_program")); | ||
| } |
There was a problem hiding this comment.
The test calls print_usage which is declared in usb_log_main.h but never implemented. This will cause a linker error when building the unit tests.
| > **Note:** The `generate_archive_filename()` function is defined and owned by the Archive Manager | ||
| > module. Other modules (e.g., the File Manager) may invoke this function in their workflows, as | ||
| > reflected in sequence diagrams, but they do not implement or own it. |
There was a problem hiding this comment.
The HLD document states that generate_archive_filename is owned by the Archive Manager module, but the File Manager module's sequence diagram (lines 189-190 in the flowcharts doc) shows File Manager calling this function. This creates confusion about ownership and responsibility. The implementation should clarify which module owns this function or update the documentation to reflect actual usage patterns.
| > **Note:** The `generate_archive_filename()` function is defined and owned by the Archive Manager | |
| > module. Other modules (e.g., the File Manager) may invoke this function in their workflows, as | |
| > reflected in sequence diagrams, but they do not implement or own it. | |
| > **Note:** The `generate_archive_filename()` function is defined and owned exclusively by the | |
| > Archive Manager module. Other modules (e.g., the File Manager) may invoke this function in their | |
| > workflows (as shown in the flowcharts/sequence diagrams, e.g., lines 189–190 in the flowcharts | |
| > document), but those diagrams represent usage only; they do not change the ownership or | |
| > implementation responsibility, which remain with the Archive Manager. |
| if (!create_directory(temp_dir_path)) { | ||
| /* Get timestamp for logging */ | ||
| if (get_current_timestamp(timestamp_buf, sizeof(timestamp_buf)) != 0) { | ||
| strcpy(timestamp_buf, "00/00/00-00:00:00"); |
There was a problem hiding this comment.
The function uses strcpy with a string literal which could be unsafe if timestamp_buf is modified. While the buffer is large enough in this case, consider using strncpy with explicit size bounds for consistency with the safe coding practices mentioned in the project guidelines.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @brief Move log files from source to destination | ||
| * | ||
| * Moves all files from LOG_PATH to temp directory. | ||
| * Matches shell script: mv $LOG_PATH/ * $USB_DIR/. |
There was a problem hiding this comment.
The comment on line 77 contains inconsistent formatting. The shell script reference "mv $LOG_PATH/ * $USB_DIR/." has an extra space before the asterisk which makes it unclear. Should be "mv $LOG_PATH/* $USB_DIR/." for clarity.
| * Matches shell script: mv $LOG_PATH/ * $USB_DIR/. | |
| * Matches shell script: mv $LOG_PATH/* $USB_DIR/. |
usbLogUpload/build.sh
Outdated
| # Generate configure script if it doesn't exist | ||
| if [ ! -f configure ]; then | ||
| print_status "Generating configure script..." | ||
| autoreconf -fiv |
There was a problem hiding this comment.
The shell script uses 'autoreconf -fiv' without checking if autoconf/automake packages are properly installed. If autoreconf exists but the necessary m4 macros or other autotools components are missing, the script will fail with unclear errors. Consider adding a more comprehensive check for autotools dependencies or providing better error messages.
| # Generate configure script if it doesn't exist | |
| if [ ! -f configure ]; then | |
| print_status "Generating configure script..." | |
| autoreconf -fiv | |
| # Verify required autotools components | |
| MISSING_AUTOTOOLS=() | |
| for TOOL in autoconf automake aclocal; do | |
| if ! command -v "$TOOL" &> /dev/null; then | |
| MISSING_AUTOTOOLS+=("$TOOL") | |
| fi | |
| done | |
| if [ "${#MISSING_AUTOTOOLS[@]}" -ne 0 ]; then | |
| print_error "Missing required autotools components: ${MISSING_AUTOTOOLS[*]}" | |
| print_error "Please install the missing packages (e.g., autoconf, automake) and retry." | |
| exit 1 | |
| fi | |
| # Generate configure script if it doesn't exist | |
| if [ ! -f configure ]; then | |
| print_status "Generating configure script..." | |
| if ! autoreconf -fiv; then | |
| print_error "autoreconf failed. Please ensure all required autotools packages and m4 macros are installed." | |
| exit 1 | |
| fi |
| /* RDK utility constants */ | ||
| #ifndef UTILS_SUCCESS | ||
| #define UTILS_SUCCESS 1 | ||
| #endif | ||
| #ifndef UTILS_FAIL | ||
| #define UTILS_FAIL -1 | ||
| #endif | ||
|
|
There was a problem hiding this comment.
The constants UTILS_SUCCESS and UTILS_FAIL are defined here (lines 40-45) but are also defined in usb_log_utils.h (lines 34-39). This creates duplicate definitions which could lead to maintenance issues if the values ever need to change. Consider removing these definitions from usb_log_utils.c and relying on the common_device_api.h header that is already included via usb_log_utils.h, or ensure only one location defines these constants.
| /* RDK utility constants */ | |
| #ifndef UTILS_SUCCESS | |
| #define UTILS_SUCCESS 1 | |
| #endif | |
| #ifndef UTILS_FAIL | |
| #define UTILS_FAIL -1 | |
| #endif |
| if (get_current_timestamp(timestamp_buf, sizeof(timestamp_buf)) != 0) { | ||
| strcpy(timestamp_buf, "00/00/00-00:00:00"); | ||
| } |
There was a problem hiding this comment.
The strcpy on line 210 is used without bounds checking. While the fallback string is short, using strcpy is unsafe practice. Use strncpy with explicit null termination or snprintf instead for safer string handling.
| if (get_current_timestamp(timestamp_buf, sizeof(timestamp_buf)) != 0) { | ||
| strcpy(timestamp_buf, "00/00/00-00:00:00"); | ||
| } |
There was a problem hiding this comment.
Another strcpy usage on line 226. Same security concern - use strncpy with explicit null termination or snprintf for safer string operations.
| TEST_F(UsbLogFileManagerTest, EnsureDirectoryExistsTest) { | ||
| // TODO: Test ensure_directory_exists | ||
| EXPECT_EQ(ensure_directory_exists(test_temp_path), 0); | ||
| } |
There was a problem hiding this comment.
The function name 'ensure_directory_exists' is referenced in the test on line 93, but this function is not defined in the usb_log_file_manager module. The actual functions available are 'create_usb_log_directory' and 'create_temporary_directory'. This test will fail to compile. Either implement 'ensure_directory_exists' or update the test to use the correct function name.
| RDK_LOG(RDK_LOG_INFO, LOG_USB_UPLOAD, | ||
| "[%s:%d] Moved %d of %d files from %s to %s\n", | ||
| __FUNCTION__, __LINE__, moved_count, file_count, source_path, dest_path); | ||
|
|
There was a problem hiding this comment.
The function 'move_log_files' iterates through directory entries and attempts to move files, but if a rename() fails partway through, the function continues to process remaining files and returns 0 (success). This means some files may be successfully moved while others fail, and the caller won't know that a partial failure occurred. Consider returning an error code if any file fails to move, or at minimum logging a summary of failures.
| if (file_count != moved_count) { | |
| /* Indicate partial or complete failure to move files */ | |
| return -3; | |
| } |
| - `DEVICE_NAME`: Device type identifier (must be "PLATCO") | ||
| - `RDK_PATH`: RDK library path (default: `/lib/rdk`) |
There was a problem hiding this comment.
The configuration section mentions that DEVICE_NAME must be "PLATCO", but this is a very specific device constraint that may not be clear to users. Consider adding more context about why only PLATCO is supported and what happens on other devices (the tool will exit with error code 4).
| char timestamp_buf[32]; | ||
|
|
||
| /* Get timestamp for logging */ | ||
| if (get_current_timestamp(timestamp_buf, sizeof(timestamp_buf)) != 0) { | ||
| strcpy(timestamp_buf, "00/00/00-00:00:00"); | ||
| } |
There was a problem hiding this comment.
The comment on line 77 states "Get timestamp for logging", but if get_current_timestamp fails, the code uses strcpy to set a fallback. This pattern is repeated multiple times in the codebase. Consider creating a helper function that returns a guaranteed valid timestamp string to reduce code duplication and improve maintainability.
usbLogUpload/Makefile.am
Outdated
| -lpthread | ||
|
|
||
| usblogupload_LDFLAGS = \ | ||
| -L${top_srcdir}/uploadstblogs/src/libuploadstblogs.la \ |
There was a problem hiding this comment.
Missing line continuation backslash at the end of line 48. This will cause a syntax error in the Makefile. The line should end with a backslash to continue to line 49.
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char syslog_enabled[64]; | ||
| char log_path[256]; | ||
| char timestamp_buf[32]; | ||
|
|
||
| /* Check if SYSLOG_NG_ENABLED is set to "true" */ | ||
| memset(syslog_enabled, 0, sizeof(syslog_enabled)); | ||
| if (getDevicePropertyData("SYSLOG_NG_ENABLED", syslog_enabled, sizeof(syslog_enabled)) != UTILS_SUCCESS) { | ||
| /* SYSLOG_NG_ENABLED not found, skip reload */ | ||
| return 0; | ||
| } | ||
|
|
||
| if (strcmp(syslog_enabled, "true") != 0) { | ||
| /* SYSLOG_NG_ENABLED is not "true", skip reload */ | ||
| return 0; | ||
| } | ||
|
|
||
| /* Get LOG_PATH for logging */ | ||
| memset(log_path, 0, sizeof(log_path)); | ||
| if (getIncludePropertyData("LOG_PATH", log_path, sizeof(log_path)) != UTILS_SUCCESS) { | ||
| strncpy(log_path, "/opt/logs", sizeof(log_path) - 1); | ||
| } | ||
|
|
There was a problem hiding this comment.
In reload_syslog_service(), log_path is populated but never used, which will trigger -Wunused-but-set-variable under -Wall -Wextra. Remove it or use it in the log message so the build stays warning-clean.
usbLogUpload/build.sh
Outdated
| if [ "$RUN_TESTS" = true ]; then | ||
| print_status "Building and running tests..." | ||
|
|
||
| if [ -f Makefile ]; then | ||
| make test | ||
|
|
||
| # Run the test executable if it exists | ||
| if [ -f bin/test_usblogupload ]; then | ||
| print_status "Running integration tests..." | ||
| ./bin/test_usblogupload | ||
| fi | ||
|
|
||
| # Run GTest unit tests if available | ||
| if [ -d unittest ] && command -v g++ &> /dev/null; then | ||
| print_status "Building and running unit tests..." | ||
| cd unittest | ||
| # Note: This would require a proper unittest Makefile | ||
| print_warning "Unit test build system not yet implemented" | ||
| cd .. | ||
| fi | ||
| else | ||
| print_error "No Makefile found for testing" | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The -t/--test path runs make test, but there is no test target defined for this module (and set -e will make the script fail). Either provide a test target (or use Automake’s standard make check) and ensure it builds/runs the intended test binaries, or update the script to call the correct existing target(s).
| /* Build full archive path: $USB_LOG/$LOG_FILE */ | ||
| snprintf(archive_path, sizeof(archive_path), "%s/%s", usb_log_dir, log_file); | ||
|
|
There was a problem hiding this comment.
The snprintf() that builds archive_path doesn’t check for truncation. If usb_log_dir/log_file are long, you can silently produce an invalid path and then fail later (or write to an unexpected location). Check the return value and handle the overflow case explicitly.
| TEST_F(UsbLogValidationTest, DeviceCompatibilityInvalidTest) { | ||
| // TODO: Test validate_device_compatibility with non-PLATCO device | ||
| // This would require mocking environment variables or config | ||
| EXPECT_TRUE(true); // Placeholder | ||
| } |
There was a problem hiding this comment.
DeviceCompatibilityInvalidTest is a placeholder (EXPECT_TRUE(true)) and doesn’t validate any behavior. Either implement it with proper mocking of getDevicePropertyData() to simulate an unsupported device and assert the expected exit code, or remove the test until it can exercise real logic.
| TEST_F(UsbLogMainTest, MainArgumentValidationTest) { | ||
| // TODO: Test main function with various argument combinations | ||
| char* test_argv[] = {(char*)"usblogupload", (char*)"/tmp/test_usb"}; | ||
| // This would require refactoring main to be testable | ||
| EXPECT_TRUE(true); // Placeholder | ||
| } No newline at end of file |
There was a problem hiding this comment.
MainArgumentValidationTest is a placeholder (EXPECT_TRUE(true)) and provides no coverage of argument handling. If main() can’t be tested directly, consider refactoring argument parsing into a helper function and unit test that helper instead.
| * @brief Test usb_log_upload_execute with valid input | ||
| */ | ||
| TEST_F(UsbLogMainTest, ExecuteWithValidInputTest) { | ||
| // TODO: Test usb_log_upload_execute with valid input | ||
| const char* test_mount = "/tmp/test_usb"; | ||
| // This test would require mocking filesystem operations | ||
| EXPECT_EQ(usb_log_upload_execute(test_mount), USB_LOG_SUCCESS); |
There was a problem hiding this comment.
This test expects usb_log_upload_execute() to succeed but does not create/mount the provided path or mock filesystem operations; with the current implementation it will typically return an error (e.g., USB not mounted). Either create the required temp directories/files as part of the fixture setup or mock validate_usb_mount_point/file operations so the test is deterministic.
| * @brief Test usb_log_upload_execute with valid input | |
| */ | |
| TEST_F(UsbLogMainTest, ExecuteWithValidInputTest) { | |
| // TODO: Test usb_log_upload_execute with valid input | |
| const char* test_mount = "/tmp/test_usb"; | |
| // This test would require mocking filesystem operations | |
| EXPECT_EQ(usb_log_upload_execute(test_mount), USB_LOG_SUCCESS); | |
| * @brief Test usb_log_upload_execute behavior with a non-existent mount point | |
| */ | |
| TEST_F(UsbLogMainTest, ExecuteWithValidInputTest) { | |
| // NOTE: A true "valid input" success-path test would require mocking | |
| // filesystem operations or setting up an actual USB mount point. | |
| // Here we instead use an obviously invalid path and verify that the call | |
| // does not report success, which is deterministic across environments. | |
| const char* test_mount = "/nonexistent_mount_for_test"; | |
| EXPECT_NE(usb_log_upload_execute(test_mount), USB_LOG_SUCCESS); |
| TEST_F(UsbLogFileManagerTest, MoveLogFilesTest) { | ||
| // TODO: Test move_log_files | ||
| const char* source = "/tmp/source"; | ||
| const char* dest = "/tmp/dest"; | ||
| EXPECT_EQ(move_log_files(source, dest), 0); | ||
| } |
There was a problem hiding this comment.
MoveLogFilesTest expects success but doesn’t create the source/destination directories or any files. As written, move_log_files() will fail to opendir(source) and return an error. Set up the directories/files within the test (or mock filesystem APIs) and assert the correct behavior (including partial failures).
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { | ||
| file_count++; | ||
|
|
||
| /* Move file using rename() */ | ||
| if (rename(src_file, dst_file) == 0) { | ||
| moved_count++; | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_USB_UPLOAD, | ||
| "[%s:%d] Moved: %s\n", __FUNCTION__, __LINE__, entry->d_name); | ||
| } else { | ||
| /* Log warning but continue with other files */ | ||
| RDK_LOG(RDK_LOG_WARN, LOG_USB_UPLOAD, | ||
| "[%s:%d] Failed to move %s: %s\n", | ||
| __FUNCTION__, __LINE__, entry->d_name, strerror(errno)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| closedir(dir); | ||
|
|
||
| RDK_LOG(RDK_LOG_INFO, LOG_USB_UPLOAD, | ||
| "[%s:%d] Moved %d of %d files from %s to %s\n", | ||
| __FUNCTION__, __LINE__, moved_count, file_count, source_path, dest_path); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
move_log_files() always returns success even when individual file moves fail (it only logs warnings). This can lead to creating an archive with missing logs while still returning 0. Track failures and return a non-zero/appropriate error code if any file fails to move (and consider whether non-regular files like symlinks should be moved to match the original mv behavior).
| void SetUp() override { | ||
| // Setup for each test case | ||
| test_usb_path = "/tmp/test_usb"; | ||
| test_temp_path = "/tmp/test_temp"; | ||
| } |
There was a problem hiding this comment.
The fixture uses hard-coded paths under /tmp (e.g., /tmp/test_usb, /tmp/test_temp). This can cause cross-test interference and flakiness when tests run in parallel or when directories already exist. Prefer creating a unique temporary directory per test run (e.g., via mkdtemp) and cleaning it up in TearDown().
| /* Move the archive from source_dir to the USB destination */ | ||
| if (rename(temp_archive_path, archive_path) != 0) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] %s USB WRITING ERROR - Failed to move archive to %s: %s\n", | ||
| __FUNCTION__, __LINE__, timestamp_buf, archive_path, strerror(errno)); | ||
| return 3; /* Exit code 3: Writing Error */ | ||
| } |
There was a problem hiding this comment.
rename(temp_archive_path, archive_path) will fail with EXDEV when moving the archive from the staging directory (typically on the rootfs) to the USB mount (different filesystem). Handle cross-filesystem moves by falling back to a copy+fsync+unlink approach (or use an existing helper from uploadstblogs if available).
usbLogUpload/src/usb_log_archive.c
Outdated
| __FUNCTION__, __LINE__, timestamp_buf, archive_path); | ||
|
|
||
| /* Initialize minimal runtime context for archive creation */ | ||
| RuntimeContext ctx; |
There was a problem hiding this comment.
Coverity Issue - Large stack use
Local variable "ctx" uses 10136 bytes of stack space, which exceeds the maximum single use of 10000 bytes.
Low Impact, CWE-400
STACK_USE
|
|
||
| /* Check if it's a regular file (skip directories) */ | ||
| struct stat st; | ||
| if (stat(src_file, &st) == 0 && S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "stat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!usb_log_path) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] Invalid parameter\n", __FUNCTION__, __LINE__); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Check if directory already exists */ | ||
| if (dir_exists(usb_log_path)) { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_USB_UPLOAD, | ||
| "[%s:%d] USB log directory already exists: %s\n", | ||
| __FUNCTION__, __LINE__, usb_log_path); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Create directory (mkdir -p behavior) */ | ||
| if (!create_directory(usb_log_path)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] Failed to create USB log directory: %s\n", | ||
| __FUNCTION__, __LINE__, usb_log_path); | ||
| return -2; | ||
| } |
There was a problem hiding this comment.
Failure paths return negative values (-1/-2), but the executable forwards these as process exit codes (e.g., main() returns usb_log_upload_execute() result). Negative returns become 255/254 at the shell level and do not match the documented/script exit codes (2/3/4). Consider returning USB_LOG_ERROR_WRITE_ERROR/USB_LOG_ERROR_INVALID_USAGE (or mapping errors in usb_log_upload_execute) instead of negative codes.
| #include "uploadstblogs_types.h" | ||
| #include <sys/stat.h> | ||
| #include <string.h> | ||
| #include <errno.h> |
There was a problem hiding this comment.
snprintf() is used in this file but <stdio.h> is not included, which can trigger implicit-declaration errors/warnings under -std=c99 toolchains. Add the correct standard header to ensure portable builds.
| #include <errno.h> | |
| #include <errno.h> | |
| #include <stdio.h> |
| FILE *pid_fp = popen("pidof syslog-ng", "r"); | ||
| if (!pid_fp) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] Failed to find syslog-ng process\n", __FUNCTION__, __LINE__); | ||
| return -1; | ||
| } | ||
|
|
||
| char pid_str[32]; | ||
| if (!fgets(pid_str, sizeof(pid_str), pid_fp)) { | ||
| pclose(pid_fp); | ||
| RDK_LOG(RDK_LOG_WARN, LOG_USB_UPLOAD, | ||
| "[%s:%d] syslog-ng process not found\n", __FUNCTION__, __LINE__); | ||
| return 0; /* Not an error - service may not be running */ | ||
| } | ||
| pclose(pid_fp); | ||
|
|
||
| /* Convert PID string to integer */ | ||
| pid_t syslog_pid = (pid_t)atoi(pid_str); | ||
| if (syslog_pid <= 0) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] Invalid syslog-ng PID: %s\n", __FUNCTION__, __LINE__, pid_str); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Send SIGHUP signal using kill() */ | ||
| if (kill(syslog_pid, SIGHUP) == 0) { | ||
| RDK_LOG(RDK_LOG_INFO, LOG_USB_UPLOAD, | ||
| "[%s:%d] %s syslog-ng reloaded successfully\n", | ||
| __FUNCTION__, __LINE__, timestamp_buf); | ||
|
|
||
| return 0; | ||
| } else { |
There was a problem hiding this comment.
pidof syslog-ng can return multiple PIDs; atoi(pid_str) will only signal the first one. If multiple syslog-ng processes exist, the reload may be incomplete. Consider tokenizing the PID list and sending SIGHUP to each PID (or otherwise matching the original killall -HUP syslog-ng behavior).
| a. COPY file to dest_path | ||
| b. VERIFY copy successful | ||
| c. DELETE source file | ||
| 4. RETURN success/failure status |
There was a problem hiding this comment.
The documented file-movement algorithm here describes copy+verify+delete semantics, but the implementation uses rename() and also treats per-file failures as non-fatal. If the implementation is intended to match the HLD, update the code accordingly; otherwise, update this algorithm description to reflect the actual behavior and its failure handling.
| a. COPY file to dest_path | |
| b. VERIFY copy successful | |
| c. DELETE source file | |
| 4. RETURN success/failure status | |
| a. ATTEMPT to MOVE file from source_path to dest_path using rename() | |
| b. IF move fails: | |
| i. LOG failure for this file | |
| ii. CONTINUE to next file (per-file failures are non-fatal) | |
| 4. RETURN status indicating overall result (e.g., success if at least one file moved, failure if none moved) |
| /* Move file using rename() */ | ||
| if (rename(src_file, dst_file) == 0) { | ||
| moved_count++; | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_USB_UPLOAD, | ||
| "[%s:%d] Moved: %s\n", __FUNCTION__, __LINE__, entry->d_name); | ||
| } else { | ||
| /* Log warning but continue with other files */ | ||
| RDK_LOG(RDK_LOG_WARN, LOG_USB_UPLOAD, | ||
| "[%s:%d] Failed to move %s: %s\n", | ||
| __FUNCTION__, __LINE__, entry->d_name, strerror(errno)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| closedir(dir); | ||
|
|
||
| RDK_LOG(RDK_LOG_INFO, LOG_USB_UPLOAD, | ||
| "[%s:%d] Moved %d of %d files from %s to %s\n", | ||
| __FUNCTION__, __LINE__, moved_count, file_count, source_path, dest_path); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
move_log_files() always returns 0 even if one or more rename() operations fail (it only logs a warning). This can silently produce incomplete archives while reporting success. Track failures and return a non-zero/"write error" exit code when any file cannot be moved.
| { | ||
| /* Check if mount point parameter is valid */ | ||
| if (!mount_point || mount_point[0] == '\0') { | ||
| return -1; |
There was a problem hiding this comment.
For a NULL/empty mount point this returns -1, which usb_log_upload_execute() will propagate as a process exit code of 255. Since usb_log_upload_execute() is a public API, consider returning a defined exit code (e.g., 4 invalid usage) for invalid parameters to keep behavior consistent.
| return -1; | |
| return 4; |
| ```mermaid | ||
| flowchart TD | ||
| A[Start] --> B[Parse Arguments] | ||
| B --> C{Valid Arguments?} | ||
| C -->|No| D[Print Usage & Exit 4] | ||
| C -->|Yes| E[Load Configuration] | ||
| E --> F{Config Loaded?} | ||
| F -->|No| G[Exit 6] | ||
| F -->|Yes| H[Validate Device] | ||
| H --> I{Device Supported?} | ||
| I -->|No| J[Exit 4] | ||
| I -->|Yes| K[Validate USB Mount] | ||
| K --> L{USB Available?} | ||
| L -->|No| M[Exit 2] | ||
| L -->|Yes| N[Create USB Log Dir] | ||
| N --> O[Generate Archive Name] | ||
| O --> P[Create Temp Directory] | ||
| P --> Q{Temp Dir Created?} | ||
| Q -->|No| R[Exit 3] | ||
| Q -->|Yes| S[Move Log Files] | ||
| S --> T[Reload Syslog Service] | ||
| T --> U[Create Archive] | ||
| U --> V{Archive Created?} | ||
| V -->|No| W[Exit 3] | ||
| V -->|Yes| X[Cleanup Temp Files] | ||
| X --> Y[Sync Filesystem] | ||
| Y --> Z[Exit 0] | ||
| ``` |
There was a problem hiding this comment.
This HLD flow and error-code enum specify a config load step and an Exit 6 path (USB_LOG_ERROR_CONFIG_LOAD), but the current implementation does not load configuration nor return 6 anywhere. Either align the design doc to the current implementation or implement the described configuration-loading/error behavior to avoid misleading readers.
|
|
||
| /* Check if it's a regular file and not a symlink (skip directories and symlinks) */ | ||
| struct stat st; | ||
| if (lstat(src_file, &st) == 0 && S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "lstat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
|
|
||
| /* Check if it's a regular file and not a symlink (skip directories and symlinks) */ | ||
| struct stat st; | ||
| if (lstat(src_file, &st) == 0 && S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) { |
There was a problem hiding this comment.
Coverity Issue - Time of check time of use
Calling function "lstat" to perform check on "src_file".
Low Impact, CWE-367
TOCTOU
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @return int 0 on success, negative error code on failure | ||
| */ | ||
| int create_temporary_directory(const char *file_name, char *temp_dir_path, size_t buffer_size) | ||
| { |
There was a problem hiding this comment.
create_temporary_directory() uses file_name in snprintf without validating it is non-NULL/non-empty. Add input validation at the start of the function and return an appropriate error code before dereferencing file_name.
| { | |
| { | |
| if (!file_name || !temp_dir_path || buffer_size == 0 || file_name[0] == '\0') { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] Invalid parameter\n", __FUNCTION__, __LINE__); | |
| return -1; | |
| } |
| FILE *pid_fp = popen("pidof syslog-ng", "r"); | ||
| if (!pid_fp) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] Failed to find syslog-ng process\n", __FUNCTION__, __LINE__); | ||
| return -1; |
There was a problem hiding this comment.
reload_syslog_service() relies on popen("pidof syslog-ng"), adding an external command dependency and spawning a shell. For embedded targets this is often unavailable/undesirable; prefer a non-shell approach (e.g., enumerating /proc or using an existing platform service API).
| /* Check if mount point parameter is valid */ | ||
| if (!mount_point || mount_point[0] == '\0') { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
validate_usb_mount_point() returns -1 for NULL/empty mount points, but the rest of the flow treats validation failures as script-style exit codes (e.g., 2 for “USB not mounted”). Returning a negative code here can leak out as an unexpected process exit status; consider returning USB_LOG_ERROR_INVALID_USAGE (4) or USB_LOG_ERROR_USB_NOT_MOUNTED (2) consistently.
| * @brief Validate device compatibility | ||
| * | ||
| * @return int 0 if compatible, negative error code otherwise | ||
| */ |
There was a problem hiding this comment.
The API contract in these doc comments says “negative error code otherwise”, but this module returns positive, script-style exit codes (e.g., 2 and 4). Please align the documentation with the actual exit-code semantics (or change the implementation to match the documented contract).
| /* Open the file and check with fstat to avoid TOCTOU */ | ||
| int fd = open(src_file, O_RDONLY | O_NOFOLLOW); | ||
| if (fd >= 0) { |
There was a problem hiding this comment.
open() and O_RDONLY/O_NOFOLLOW are used here but <fcntl.h> is not included, which can fail to compile on stricter toolchains. Add the missing include.
| #include "usb_log_utils.h" | ||
| #include "context_manager.h" | ||
| #include "archive_manager.h" | ||
| #include "uploadstblogs_types.h" |
There was a problem hiding this comment.
snprintf() is used in this module but <stdio.h> is not included, which can cause implicit declaration build failures under C99 with strict warnings. Add <stdio.h> to the includes.
| #include "uploadstblogs_types.h" | |
| #include "uploadstblogs_types.h" | |
| #include <stdio.h> |
| /* Check argument count - should be exactly 2 (program name + USB mount point) */ | ||
| if (argc != 2) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] USAGE: %s <USB mount point>\n", | ||
| __FUNCTION__, __LINE__, argv[0]); | ||
| return 4; /* Exit code 4 matches original script */ | ||
| } | ||
|
|
||
| /* Check if USB mount point argument is valid */ | ||
| if (!argv[1] || argv[1][0] == '\0') { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | ||
| "[%s:%d] USAGE: %s <USB mount point>\n", | ||
| __FUNCTION__, __LINE__, argv[0]); | ||
| return 4; | ||
| } | ||
|
|
There was a problem hiding this comment.
validate_input_parameters() logs argv[0] here but does not validate that argv is non-NULL or that argv[0] is present. Since this is a public function, add defensive checks (treat missing argv/argv[0] as invalid usage) to avoid potential crashes.
| /* Check argument count - should be exactly 2 (program name + USB mount point) */ | |
| if (argc != 2) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] USAGE: %s <USB mount point>\n", | |
| __FUNCTION__, __LINE__, argv[0]); | |
| return 4; /* Exit code 4 matches original script */ | |
| } | |
| /* Check if USB mount point argument is valid */ | |
| if (!argv[1] || argv[1][0] == '\0') { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] USAGE: %s <USB mount point>\n", | |
| __FUNCTION__, __LINE__, argv[0]); | |
| return 4; | |
| } | |
| const char *prog_name = "program"; | |
| /* Basic validation of argv and argv[0] to avoid NULL dereference */ | |
| if (argc < 1 || argv == NULL || argv[0] == NULL) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] USAGE: %s <USB mount point>\n", | |
| __FUNCTION__, __LINE__, prog_name); | |
| return 4; /* Treat missing argv/argv[0] as invalid usage */ | |
| } | |
| /* Now safe to use program name from argv[0] */ | |
| prog_name = argv[0]; | |
| /* Check argument count - should be exactly 2 (program name + USB mount point) */ | |
| if (argc != 2) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] USAGE: %s <USB mount point>\n", | |
| __FUNCTION__, __LINE__, prog_name); | |
| return 4; /* Exit code 4 matches original script */ | |
| } | |
| /* Check if USB mount point argument is valid */ | |
| if (!argv[1] || argv[1][0] == '\0') { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] USAGE: %s <USB mount point>\n", | |
| __FUNCTION__, __LINE__, prog_name); | |
| return 4; | |
| } |
|
|
||
| return 0; |
There was a problem hiding this comment.
move_log_files() reports success unconditionally (return 0) even if moves fail earlier (only warnings are logged). This can make the overall upload succeed while logs were not transferred. Consider returning a non-zero/expected exit code when any file move fails (and handle EXDEV by falling back to copy+unlink if needed).
| return 0; | |
| /* Treat partial move as failure: success only if all files moved or none to move */ | |
| if (file_count == 0 || moved_count == file_count) { | |
| return 0; | |
| } | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] One or more log files failed to move (moved %d of %d)\n", | |
| __FUNCTION__, __LINE__, moved_count, file_count); | |
| return -3; |
| /* Create USB log directory if it doesn't exist */ | ||
| ret = create_usb_log_directory(usb_log_dir); | ||
| if (ret != 0) { | ||
| return ret; |
There was a problem hiding this comment.
The module is documented to return specific positive exit codes (0/2/3/4), but several helpers return negative values (e.g., create_usb_log_directory/move_log_files/create_usb_log_archive). Returning those values directly here will produce non-script-compatible exit codes (and negatives get truncated by the shell). Consider mapping all internal errors to the defined USB_LOG_ERROR_* codes at this boundary.
| return ret; | |
| RDK_LOG(RDK_LOG_ERROR, LOG_USB_UPLOAD, | |
| "[%s:%d] Failed to create USB log directory '%s' (err=%d)\n", | |
| __FUNCTION__, __LINE__, usb_log_dir, ret); | |
| return USB_LOG_ERROR_GENERAL; |
2 participants
Reason for change: Prepare HLD Doc for usblogupload
Test Procedure: Verify build is passing
Risks: Low
Signed-off-by: Abhinav P V Abhinav_Valappil@comcast.com