-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/simplifylib coverity file change #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors the uploadstblogs codebase by simplifying the RuntimeContext structure and consolidating related functionality. The main changes flatten nested structures within RuntimeContext (removing paths, endpoints, flags, settings, device, and retry sub-structures) and merge related source files for better maintainability.
Key changes:
- Flattened RuntimeContext structure by promoting nested fields to top-level members
- Consolidated strategy implementations into a single strategies.c file (merging strategy_dcm.c, strategy_ondemand.c, strategy_reboot.c)
- Merged log collection functionality into archive_manager.c (from log_collector.c)
- Combined cleanup functionality into cleanup_handler.c (from cleanup_manager.c)
- Added new uploadstblogs API functions (uploadstblogs_run, uploadstblogs_execute) with conditional binary compilation
- Updated all test files to use the new flattened field access patterns
- Modified build system (Makefile.am) to reflect file consolidations
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| uploadstblogs/unittest/validation_gtest.cpp | Updated field access from ctx.paths.* to ctx.* |
| uploadstblogs/unittest/upload_engine_gtest.cpp | Updated field access patterns |
| uploadstblogs/unittest/strategy_selector_gtest.cpp | Updated field access from nested to flat structure |
| uploadstblogs/unittest/strategy_ondemand_gtest.cpp | File deleted, tests consolidated |
| uploadstblogs/unittest/strategy_handler_gtest.cpp | Updated field access patterns |
| uploadstblogs/unittest/strategy_dcm_gtest.cpp | File deleted, tests consolidated |
| uploadstblogs/unittest/strategies_gtest.cpp | New file with consolidated strategy tests |
| uploadstblogs/unittest/retry_logic_gtest.cpp | Updated ctx.retry.* to ctx.* |
| uploadstblogs/unittest/path_handler_gtest.cpp | Updated nested field access to flat, added HTTP result file handling |
| uploadstblogs/unittest/mocks/mock_file_operations.h | Added copy_file mock declaration |
| uploadstblogs/unittest/mocks/mock_file_operations.cpp | Added copy_file mock implementation |
| uploadstblogs/unittest/log_collector_gtest.cpp | Updated to use archive_manager.h with flattened structure |
| uploadstblogs/unittest/event_manager_gtest.cpp | Updated ctx.device.* to ctx.* |
| uploadstblogs/unittest/context_manager_gtest.cpp | Updated all nested field accesses |
| uploadstblogs/unittest/cleanup_handler_gtest.cpp | Updated includes and mock initialization |
| uploadstblogs/unittest/archive_manager_gtest.cpp | Added new log collection tests, updated field access |
| uploadstblogs/unittest/Makefile.am | Updated test executable names and removed obsolete tests |
| uploadstblogs/src/validation.c | Updated field access, added conditional PREV_LOG_PATH validation |
| uploadstblogs/src/uploadstblogs.c | Added new API functions, updated field access throughout |
| uploadstblogs/src/strategy_selector.c | Updated all field access patterns |
| uploadstblogs/src/strategy_reboot.c | File deleted, merged into strategies.c |
| uploadstblogs/src/strategy_ondemand.c | File deleted, merged into strategies.c |
| uploadstblogs/src/strategy_handler.c | Updated field access patterns |
| uploadstblogs/src/strategy_dcm.c | File deleted, merged into strategies.c |
| uploadstblogs/src/strategies.c | New consolidated file containing all three strategy implementations |
| uploadstblogs/src/retry_logic.c | Updated ctx.retry.* to ctx.* |
| uploadstblogs/src/path_handler.c | Updated field access, added HTTP result file routing for RRD vs standard uploads |
| uploadstblogs/src/log_collector.c | File deleted, merged into archive_manager.c |
| uploadstblogs/src/event_manager.c | Updated field access, added conditional maintenance events |
| uploadstblogs/src/context_manager.c | Updated all field initializations to flat structure |
| uploadstblogs/src/cleanup_manager.c | File deleted, merged into cleanup_handler.c |
| uploadstblogs/src/cleanup_handler.c | Merged cleanup_manager functionality, updated temp file cleanup |
| uploadstblogs/src/archive_manager.c | Merged log_collector functionality, updated field access |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ctx->rrd_flag == 0) { | ||
| // Check PREV_LOG_PATH - critical for upload (matches script behavior) | ||
| if (strlen(ctx->paths.prev_log_path) > 0) { | ||
| if (!dir_exists(ctx->paths.prev_log_path)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, "[%s:%d] The Previous Logs folder is missing: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->paths.prev_log_path); | ||
| if (strlen(ctx->prev_log_path) > 0) { | ||
| if (!dir_exists(ctx->prev_log_path)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, "[%s:%d] The Previous Logs folder is missing: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->prev_log_path); | ||
| // Script sends MAINT_LOGUPLOAD_ERROR=5 when PREV_LOG_PATH is missing | ||
| emit_folder_missing_error(); | ||
| all_valid = false; | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, "[%s:%d] PREV_LOG_PATH exists: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->paths.prev_log_path); | ||
| emit_folder_missing_error(); | ||
| all_valid = false; | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, "[%s:%d] PREV_LOG_PATH exists: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->prev_log_path); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'ctx->rrd_flag == 0' is added to check PREV_LOG_PATH validation. However, this logic change may affect RRD upload scenarios where PREV_LOG_PATH might still be needed. The original script behavior should be verified to ensure this conditional skipping of PREV_LOG_PATH validation for RRD uploads is correct.
|
|
||
| // Send maintenance event only if device is not broadband and maintenance enabled | ||
| if (!is_device_broadband(ctx) && is_maintenance_enabled()) { | ||
| if (!is_device_broadband(ctx) && is_maintenance_enabled() && ctx->rrd_flag == 0) { |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'ctx->rrd_flag == 0' is added to emit_upload_success and emit_no_logs_reboot functions to conditionally send maintenance events. This changes the original behavior where maintenance events were sent regardless of rrd_flag. Verify that this is the intended behavior change and matches the script logic, as this could affect maintenance event reporting for RRD scenarios.
| if (!is_device_broadband(ctx) && is_maintenance_enabled() && ctx->rrd_flag == 0) { | |
| if (!is_device_broadband(ctx) && is_maintenance_enabled()) { |
| int performMetadataPostWithCertRotationEx(const char *upload_url, const char *outfile, | ||
| const char *extra_fields, MtlsAuth_t *sec_out, | ||
| long *http_code_out); |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name 'outfile' in the function signature should match the documentation and parameter description which refers to it as the output file for HTTP results. The parameter was previously named 'filepath' which was misleading. Consider updating the comment to clarify that this is now the output file path for storing HTTP results rather than the archive filepath.
| @@ -0,0 +1,1109 @@ | |||
| /* | |||
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file starts with a UTF-8 BOM (Byte Order Mark) character sequence (). This can cause compilation issues with some C compilers and should be removed. The BOM is not needed for C source files and may lead to unexpected behavior.
| /* | |
| /* |
No description provided.