RDKB-60656 : Available memory check for firmware downloads #13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a memory availability check before firmware downloads to prevent download attempts when insufficient memory is available. The change adds a conditional check using a new function can_proceed_fw_download() before initiating HTTP firmware downloads.
Changes:
- Added memory check guard before firmware download execution
- Integrated new library
lfw_download_chkfor memory validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/cm_http_dl.c | Added conditional check using can_proceed_fw_download() to gate firmware download execution |
| source/Makefile.am | Added linker flag for new library -lfw_download_chk to support memory check functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }else{ | ||
| http_status = HTTP_Download(); | ||
| } | ||
|
|
There was a problem hiding this comment.
In the "not enough memory" path, http_status is never assigned but is still read immediately afterward to decide ret_code (if (http_status == 200)). This can yield undefined behavior and may incorrectly return success even though the download was skipped. Set ret_code (and/or http_status) explicitly when memory is insufficient, or early-return from this branch.
| }else{ | |
| http_status = HTTP_Download(); | |
| } | |
| /* Treat insufficient memory as a failed download */ | |
| http_status = 0; | |
| }else{ | |
| http_status = HTTP_Download(); | |
| } |
| if(can_proceed_fw_download() == FW_DWNLD_MEMCHK_NOT_ENOUGH_MEM){ | ||
| printf("Available memory is not enough to proceed with firmware download\n"); |
There was a problem hiding this comment.
can_proceed_fw_download() and FW_DWNLD_MEMCHK_NOT_ENOUGH_MEM are introduced here but no corresponding header is included in this file. Please include the header that declares this API (rather than relying on implicit declarations or transitive includes), otherwise this may fail to compile under C99+ / -Werror builds.
bunnam988
force-pushed
the
feature/RDKB-60656
branch
from
5254500 to
1dd74d0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/cm_http_dl.c:652
- In the low-memory case,
http_statusis never set, but it’s still used in theif (http_status == 200)check below. This leads to undefined behavior and may incorrectly return success/failure. Sethttp_status(e.g., to a non-200 error code) and/or setret_codeand return early when memory is insufficient.
if(can_proceed_fw_download() == FW_DWNLD_MEMCHK_NOT_ENOUGH_MEM){
printf("Available memory is not enough to proceed with firmware download\n");
}else{
http_status = HTTP_Download();
}
// The return code is after RETRY_HTTP_DOWNLOAD_LIMIT has been reached
// For 200, return SUCCESS, else FAILURE and retry in the next window
if(http_status == 200)
ret_code = 0;
else
ret_code = 1;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <telemetry_busmessage_sender.h> | ||
| #include "fw_download_check.h" | ||
| #include <syscfg/syscfg.h> |
There was a problem hiding this comment.
#include "fw_download_check.h" introduces a new external header dependency, but this repository’s native-build dependency setup (e.g., cov_docker_script/component_config.json) doesn’t appear to install/provide it. If this header isn’t in the toolchain’s default include paths, CI/native builds will fail; consider adding the appropriate include path/dependency so the header is available in all build environments.
| AM_LDFLAGS = -lccsp_common | ||
| AM_LDFLAGS += -lcm_mgnt -lsysevent -lsyscfg | ||
| AM_LDFLAGS += -lcm_mgnt -lsysevent -lsyscfg -lfw_download_chk | ||
| XconfHttpDl_SOURCES = cm_http_dl.c |
There was a problem hiding this comment.
Linking against -lfw_download_chk adds a new library dependency, but configure.ac/native-build dependency config doesn’t appear to check for or build/install this library. This can cause link failures in CI or for developers without that library preinstalled; add a configure-time check (or ensure the native-build dependency scripts install it) so the build fails clearly when missing.
2 participants
RDKB-60656 : Available memory check for firmware downloads
Reason for change: Before firmware download, we need to check if the device have enough memory
Test Procedure: 1. while firmware download, available memory check logs should be seen.
2. If available memory < required memory, firmware download should not start.
Risks: medium
Priority: P1
Gerrit meta-layer changes:
https://gerrit.teamccp.com/#/q/topic:RDKB-60656+(status:open+OR+status:merged)
List all dependent GitHub PRs:
rdkcentral/provisioning-and-management#178
rdkcentral/utopia#182
rdkcentral/cable-modem-agent#23
rdkcentral/miscellaneous-broadband#37
https://github.com/rdk-gdcs/apparmor-profiles/pull/49