RDKB-60656 : Available memory check for firmware downloads#178
RDKB-60656 : Available memory check for firmware downloads#178
Conversation
c18420c to
8f85a4d
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a memory availability check before firmware downloads to prevent download failures due to insufficient memory. Two new TR-181 parameters are introduced to configure memory thresholds, and a pre-download validation function is called to verify adequate memory is available.
Changes:
- Added two new TR-181 parameters for configuring firmware download memory thresholds (AvlMem_RsrvThreshold and ImageProcMemPercent)
- Integrated memory check before firmware download by calling
can_proceed_fw_download() - Implemented getter and setter functions for the new configuration parameters using syscfg storage
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| config-arm/TR181-USGv2.XML | Defines two new unsignedInt parameters for firmware download memory configuration |
| source/TR-181/middle_layer_src/cosa_deviceinfo_dml.c | Implements getter and setter functions for the new memory threshold parameters |
| source-arm/TR-181/board_sbapi/cosa_deviceinfo_apis.c | Adds memory check before firmware download and includes fw_download_check.h header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | ||
| { | ||
| if (syscfg_set_u_commit (NULL, "FwDwld_ImageProcMemPercent", uValue) != 0) { | ||
| return FALSE; | ||
| } | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
There is no input validation for the uValue parameter. Consider adding validation to ensure the value is within a reasonable range. For ImageProcMemPercent, values should likely be in the range 0-100 if this represents a percentage. Without validation, invalid values could cause unexpected behavior in the firmware download memory check.
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); | ||
| return TRUE; | ||
| } | ||
|
|
||
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | ||
| { | ||
| char buf[10]= {0}; | ||
| syscfg_get( NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); |
There was a problem hiding this comment.
The syscfg_get return value should be checked for errors. Most other syscfg_get calls in this file check the return value and handle errors appropriately. If syscfg_get fails, the buffer may contain garbage data, and atoi will return an unpredictable value. Consider checking the return value and either returning FALSE or setting a default value when syscfg_get fails.
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | |
| *puLong = atoi(buf); | |
| return TRUE; | |
| } | |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | |
| { | |
| char buf[10]= {0}; | |
| syscfg_get( NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)); | |
| *puLong = atoi(buf); | |
| if (syscfg_get(NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)) != 0 || buf[0] == '\0') | |
| { | |
| /* On failure or empty value, default to 0 */ | |
| *puLong = 0; | |
| } | |
| else | |
| { | |
| *puLong = (ULONG)atoi(buf); | |
| } | |
| return TRUE; | |
| } | |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | |
| { | |
| char buf[10]= {0}; | |
| if (syscfg_get(NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)) != 0 || buf[0] == '\0') | |
| { | |
| /* On failure or empty value, default to 0 */ | |
| *puLong = 0; | |
| } | |
| else | |
| { | |
| *puLong = (ULONG)atoi(buf); | |
| } |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_AvlMem_RsrvThreshold") == 0) | ||
| { | ||
| if (syscfg_set_u_commit (NULL, "FwDwld_AvlMem_RsrvThreshold", uValue) != 0) { | ||
| return FALSE; | ||
| } | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
The indentation is inconsistent in this block. The opening brace should be indented consistently with the surrounding code, and the return statement should be indented properly. Compare with the similar code block at lines 1431-1437 which has the same indentation issue.
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | ||
| { | ||
| if (syscfg_set_u_commit (NULL, "FwDwld_ImageProcMemPercent", uValue) != 0) { | ||
| return FALSE; | ||
| } | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
The indentation is inconsistent in this block. The return statement should be aligned with the if statement rather than the code inside the if block. This inconsistency also exists in the previous block at lines 1423-1429.
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); | ||
| return TRUE; | ||
| } | ||
|
|
||
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | ||
| { | ||
| char buf[10]= {0}; | ||
| syscfg_get( NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); |
There was a problem hiding this comment.
The syscfg_get return value should be checked for errors. Most other syscfg_get calls in this file check the return value and handle errors appropriately. If syscfg_get fails, the buffer may contain garbage data, and atoi will return an unpredictable value. Consider checking the return value and either returning FALSE or setting a default value when syscfg_get fails.
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | |
| *puLong = atoi(buf); | |
| return TRUE; | |
| } | |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | |
| { | |
| char buf[10]= {0}; | |
| syscfg_get( NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)); | |
| *puLong = atoi(buf); | |
| if (syscfg_get(NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)) == 0) | |
| { | |
| *puLong = atoi(buf); | |
| } | |
| else | |
| { | |
| /* Default to 0 if syscfg_get fails */ | |
| *puLong = 0; | |
| } | |
| return TRUE; | |
| } | |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | |
| { | |
| char buf[10]= {0}; | |
| if (syscfg_get(NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)) == 0) | |
| { | |
| *puLong = atoi(buf); | |
| } | |
| else | |
| { | |
| /* Default to 0 if syscfg_get fails */ | |
| *puLong = 0; | |
| } |
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_AvlMem_RsrvThreshold") == 0) | ||
| { | ||
| char buf[10]= {0}; | ||
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); | ||
| return TRUE; | ||
| } | ||
|
|
||
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent") == 0) | ||
| { | ||
| char buf[10]= {0}; | ||
| syscfg_get( NULL, "FwDwld_ImageProcMemPercent", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); | ||
| return TRUE; |
There was a problem hiding this comment.
The new parameters X_RDKCENTRAL-COM_FwDwld_AvlMem_RsrvThreshold and X_RDKCENTRAL-COM_FwDwld_ImageProcMemPercent lack test coverage. The repository has existing tests for similar DeviceInfo parameter getters and setters (see source/test/cosa_deviceinfo_dml_custom_test.cpp). Consider adding tests to verify the get and set functionality for these new parameters, including edge cases like syscfg_get failures.
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_AvlMem_RsrvThreshold") == 0) | ||
| { | ||
| if (syscfg_set_u_commit (NULL, "FwDwld_AvlMem_RsrvThreshold", uValue) != 0) { | ||
| return FALSE; | ||
| } | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
There is no input validation for the uValue parameter. Consider adding validation to ensure the value is within a reasonable range. For example, if these values represent memory thresholds in MB or percentages, you should validate that they don't exceed system limits or are within expected ranges (e.g., 0-100 for percentages). Without validation, invalid values could cause unexpected behavior in the firmware download memory check.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| libCcspPandM_board_sbapi_la_LDFLAGS = -lccsp_common -ltime_conversion -lwebconfig_framework -lmsgpackc -lfw_download_chk | ||
| if FEATURE_HOTSPOT_SUPPORT | ||
| libCcspPandM_board_sbapi_la_LDFLAGS += -lHotspotApi | ||
| endif |
There was a problem hiding this comment.
libCcspPandM_board_sbapi_la_LDFLAGS is being assigned with = here, which overwrites any earlier conditional += additions (e.g., -lrdkconfig from IS_LIBRDKCONFIG_ENABLED and -lnet from CORE_NET_LIB_FEATURE_SUPPORT just above). This will silently drop those link dependencies in configurations where they’re enabled. Consolidate into a single base assignment and only use += afterwards, or change this to += -lfw_download_chk instead of reassigning the whole variable.
| if (strcmp(ParamName, "X_RDKCENTRAL-COM_FwDwld_AvlMem_RsrvThreshold") == 0) | ||
| { | ||
| char buf[10]= {0}; | ||
| syscfg_get( NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)); | ||
| *puLong = atoi(buf); | ||
| return TRUE; |
There was a problem hiding this comment.
The syscfg value is read into char buf[10], which is too small for a uint32 string representation (needs up to 10 digits + NUL). This can truncate the stored value and atoi() will then return the wrong number. Increase the buffer (e.g., 12+) and prefer strtoul with proper error checking instead of atoi (which returns int and can overflow).
| #include "cosa_drg_common.h" | ||
| #include "safec_lib_common.h" | ||
| #include "ansc_string_util.h" | ||
| #include "fw_download_check.h" |
There was a problem hiding this comment.
fw_download_check.h is not present anywhere in this repo, so this include will fail unless the build environment provides it via an external -I path/package. Please add the header to the repo (or ensure it’s exported by a declared dependency and included via the correct include path), and update the build system accordingly so local builds don’t break.
| #include "fw_download_check.h" |
eda0c29 to
f06296b
Compare
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
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/xconf-client#13 rdkcentral/provisioning-and-management#178 rdkcentral/cable-modem-agent#23 rdkcentral/miscellaneous-broadband#37 https://github.com/rdk-gdcs/apparmor-profiles/pull/49
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/xconf-client#13
rdkcentral/utopia#182
rdkcentral/cable-modem-agent#23
rdkcentral/miscellaneous-broadband#37
https://github.com/rdk-gdcs/apparmor-profiles/pull/49