-
Notifications
You must be signed in to change notification settings - Fork 2
rebase #36
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: feature/logging
Are you sure you want to change the base?
rebase #36
Conversation
| name: Execute L1 test suite in test container environment | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Log in to GitHub Container Registry | ||
| uses: docker/login-action@v2 | ||
| with: | ||
| registry: ghcr.io | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Pull test container image | ||
| run: docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest | ||
|
|
||
| - name: Start test container | ||
| run: | | ||
| docker run -d --name native-platform -v ${{ github.workspace }}:/mnt/L1_CONTAINER_SHARED_VOLUME ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest | ||
|
|
||
| - name: Run unit tests | ||
| run: sh unit_test.sh | ||
| - name: Run L1 Unit Tests inside container | ||
| run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh unit_test.sh" | ||
|
|
||
| - name: Upload test results to automatic test result management system | ||
| - name: Copy L1 test results to runner | ||
| run: | | ||
| docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report | ||
| ls -l /tmp/Gtest_Report | ||
|
|
||
| upload-test-results: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, explicitly add a permissions block to the workflow. This can be added at the root level (applies to all jobs), or individually to jobs. The simplest, most maintainable approach is to add it at the root, immediately after the name field and before the on: field, to set minimal permissions (usually contents: read) for the workflow. If a job requires additional permissions, you can specify them at the job level, overriding the root. In your workflow, none of the visible steps appear to require anything beyond reading contents (e.g., there are no pushes or PR creations). Thus, adding permissions: contents: read immediately after the name line is the best fix.
-
Copy modified lines R2-R3
| @@ -1,4 +1,6 @@ | ||
| name: L1 Unit Tests | ||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| pull_request: |
#68) * RDKEMW-12237: Uploadlib use same file to get S3 url output for all the component Reason for change: Pass output file name as a parameter to API Test Procedure: Make sure S3 url output store inside the file passed to the API Risks: Medium Priority: P1 --------- Signed-off-by: Sahu <satyasundar_sahu@comcast.com> Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Co-authored-by: Gomathi Shankar <gomathishankar37@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 PR performs a major refactoring of the RDK common_utilities codebase to transform rdkfwupgrader into a library-driven architecture. The changes include extracting upload functionality into a separate module, introducing new logging macros for common utilities, improving error handling, and adding comprehensive unit tests.
Changes:
- Extracted upload functionality from rdkfwupgrader into new
uploadutilslibrary with support for mTLS, CodeBig, and S3 uploads - Replaced
SWLOG_*logging macros withCOMMONUTILITIES_*macros throughout the codebase to distinguish common utility logs from firmware upgrade logs - Added robust error handling for system calls (chdir, lstat, stat, fclose) and CURL operations
- Added comprehensive unit tests for the new upload utilities
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/system_utils.c | Added error checking for chdir/lstat calls, fixed resource leaks, added construct_full_path helper |
| utils/rdkv_cdl_log_wrapper.h | Added COMMONUTILITIES_* logging macros, removed trailing newlines from SWLOG_* macros |
| utils/rdk_fwdl_utils.* | Increased MAX_DEVICE_PROP_BUFF_SIZE, added null checks, changed logging to COMMONUTILITIES_* |
| utils/common_device_api.* | Added new utility functions (GetUTCTime, GetCapabilities, GetTimezone, etc.), fixed GetPartnerId |
| uploadutils/* | New upload utility module with mTLS, CodeBig, and S3 support |
| unit-test/* | Added comprehensive unit tests for upload utilities |
| dwnlutils/urlHelper.* | Added urlEncodeString function, improved error handling, added retry logic for CURL 56/18/28 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (chdir(dir) != 0) { | ||
| COMMONUTILITIES_ERROR("Failed to change directory to %s \n", dir); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
Copilot
AI
Jan 16, 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.
Extra space before newline in error message. Should be "Failed to change directory to %s\n" instead of "Failed to change directory to %s \n".
| COMMONUTILITIES_ERROR("lstat failed for %s: %s\n", entry->d_name, strerror(errno)); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
Copilot
AI
Jan 16, 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.
Trailing whitespace after the closing brace.
| strncpy(out, constructed_path, strlen(constructed_path) + 1); // Copy path to out | ||
| COMMONUTILITIES_DEBUG(" findPFile : Constructed path out : %s", out); | ||
| free(constructed_path); | ||
| constructed_path =NULL; |
Copilot
AI
Jan 16, 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.
Missing space before NULL. Should be constructed_path = NULL;.
| struct dirent *entry; | ||
| char filePath[RDK_APP_PATH_LEN]; | ||
|
|
||
| char filePath[RDK_APP_PATH_LEN+1] = {0}; |
Copilot
AI
Jan 16, 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.
Trailing whitespace at the end of the line.
|
|
||
| if (emptyFolder(filePath) != RDK_API_SUCCESS) { |
Copilot
AI
Jan 16, 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.
Line 704 contains only whitespace and should be removed.
| FILE *fp = userdata; | ||
| if(fp != NULL && buffer != NULL) { | ||
| SWLOG_INFO("header_callback():=%s\n",buffer); | ||
| COMMONUTILITIES_INFO("header_callback():=%s",buffer); // No need to end with new line \n as buffer already has \r\n |
Copilot
AI
Jan 16, 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.
Comment uses informal contraction. Should be "newline" instead of "new line".
| COMMONUTILITIES_INFO("header_callback():=%s",buffer); // No need to end with new line \n as buffer already has \r\n | |
| COMMONUTILITIES_INFO("header_callback():=%s",buffer); // No need to end with newline \n as buffer already has \r\n |
| if( pTmp) | ||
| { | ||
| ++pTmp; | ||
| } | ||
| else | ||
| { | ||
| continue; | ||
| } |
Copilot
AI
Jan 16, 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.
Inconsistent indentation mixing tabs and spaces. The code should use consistent indentation throughout.
| } | ||
| if (buff_size == 0 || buff_size > MAX_DEVICE_PROP_BUFF_SIZE) { | ||
| SWLOG_ERROR("%s : buff size not in the range. size should be < %d\n", __FUNCTION__, MAX_DEVICE_PROP_BUFF_SIZE); | ||
| if (buff_size == 0 || buff_size >= MAX_DEVICE_PROP_BUFF_SIZE) { |
Copilot
AI
Jan 16, 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.
Changed comparison from > to >= which may break existing code that passes MAX_DEVICE_PROP_BUFF_SIZE as the buffer size. This is a subtle API behavior change that should be documented or reconsidered.
| ; | ||
| } | ||
| snprintf( pPartnerId, szBufSize, "%s", buf ); | ||
| snprintf( pPartnerId, szBufSize, "%s", pTmp ); |
Copilot
AI
Jan 16, 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.
Changed from copying buf to copying pTmp. This appears intentional to skip leading whitespace, but verify this is the correct behavior change as it modifies what gets copied to the output buffer.
Common Utils 1.5.0 release
RDKEMW-12849: Update to print logs to console Reason for change: RDKLogger sends the output directly to syslog to gain the performance. But the platform components like `rdkfwupdater`, `rfc`, `rdm`, `logupload` & `crashupload` are spun by scripts and console output are redirected to file manually. So, for these components, we have to configure logger to print to console instead syslog. Once these components become standalone and launched by systemd service independently, this change can be removed to gain performance. Test Procedure: Ensure logs from above mentioned components are printed. Risks: None. In the past also output was redirected; so whatever gained in last 2 weeks is not present anymore. Signed-off-by: Karunakaran A <karunakaran_amirthalingam@cable.comcast.com>
…ts To C Implementation (#73)
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
Copilot reviewed 45 out of 45 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret_code = curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); | ||
| if (ret_code != CURLE_OK) { | ||
| COMMONUTILITIES_ERROR("%s: CURLOPT_VERBOSE failed: %s\n", | ||
| __FUNCTION__, curl_easy_strerror(ret_code)); | ||
| fclose(resp_fp); | ||
| if (headers) curl_slist_free_all(headers); | ||
| return (int)ret_code; | ||
| } |
Copilot
AI
Feb 3, 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.
performHttpMetadataPost(): CURLOPT_VERBOSE is enabled unconditionally. This is very noisy in production and can leak sensitive request/response details into logs. Gate this behind a build-time/debug flag (e.g., CURL_DEBUG) similar to performS3PutUpload().
| char data[32]; | ||
| EXPECT_NE(GetPartnerId(data, 7),0); | ||
| printf("Pertner ID = %s\n", data); | ||
| EXPECT_EQ(GetPartnerId(data, 7), 0); |
Copilot
AI
Feb 3, 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.
This test now expects GetPartnerId(...) to return 0, but GetPartnerId returns the number of characters copied (stripinvalidchar result). In the no_source_file case it typically falls back to "comcast" and should return a non-zero length. Update the assertion to match the API contract (and optionally validate the resulting string).
| EXPECT_EQ(GetPartnerId(data, 7), 0); | |
| EXPECT_NE(GetPartnerId(data, 7), 0); |
| if (chdir(dir) != 0) { | ||
| COMMONUTILITIES_ERROR("Failed to change directory to %s \n", dir); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
Copilot
AI
Feb 3, 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.
findFile() changes the process working directory with chdir(dir) but only attempts to restore it with chdir(".."). If dir is an absolute path (or a relative path not under the original cwd), this will not restore the original working directory and can break subsequent file operations (and is not thread-safe). Save/restore the original cwd (e.g., open(".") + fchdir) or avoid chdir entirely by building full paths / using openat-based traversal.
| if (lstat(entry->d_name, &statbuf) == -1) { | ||
| COMMONUTILITIES_ERROR("lstat failed for %s: %s\n", entry->d_name, strerror(errno)); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
Copilot
AI
Feb 3, 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.
On lstat() failure, findFile() closes the directory and returns 0 immediately. This makes a single unreadable entry (permissions, transient ENOENT, broken symlink, etc.) abort the entire search and can yield false negatives. Consider logging and continuing to the next entry instead of returning early.
| int doCodeBigSigning( int server_type, const char *SignInput, char *signurl, size_t signurlsize, char *outhheader, size_t outHeaderSize ) | ||
| { | ||
| return 0; | ||
| } |
Copilot
AI
Feb 3, 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.
doCodeBigSigning() (and related helpers) are currently stubbed to return success (0) without populating outputs. Since this file is compiled into libuploadutil when USE_CPC_CODE is disabled, it will override any real implementation and cause CodeBig signing/URL discovery to silently “succeed” with empty/invalid data. Either provide the real implementation here, or remove these stubs from the build and link against the intended implementation/library.
| char* constructed_path = construct_full_path(path, entry->d_name); | ||
| if (constructed_path) { | ||
| strncpy(out, constructed_path, strlen(constructed_path) + 1); // Copy path to out | ||
| COMMONUTILITIES_DEBUG(" findPFile : Constructed path out : %s", out); |
Copilot
AI
Feb 3, 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.
findPFile(): copying the constructed path into out uses strncpy(out, constructed_path, strlen(constructed_path)+1) without knowing the capacity of out. If the constructed path is longer than the caller-allocated buffer, this will overflow out. Consider changing the API to accept out buffer size and use snprintf, or enforce a documented max length and cap the copy with explicit NUL termination.
| if (!strtokvalue || !string || !outValue) { | ||
| COMMONUTILITIES_ERROR("Invalid Parameters %p %p %p", strtokvalue, string, outValue); | ||
| return; | ||
| } | ||
| FILE *file = fopen(path,"r"); | ||
|
|
||
| if( (strtokvalue != NULL) && (string != NULL) && ( outValue != NULL) ){ | ||
| if( file ){ | ||
| while(fgets(lines, sizeof(lines), file)){ | ||
| token = strtok(lines, strtokvalue); | ||
|
|
||
| while(token != NULL){ | ||
| if(strcmp(token,string) == 0 ){ | ||
| strncpy(outValue, strtok(NULL,strtokvalue),128); | ||
| break; | ||
| } | ||
| token = strtok(NULL,strtokvalue); | ||
| } | ||
| if( file ){ |
Copilot
AI
Feb 3, 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.
getStringValueFromFile(): parameter validation checks strtokvalue/string/outValue, but path is still used unconditionally in fopen(path, ...) and in the error log. If path is NULL this will crash. Please include path in the validation check.
| while(token != NULL){ | ||
| if(strcmp(token,string) == 0 ){ | ||
| strncpy(outValue, strtok(NULL,strtokvalue),128); | ||
| break; |
Copilot
AI
Feb 3, 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.
getStringValueFromFile(): strtok(NULL, strtokvalue) can return NULL (e.g., key present but no value). Passing that NULL to strncpy will crash. Guard the return value of strtok before copying, and ensure outValue is always NUL-terminated (strncpy does not guarantee this when truncating).
| while( ((*pPtr=(char)fgetc( fp )) != EOF) && !feof( fp ) ) | ||
| { | ||
| ++pPtr; | ||
| } | ||
| *pPtr = 0; |
Copilot
AI
Feb 3, 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.
GetFileContents(): this loop casts fgetc() to char and compares the stored char to EOF. EOF is an int and can’t be represented safely in a char; this can mis-handle bytes like 0xFF and can fail to terminate correctly. Use an int to hold fgetc()’s return value and stop on EOF before storing into the buffer.
| /* Step 1: Get CodeBig signed URL and authorization header */ | ||
| if (doCodeBigSigningForUpload(server_type, filepath, codebig_url, | ||
| sizeof(codebig_url), auth_header, sizeof(auth_header)) != 0) { | ||
| COMMONUTILITIES_ERROR("%s: CodeBig signing failed\n", __FUNCTION__); | ||
| return -1; | ||
| } |
Copilot
AI
Feb 3, 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.
performCodeBigMetadataPost(): CodeBig signing returns both a signed URL (codebig_url) and an authorization header (auth_header), but auth_header is never applied to the request. If the backend expects this header, the metadata POST will fail or be unauthenticated. Consider extending performHttpMetadataPost() to accept additional headers or setting CURLOPT_HTTPHEADER here with auth_header.
Common Utils 1.5.1 release
RDKEMW-12849: Update to print logs to console For coverity workflow fail we will create cmf ticket for check.
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
Copilot reviewed 46 out of 46 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.
rdklogger issue fix
rdklogger issue fix
No description provided.