-
Notifications
You must be signed in to change notification settings - Fork 44
RDKBNETWOR-66 : Implement MAP-E in RdkWanManager #134
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: main
Are you sure you want to change the base?
Conversation
Reason for change: Receive the MAP-E options from DhcpManager and configure the tunnel interface. Test Procedure: Verify functionality by passing DHCPv6 Option 94 and the ip6tnl tunnel interface is correctly configured and brought up. Testing Done : Results are captured in RDKBNETWOR-66 Risks: None. Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
Reason for change: Enclose MAP-E logic within FEATURE_MAPE flag Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
Reason for change: Write the PSID vlaues to /proc/nat_port so that the kernel reads these value and configures correct ports for MAPE. Test Procedure: Verify functionality by passing DHCPv6 Option 94 and the ip6tnl tunnel interface is correctly configured and brought up. Testing Done : Results are captured in RDKBNETWOR-66 Risks: None. Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
| char wan_interface[BUFLEN_64]={0}; | ||
|
|
||
| CcspTraceInfo(("%s %d - stop MAP-E\n", __FUNCTION__, __LINE__)); | ||
| if (syscfg_get(NULL, SYSCFG_WAN_INTERFACE, wan_interface, sizeof(wan_interface)) != ANSC_STATUS_SUCCESS) |
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.
Why do we need a syscfg, we can use the current interface name.
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.
Addressed
| } | ||
| #endif | ||
|
|
||
| #ifdef FEATURE_MAPE |
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.
Isn't it similar to the previous if condition for the MAPE? can we have single condition?
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.
Yes, merged to single if condition
| CcspTraceError(("%s %d Error resetting MAP-T configuration", __FUNCTION__, __LINE__)); | ||
| } | ||
|
|
||
| /* Clear DHCPv4 client */ |
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.
Starting DHCPv4 should be common for both MAPE and MAPT.
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.
Moved outside if condition
| CcspTraceInfo(("%s %d: Stopping DHCP v4\n", __FUNCTION__, __LINE__)); | ||
| WanManager_StopDhcpv4Client(p_VirtIf, STOP_DHCP_WITH_RELEASE); | ||
| } | ||
| if (p_VirtIf->IP.Dhcp4cStatus == DHCPC_STARTED) |
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.
Stopping DHCPv4 should be common for both MAPE and MAPT
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.
Moved outside MAPT condition
source/WanManager/wanmgr_net_utils.c
Outdated
| inet_ntop(AF_INET6, &in6, tunnel_source, sizeof(tunnel_source)); | ||
|
|
||
| //tunnel_source | ||
| ret = v_secure_system("ip -6 tunnel add %s mode ip4ip6 remote %s local %s encaplimit none dev %s", TUNNEL_NAME, br_ipv6_prefix, tunnel_source, "erouter0"); |
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.
Please don't hard code erouter0
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.
Addressed
| #endif //FEATURE_MAPT | ||
|
|
||
| #ifdef FEATURE_MAPE | ||
| else if ( pInterface->Selection.Status == WAN_IFACE_ACTIVE && p_VirtIf->Status == WAN_IFACE_STATUS_UP && p_VirtIf->MAP.dhcp6cMAPparameters.mapType == MAP_TYPE_MAPE && p_VirtIf->MAP.MapeStatus == WAN_IFACE_MAPE_STATE_UP) |
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.
Please add it with the MAPT if condition itself. Both are same transition
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.
Combined the conditions
| } | ||
| #endif //FEATURE_MAPT | ||
|
|
||
| #ifdef FEATURE_MAPE |
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.
Lets keep both transitions together.
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.
Combined the conditions
source/WanManager/wanmgr_net_utils.c
Outdated
| #define BUFF_SIZE_64 64 | ||
| #define BUFF_SIZE_1024 1024 | ||
| #define IPV4_ADDR_LEN_IN_BITS 32 | ||
| #define BUF_LEN (10 * (sizeof(struct inotify_event) + 255 + 1)) |
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.
is this used in the code?
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.
Removed unused macros
source/WanManager/wanmgr_net_utils.c
Outdated
| } | ||
|
|
||
| #ifdef FEATURE_MAPE | ||
| ANSC_STATUS WanManager_MAPEConfiguration(ipc_map_data_t *dhcp6cMAPEMsgBody) |
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.
Can this function be moved to a new file for MAPE?
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.
Moved to new file as suggested
1b28a7c to
109927b
Compare
Reason for change: Address the review comments. Test Procedure: Verify functionality by passing DHCPv6 Option 94 and the ip6tnl tunnel interface is correctly configured and brought up. Testing Done : Results are captured in RDKBNETWOR-66 Risks: None. Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
109927b to
5185d67
Compare
|
Hi @Krithiksha11 : Please append a credit to NOTICE at top level, to match the new DT code: I will clear off the BD failure. |
Reason for change: Appended a credit to NOTICE at top level, to match the new DT code. Risks: None. Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
Hi @mhughesacn, I have added the same. Please review it. Thank you |
|
Thank you @Krithiksha11 |
config/RdkWanManager_v2.xml
Outdated
| <name>Enable</name> | ||
| <type>boolean</type> | ||
| <syntax>bool</syntax> | ||
| <writable>true</writable> |
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.
I think it would be better to keep these DMLs read-only, as we're only using RBUS events or IPC methods to send lease details to the WAN Manager.
If these DMLs are writable, it opens up the possibility for someone to modify the configuration directly, which could conflict with the current design.
I understand that the BBF specification defines them as writable, but allowing write access here could introduce issues.
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.
Addressed
|
Hi @Krithiksha11 : I just noticed this: If you make any more changes to this PR, please will you fix the headers on the new files you created. They say "this component's Licenses.txt file", but newer headers say "this component's LICENSE file". We changed the license name a long time ago, but headers have been slow to catch up. Many thanks! |
Reason for change: Address the review comments. Test Procedure: Verify functionality by passing DHCPv6 Option 94 and the ip6tnl tunnel interface is correctly configured and brought up. Testing Done : Results are captured in RDKBNETWOR-66 Risks: None. Signed-off-by: Krithiksha Prabhakar <krithiksha.prabhakar@telekom-digital.com>
1f6e090 to
17f486c
Compare
Hi @mhughesacn, I have addressed the requested change. Please review them. Thank you! |
|
Looks good - thank you! |
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 implements MAP-E support in RdkWanManager by adding the capability to receive MAP-E options from DhcpManager and configure the tunnel interface accordingly. The implementation includes creating a new MAP-E configuration module, updating the state machine to handle MAP-E transitions, and providing TR-181 data model support for MAP objects.
- Unifies MAP-T and MAP-E handling in the WAN interface state machine
- Adds new MAP-E configuration module with tunnel setup functionality
- Introduces comprehensive TR-181 data model support for MAP objects
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| source/WanManager/wanmgr_sysevents.h | Adds MAP-E feature flag definitions and syscfg variables |
| source/WanManager/wanmgr_sysevents.c | Updates state machine to use unified MAP_ACTIVE state |
| source/WanManager/wanmgr_net_utils.h | Updates function signatures to use unified MAP data structure |
| source/WanManager/wanmgr_net_utils.c | Refactors MAP-T functions to use unified data structure |
| source/WanManager/wanmgr_mape.h | New header defining MAP-E configuration functions |
| source/WanManager/wanmgr_mape.c | New module implementing MAP-E tunnel configuration |
| source/WanManager/wanmgr_interface_sm.h | Adds MAP-E interface status types |
| source/WanManager/wanmgr_interface_sm.c | Implements unified MAP state machine transitions |
| source/WanManager/wanmgr_dhcpv6_apis.c | Updates to use unified MAP data structure |
| source/WanManager/wanmgr_dhcp_event_handler.c | Adds MAP-E event handling support |
| source/WanManager/wanmgr_data.c | Adds MAP data structure initialization |
| source/WanManager/Makefile.am | Includes new MAP-E source file in build |
| source/TR-181/middle_layer_src/* | Complete TR-181 data model implementation for MAP objects |
| config/RdkWanManager_v2.xml | XML configuration for MAP TR-181 objects |
| NOTICE | Updates copyright to include Deutsche Telekom |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
source/WanManager/wanmgr_mape.c
Outdated
| syscfg_unset (NULL, "mape_psid"); | ||
| syscfg_unset (NULL, "mape_psid_len"); | ||
| syscfg_unset (NULL, "mape_ipv4_address"); | ||
| syscfg_unset (NULL, "mape_ipv4_address"); |
Copilot
AI
Sep 11, 2025
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.
Duplicate syscfg_unset call for 'mape_ipv4_address' on consecutive lines 245 and 246. Remove the duplicate call.
| syscfg_unset (NULL, "mape_ipv4_address"); |
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.
Removed the duplicate call.
| CcspTraceInfo(("%s %d - stop MAP-E\n", __FUNCTION__, __LINE__)); | ||
|
|
||
| snprintf(cmd, sizeof(cmd), "iproute del default dev %s", p_VirtIf->Name); | ||
| system(cmd); |
Copilot
AI
Sep 11, 2025
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.
Direct system() calls should be replaced with secure_wrapper functions like v_secure_system() for security. The cmd variable on line 346 is constructed from user input and should also use secure execution.
| system(cmd); | |
| v_secure_system(cmd); |
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.
Replaced system() with v_secure_system() calls.
| system(cmd); | ||
|
|
||
| system("ip link set dev ip6tnl down"); | ||
| system("ip -6 tunnel del ip6tnl"); |
Copilot
AI
Sep 11, 2025
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.
Direct system() calls should be replaced with secure_wrapper functions like v_secure_system() for security. The cmd variable on line 346 is constructed from user input and should also use secure execution.
| system(cmd); | |
| system("ip link set dev ip6tnl down"); | |
| system("ip -6 tunnel del ip6tnl"); | |
| v_secure_system(cmd); | |
| v_secure_system("ip link set dev ip6tnl down"); | |
| v_secure_system("ip -6 tunnel del ip6tnl"); |
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.
Replaced system() with v_secure_system() calls.
fbf6e57 to
5b4c096
Compare
5b4c096 to
ef719db
Compare
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Reason for change: Receive the MAP-E options from DhcpManager and configure the tunnel interface.
Test Procedure: Verify functionality by passing DHCPv6 Option 94 and the ip6tnl tunnel interface is correctly configured and brought up.
Testing Done : Results are captured in RDKBNETWOR-66
Risks: None.