RDKBACCL-1485: Customised WAN testing - DOnt Merge - #504
Conversation
Reason for change:Configurable WAN feature Changes required for customisation to work if done Test Procedure: Build and flash the image with customised wan Risks: None
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Banana Pi R4 Yocto layer to support customized/configurable WAN interface handling by updating multiple default configs from erouter0 to erouter1, and by adding HAL patches that read the WAN virtual interface name from /nvram/wan_name.txt.
Changes:
- Update various default configs (device properties, TR-069 config, PSM defaults, utopia defaults) to use
erouter1instead oferouter0. - Add HAL patching to derive WAN interface name dynamically from
/nvram/wan_name.txt(platform + ethsw), and wire those patches into the relevant bbappends. - Modify usp-pa systemd unit at install time to pass an interface derived from
sysevent.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| meta-rdk-mtk-bpir4/recipes-rdkb/usp-pa/usp-pa.bbappend | Rewrites usp-pa systemd ExecStart to pass a WAN interface from sysevent. |
| meta-rdk-mtk-bpir4/recipes-rdkb/sysint-broadband/sysint-broadband.bbappend | Installs additional device scripts into /usr/ccsp/tad. |
| meta-rdk-mtk-bpir4/recipes-rdkb/sysint-broadband/files/bpi_custom_device.properties | Updates device property defaults to erouter1. |
| meta-rdk-mtk-bpir4/recipes-ccsp/util/utopia.bbappend | Updates utopia system defaults WAN interface placeholders to erouter1. |
| meta-rdk-mtk-bpir4/recipes-ccsp/hal/hal-platform-generic_git.bbappend | Adds the new platform HAL patch to SRC_URI. |
| meta-rdk-mtk-bpir4/recipes-ccsp/hal/hal-ethsw-generic_git.bbappend | Adds the new ethsw HAL patch to SRC_URI. |
| meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-platform.patch | Introduces WAN interface name lookup from /nvram/wan_name.txt for base MAC path selection. |
| meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch | Introduces WAN interface name lookup from /nvram/wan_name.txt for ethsw operations. |
| meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/ccsp-tr069-pa.bbappend | Updates TR-069 outbound interface to erouter1. |
| meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/ccsp-psm/bbhm_def_cfg_banana.xml | Updates PSM defaults for EthLink/WanManager virtual interface name to erouter1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TARGET_CFLAGS += " -DINCLUDE_LCM_DATAMODEL " | ||
| do_install:append:class-target () { | ||
|
|
||
| sed -i "/^ExecStart=/c\\ExecStart=/bin/sh -c '/usr/bin/obuspa --plugin /usr/libexec/usp-pa-vendor-rdk.so -v1 --resetfile /etc/usp-pa/usp_factory_reset.conf --truststore /etc/usp-pa/usp_truststore.pem --interface \"\$(sysevent get current_wan_ifname)\" --log syslog --dbfile /nvram/usp-pa.db'" ${D}${systemd_unitdir}/system/usp-pa.service |
| --- platform/rdkb_hal/src/platform/platform_hal.c_orig | ||
| +++ platform/rdkb_hal/src/platform/platform_hal.c | ||
| @@ -167,9 +167,24 @@ |
| { | ||
| char cmd[32] = {0}; | ||
|
|
||
| - sprintf(cmd,"ifconfig %s %s",ETH_WAN_INTERFACE, enable ? "up":"down"); | ||
| + sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down"); | ||
| system(cmd); | ||
|
|
| <Record name="dmsb.ethlink.ifcount" type="astr">1</Record> | ||
| <Record name="dmsb.ethlink.1.Enable" type="astr">FALSE</Record> | ||
| <Record name="dmsb.ethlink.1.alias" type="astr">WANOE</Record> | ||
| <Record name="dmsb.ethlink.1.name" type="astr">erouter0</Record> | ||
| <Record name="dmsb.ethlink.1.name" type="astr">erouter1</Record> | ||
| <Record name="dmsb.ethlink.1.lowerlayers" type="astr"></Record> |
| int link = 0; | ||
| char path[32] = {0}; | ||
|
|
||
| - sprintf(path, "/sys/class/net/erouter0/carrier"); | ||
| + sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName); | ||
| link = is_interface_link(path); |
| + sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down"); | ||
| system(cmd); | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-platform.patch:1
- The patch header references
platform_hal.c_orig, which is inconsistent with the existing HAL patch path style (e.g.,platform.orig/...) and is likely to makedo_patchfail to locate the file (default-p1strip). Update the---path so the old file resolves tordkb_hal/src/platform/platform_hal.c.
--- platform/rdkb_hal/src/platform/platform_hal.c_orig
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:99
cmd[32]combined withsprintf("ifconfig %s ...", g_virtualIfName, ...)can overflow now that the interface name is configurable (read from/nvram/wan_name.txt). Usesnprintf(and a larger buffer) to avoid overflow.
char cmd[32] = {0};
- sprintf(cmd,"ifconfig %s %s",ETH_WAN_INTERFACE, enable ? "up":"down");
+ sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down");
system(cmd);
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:108
path[32]is too small for/sys/class/net/<ifname>/carrierwith a configurable interface name, andsprintfcan overflow it. Use a larger buffer andsnprintf.
char path[32] = {0};
- sprintf(path, "/sys/class/net/erouter0/carrier");
+ sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName);
link = is_interface_link(path);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:50
- Same NUL-termination issue here:
strncpymay leavewanPhyNameunterminated. Usesnprintf(or explicitly terminate).
+ strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:65
strncpy(wanPhyName, fileValue, sizeof(wanPhyName) - 1)may leavewanPhyNameunterminated, andfileValuecomes from a file so length is not guaranteed. Usesnprintf(or explicitly terminate).
+ strncpy(wanPhyName,fileValue, sizeof(wanPhyName) - 1);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:68
- Same NUL-termination issue:
strncpymay leavewanPhyNameunterminated ifout_valueis long.
+ strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-RDKBACCL-1938-DCMSetting.conf-is-empty-due-to-Config.patch:13
- Typo in patch description: "configurred" → "configured".
Reason for change : In Ethernet based code flow , the Device.IP.Interface DM's are not updating with the configurred wan name when Configurable wan is enabled
| + else | ||
| + { | ||
| + fprintf(fp, "%s", out_value); | ||
| + strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1); |
| + snprintf(wan_mac, sizeof(wan_mac), "%02x:%02x:%02x:%02x:%02x:%02x", macAddr.hw[0], macAddr.hw[1], macAddr.hw[2], | ||
| + macAddr.hw[3], macAddr.hw[4], macAddr.hw[5]); | ||
| + | ||
| + v_secure_system("syscfg get wan_physical_ifname > /tmp/wan_name.txt"); |
| Subject: [PATCH] RDKBACCL-1938 : DCMSetting.conf is empty due to Configurable | ||
| WAN Interface | ||
|
|
||
| Reason for change: After enbaling Configurable WAN , DCM feature is not working as expected |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-platform.patch:3
- Patch header references
platform_hal.c_orig, which is not the naming used by other patches in this layer (they useplatform.orig/.../platform_hal.c). This is likely to make the patch fail to apply during the Yocto build.
--- platform/rdkb_hal/src/platform/platform_hal.c_orig
+++ platform/rdkb_hal/src/platform/platform_hal.c
@@ -167,9 +167,24 @@
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:99
- This hunk keeps
cmdat 32 bytes and usessprintf(...)with a variable-length interface name, which can overflowcmd(memory-safety issue). Increase the buffer and usesnprintf.
{
char cmd[32] = {0};
- sprintf(cmd,"ifconfig %s %s",ETH_WAN_INTERFACE, enable ? "up":"down");
+ sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down");
system(cmd);
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:108
pathis only 32 bytes but is populated viasprintfwith a potentially longer interface name (/sys/class/net/<ifname>/carrier), which can overflow the buffer. Use a larger buffer andsnprintf.
int link = 0;
char path[32] = {0};
- sprintf(path, "/sys/class/net/erouter0/carrier");
+ sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName);
link = is_interface_link(path);
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:42
g_virtualIfNameis sourced from/nvram/wan_name.txtand later used in interface operations (including building shell commands elsewhere in this patch). It should be validated to contain only a safe interface-name charset; otherwise keep the default.
+ buffer[strcspn(buffer, "\n")] = '\0';
+ if(strlen(buffer) > 0)
+ {
+ snprintf(g_virtualIfName,sizeof(g_virtualIfName),"%s",buffer);
+ }
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/ccsp-psm/bbhm_def_cfg_banana.xml:1337
- The EthLink and WanManager VirtualInterface names are updated to
erouter1, butdmsb.vlanmanager.1.nameremainserouter0(line 1345). This inconsistency can cause VLAN manager to reference a different/non-existent interface. Consider updating it to match the new WAN interface name.
<Record name="dmsb.ethlink.1.name" type="astr">erouter1</Record>
| + if (fgets(g_virtualIfName, sizeof(g_virtualIfName), fp)) | ||
| + { | ||
| + g_virtualIfName[strcspn(g_virtualIfName, "\n")] = '\0'; | ||
| + if (g_virtualIfName[0] == '\0') | ||
| + strcpy(g_virtualIfName, "erouter0"); |
| TARGET_CFLAGS += " -DINCLUDE_LCM_DATAMODEL " | ||
| do_install:append:class-target () { | ||
|
|
||
| sed -i "/^ExecStart=/c\\ExecStart=/bin/sh -c '/usr/bin/obuspa --plugin /usr/libexec/usp-pa-vendor-rdk.so -v1 --resetfile /etc/usp-pa/usp_factory_reset.conf --truststore /etc/usp-pa/usp_truststore.pem --interface \"\$(sysevent get current_wan_ifname)\" --log syslog --dbfile /nvram/usp-pa.db'" ${D}${systemd_unitdir}/system/usp-pa.service |
Reason for change: Monitor the status lan0 Test procedure: WAN interface to get IP Risks: Low Signed-off-by: Manigandan Gopalakrishnan <Manigandan_Gopalakrishnan@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-platform.patch:2
- This patch header uses
platform_hal.c_orig, which is inconsistent with the other HAL patch headers in this layer (they useplatform.orig/.../platform_hal.c). Keeping consistent paths reduces the risk of patch application failures during the Yocto patch step.
--- platform/rdkb_hal/src/platform/platform_hal.c_orig
+++ platform/rdkb_hal/src/platform/platform_hal.c
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:99
- This patch makes the
ifconfigcommand useg_virtualIfName, but keepscmd[32]and usessprintf(). With a non-trivial interface name, this can overflowcmdand corrupt memory. Increase the buffer and usesnprintf().
char cmd[32] = {0};
- sprintf(cmd,"ifconfig %s %s",ETH_WAN_INTERFACE, enable ? "up":"down");
+ sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down");
system(cmd);
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:107
path[32]is too small for "/sys/class/net//carrier" when<ifname>is variable, andsprintf()can overflow it. This can break link detection and cause memory corruption; widen the buffer and usesnprintf().
int link = 0;
char path[32] = {0};
- sprintf(path, "/sys/class/net/erouter0/carrier");
+ sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName);
| SRC_URI_append += "file://configurable-wan-interface-ethsw.patch" | ||
| SRC_URI_append += "file://vlan-manager-integration.patch" | ||
|
|
||
| CFLAGS_append = "${@bb.utils.contains('DISTRO_FEATURES', 'vlan_manager' , ' -DFEATURE_RDKB_VLAN_MANAGER ','', d)}" |
| char path[32] = {0}; | ||
|
|
||
| +#ifdef FEATURE_RDKB_VLAN_MANAGER /* link status of lan0 has to be monitored*/ | ||
| + snprintf(path, sizeof(path), "/sys/class/net/%s/carrier", ETH_WAN_IFNAME); | ||
| +#else | ||
| sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName); |
| + else{ | ||
| + safec_rc=strcpy_s(current_wan_ifname, sizeof(current_wan_ifname),default_wan_ifname); | ||
| + ERR_CHK(safec_rc); | ||
| + safec_rc=strcpy_s(ecm_wan_ifname, sizeof(ecm_wan_ifname),default_wan_ifname); | ||
| + ERR_CHK(safec_rc); | ||
| + } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (11)
meta-rdk-mtk-bpir4/recipes-ccsp/hal/hal-ethsw-generic_git.bbappend:9
CFLAGS_append = ...overwrites the earlierCFLAGS_append += " -D_PLATFORM_BANANAPI_R4_ ", so the platform define gets dropped whenvlan_manageris enabled/disabled. This should be an append rather than an assignment.
SRC_URI_append += "file://configurable-wan-interface-ethsw.patch"
SRC_URI_append += "file://vlan-manager-integration.patch"
CFLAGS_append = "${@bb.utils.contains('DISTRO_FEATURES', 'vlan_manager' , ' -DFEATURE_RDKB_VLAN_MANAGER ','', d)}"
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-platform.patch:2
- This patch header references
platform_hal.c_orig, which is unlikely to exist in the hal-platform source tree. With the usual-p1patch strip level, this will attempt to patchrdkb_hal/src/platform/platform_hal.c_origand fail. Align the header paths with the other HAL patches (useplatform.orig/.../platform_hal.c).
--- platform/rdkb_hal/src/platform/platform_hal.c_orig
+++ platform/rdkb_hal/src/platform/platform_hal.c
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:40
strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1)does not guarantee NUL-termination whenout_valuefills the buffer, which can lead to non-terminated interface names being passed to later calls. Prefersnprintf(or add an explicit terminator).
+ fprintf(fp, "%s", out_value);
+ strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1);
+ fclose(fp);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:51
- Same
strncpy(..., sizeof(wanPhyName) - 1)NUL-termination issue here; use a bounded formatting/copy that guarantees termination.
+ {
+ fprintf(fp, "%s", out_value);
+ strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1);
+ fclose(fp);
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:66
- Same
strncpy(..., sizeof(wanPhyName) - 1)NUL-termination issue here;fileValuecan also be exactly the buffer size. Usesnprintf(or explicitly terminate afterstrncpy).
+ if ( syscfg_set_commit( NULL, "wan_physical_ifname", fileValue ) != 0 )
+ strcpy(wanPhyName, "erouter0");
+ else
+ strncpy(wanPhyName,fileValue, sizeof(wanPhyName) - 1);
+ }
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:69
- Same
strncpy(..., sizeof(wanPhyName) - 1)NUL-termination issue here.
+ else
+ strncpy(wanPhyName, out_value, sizeof(wanPhyName) - 1);
+ }
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/ccsp-psm/bbhm_def_cfg_banana.xml:1337
- The default config updates EthLink and WanManager VirtualInterface names to
erouter1, butdmsb.vlanmanager.1.nameremainserouter0. This inconsistency can lead to VLAN manager / WAN manager referring to different virtual interfaces.
<Record name="dmsb.ethlink.1.name" type="astr">erouter1</Record>
meta-rdk-mtk-bpir4/recipes-ccsp/ccsp/files/0001-Implement-WAN-interface-handling-for-multiple-platfo.patch:79
- This command writes
/tmp/wan_name.txtbut the file is never referenced anywhere else in this layer. Keeping it causes unnecessary shell-outs during init and can fail in constrained environments. Remove it unless it is intentionally used elsewhere.
+ v_secure_system("syscfg get wan_physical_ifname > /tmp/wan_name.txt");
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:99
- The patch replaces a constant interface name with
g_virtualIfNamebut keepssprintf(cmd, ...)into a 32-byte buffer. With a configurable interface name this can overflowcmd. At minimum switch the added line tosnprintf(cmd, sizeof(cmd), ...)to avoid overflow (even if it truncates).
char cmd[32] = {0};
- sprintf(cmd,"ifconfig %s %s",ETH_WAN_INTERFACE, enable ? "up":"down");
+ sprintf(cmd,"ifconfig %s %s",g_virtualIfName, enable ? "up":"down");
system(cmd);
meta-rdk-mtk-bpir4/recipes-ccsp/hal/files/configurable-wan-interface-ethsw.patch:108
- Similar to the
cmdcase, the patch changes tosprintf(path, ...)withg_virtualIfNamebutpathis only 32 bytes. This can overflow when the interface name is longer than expected. Usesnprintf(path, sizeof(path), ...)on the added line.
int link = 0;
char path[32] = {0};
- sprintf(path, "/sys/class/net/erouter0/carrier");
+ sprintf(path, "/sys/class/net/%s/carrier",g_virtualIfName);
link = is_interface_link(path);
meta-rdk-mtk-bpir4/recipes-rdkb/sysint-broadband/sysint-broadband.bbappend:25
- The new installs assume
${D}${base_libdir}/rdkalready exists. If the directory is not created by the base recipe in some build configurations,installwill fail. Create the directory explicitly before installing into it.
install -m 0755 ${S}/devicebpi/scripts/task_health_monitor.sh ${D}/usr/ccsp/tad
install -m 0755 ${S}/devicebpi/scripts/dca_utility.sh ${D}${base_libdir}/rdk
install -m 0755 ${S}/devicebpi/scripts/DCMscript.sh ${D}${base_libdir}/rdk
install -m 0755 ${S}/devicebpi/scripts/uploadSTBLogs.sh ${D}${base_libdir}/rdk
…interface-handling-for-multiple-platfo.patch
…DCMSetting.conf-is-empty-due-to-Config.patch
…DKBACCL-1961-Error-flooding-in-Console.patch
…erface-ethsw.patch
Removed configurable-wan-platform.patch from SRC_URI.
…-SSH-and-WebUI-not-working-Ethernet-Con.patch
Removed installation of several scripts from the sysint-broadband.bbappend file.
Reason for change:Configurable WAN feature Changes required for customisation to work if done Test Procedure: Build and flash the image with customised wan Risks: None