Feature/rdkemw 22385 - #166
Open
Abhinavpv28 wants to merge 38 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the uploadstblogs library/binary to support an “UploadLogsNow” workflow when invoked via the structured API, and makes the library headers installable/usable by external consumers (including C++ callers).
Changes:
- Add an
uploadlogsnow_modeflag toUploadSTBLogsParamsand routeuploadstblogs_run()through the UploadLogsNow workflow when enabled. - Install
uploadstblogspublic headers via Automake for consumption by other components. - Add C++ compatibility guards (
extern "C") to the public header.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| uploadstblogs/src/uploadstblogs.c | Adds API-side UploadLogsNow mode branch to execute the custom workflow early. |
| uploadstblogs/src/Makefile.am | Installs uploadstblogs headers for external consumers. |
| uploadstblogs/include/uploadstblogs.h | Adds extern "C" guards for C++ consumption. |
| uploadstblogs/include/uploadstblogs_types.h | Extends public API params with uploadlogsnow_mode. |
| "[%s:%d] UploadLogsNow mode detected via API, executing custom workflow\n", | ||
| __FUNCTION__, __LINE__); | ||
|
|
||
| ret = execute_uploadlogsnow_workflow(&ctx); |
Comment on lines
+44
to
+48
| # Install public headers for external consumers (e.g. tr69hostif) | ||
| uploadstblogsincludedir = $(includedir)/uploadstblogs | ||
| uploadstblogsinclude_HEADERS = \ | ||
| $(top_srcdir)/uploadstblogs/include/uploadstblogs.h \ | ||
| $(top_srcdir)/uploadstblogs/include/uploadstblogs_types.h |
Comment on lines
+124
to
+126
| #ifdef __cplusplus | ||
| } | ||
| #endif |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
uploadstblogs/src/uploadstblogs.c:292
- uploadstblogs_run() is documented to return 0 on success and 1 on failure, but execute_uploadlogsnow_workflow() returns 0 on success and a negative value on failure. Returning its raw return code breaks the API contract for external callers.
ret = execute_uploadlogsnow_workflow(&ctx);
uploadstblogs/include/uploadstblogs.h:125
- This header is now being installed for external consumers, but it still declares main(). Exposing main() from a public library header is misleading and can cause conflicts in consumers; it should be available only when building the standalone binary.
int main(int argc, char** argv);
#ifdef __cplusplus
}
uploadstblogs/src/context_manager.c:158
- rdk_LogOutput_File filelog is passed to rdk_logger_ext_init() with only fileName/fileLocation initialized; any remaining fields in the struct are left uninitialized and may be read by the logger implementation (undefined behavior). Zero-initialize the struct before populating fields.
rdk_LogOutput_File filelog;
Comment on lines
+58
to
+62
| rdk_logger_ext_config_t config = { | ||
| .pModuleName = "LOG.RDK.UPLOADSTB", /* Module name */ | ||
| .loglevel = RDK_LOG_INFO, /* Default log level */ | ||
| .output = RDKLOG_OUTPUT_CONSOLE, /* Output to console (stdout/stderr) */ | ||
| .output = RDKLOG_OUTPUT_FILE, |
Code Coverage Summary |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
dcm_utils.c:66
- The rdk_logger_ext_config_t designated initializer duplicates fields (.output and .pFilePolicy) and is missing a comma, which will either fail compilation or trigger -Werror warnings about overwritten initializers. It also sets the module name to LOG.RDK.UPLOADSTB inside DCMLOGInit(), which conflicts with the DCM logging component used elsewhere (LOG.RDK.DCM).
.output = RDKLOG_OUTPUT_CONSOLE, /* Output to console (stdout/stderr) */
.output = RDKLOG_OUTPUT_FILE,
.format = RDKLOG_FORMAT_WITH_TS, /* Timestamped format */
.pFilePolicy = NULL /* Not using file output, so NULL */
.pFilePolicy = &filelog /* using file output */
uploadstblogs/src/uploadstblogs.c:290
- uploadstblogs_run() initializes telemetry (t2_init) before validate_system(), but on validation failure the function returns without calling t2_uninit() or cleanup_iarm_connection(), while the new UploadLogsNow early-return path does. This leaves resources initialized on an error path and makes cleanup inconsistent.
/* Handle UploadLogsNow mode */
if (ctx.uploadlogsnow_mode) {
RDK_LOG(RDK_LOG_INFO, LOG_UPLOADSTB,
"[%s:%d] UploadLogsNow mode detected via API, executing custom workflow\n",
__FUNCTION__, __LINE__);
uploadstblogs/src/context_manager.c:163
- rdk_LogOutput_File is stack-allocated and only fileName/fileLocation are initialized. If the struct contains other policy fields (e.g., size/count), they will be left uninitialized. Zero-initialize the struct before populating fields to avoid undefined behavior.
rdk_LogOutput_File filelog;
strncpy(filelog.fileName, "dcmscript.log", sizeof(filelog.fileName)-1);
filelog.fileName[sizeof(filelog.fileName) - 1] = '\0';
strncpy(filelog.fileLocation, "/opt/logs/", sizeof(filelog.fileLocation)-1);
filelog.fileLocation[sizeof(filelog.fileLocation) - 1] = '\0';
| TriggerType trigger_type; /**< Trigger type (TRIGGER_SCHEDULED, TRIGGER_ONDEMAND, etc.) */ | ||
| bool rrd_flag; /**< RRD flag */ | ||
| const char* rrd_file; /**< RRD upload log file path (optional) */ | ||
| bool uploadlogsnow_mode; /**< When true, execute UploadLogsNow workflow */ |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
uploadstblogs/src/uploadstblogs.c:299
uploadstblogs_run()is documented to return 0 on success and 1 on failure, but in UploadLogsNow mode it returns the raw result ofexecute_uploadlogsnow_workflow(), which can be negative (e.g. -1). This breaks the API contract for external callers.
return ret;
dcm_utils.c:63
- DCMLOGInit() switched to extended logger init without guarding it behind an extended-logger feature macro and without falling back to
rdk_logger_init()when extended logging isn’t enabled. This risks build/link failures on platforms that only provide the standard logger path, and also logs an incorrect component name ("UPLOADSTB") on failure.
rdk_logger_ext_config_t config = {
.pModuleName = "LOG.RDK.DCM", /* Module name */
.loglevel = RDK_LOG_INFO, /* Default log level */
.output = RDKLOG_OUTPUT_FILE,
.format = RDKLOG_FORMAT_WITH_TS, /* Timestamped format */
uploadstblogs/src/context_manager.c:163
rdk_LogOutput_File filelogis not zero-initialized, so any fields beyondfileName/fileLocation(e.g.,fileSizeMax,fileCountMaxas used inbackup_logs/src/backup_logs.c) are passed to the logger with indeterminate values. This can cause unpredictable logger behavior.
rdk_LogOutput_File filelog;
strncpy(filelog.fileName, "dcmscript.log", sizeof(filelog.fileName)-1);
filelog.fileName[sizeof(filelog.fileName) - 1] = '\0';
strncpy(filelog.fileLocation, "/opt/logs/", sizeof(filelog.fileLocation)-1);
filelog.fileLocation[sizeof(filelog.fileLocation) - 1] = '\0';
| #include <signal.h> | ||
| #include <dirent.h> | ||
| #include <errno.h> | ||
| #include "rdk_logger.h" |
Code Coverage Summary |
1 similar comment
Code Coverage Summary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.