RDKB-63242 : Support for iproute2 to create macvlan#31
RDKB-63242 : Support for iproute2 to create macvlan#31S-Parthiban-Selvaraj wants to merge 4 commits intomainfrom
Conversation
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
There was a problem hiding this comment.
Pull request overview
Adds support for creating/deleting untagged interfaces via iproute2 (macvlan), and updates the Ethernet Link data model to allow signed MAC address offsets.
Changes:
- Introduces
EthLink_Get/SetParamIntValueand updatesMACAddrOffSethandling to be signed. - Creates/deletes untagged interfaces using
ip link add ... type macvlanwhen HALs are disabled. - Updates RDK VLAN manager XML to expose
MACAddrOffSetas a writableint.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| source/TR-181/middle_layer_src/vlan_apis.c | Switches MAC offset retrieval to an int getter. |
| source/TR-181/middle_layer_src/ethernet_dml.h | Adds Get/SetParamIntValue API declarations. |
| source/TR-181/middle_layer_src/ethernet_dml.c | Implements new int getter/setter for MACAddrOffSet. |
| source/TR-181/middle_layer_src/ethernet_apis.c | Adds iproute2 macvlan create/delete path for untagged interfaces. |
| source/TR-181/include/ethernet_apis.h | Changes MACAddrOffSet to signed (LONG). |
| config/RdkVlanManager.xml | Changes MACAddrOffSet datatype to writable int. |
Comments suppressed due to low confidence (1)
source/TR-181/middle_layer_src/vlan_apis.c:452
- The error log message is misleading: this is reading
MACAddrOffSet, but the message says 'Failed to set Enable data model'. Update the message to reflect the actual operation (e.g., failure to getMACAddrOffSet).
if (EthLink_GetParamIntValue(pNewEntry, "MACAddrOffSet", pOffSet) != TRUE)
{
CcspTraceError(("%s - Failed to set Enable data model\n", __FUNCTION__));
return ANSC_STATUS_FAILURE;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EthLink_GetParamIntValue | ||
| ( | ||
| ANSC_HANDLE hInsContext, | ||
| char* ParamName, | ||
| int* pInt | ||
| ) | ||
| { | ||
| PDML_ETHERNET p_EthLink = (PDML_ETHERNET )hInsContext; | ||
|
|
||
| /* check the parameter name and return the corresponding value */ | ||
| if (strcmp(ParamName, "MACAddrOffSet") == 0) | ||
| { | ||
| *puLong = p_EthLink->MACAddrOffSet; | ||
| *pInt = p_EthLink->MACAddrOffSet; | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
p_EthLink->MACAddrOffSet is changed to LONG (see ethernet_apis.h), but the new getter/setter use int (int* / int). This can truncate on platforms where LONG is wider than int. Use a consistent signed type end-to-end (e.g., change these APIs to LONG*/LONG, or keep the struct field as a 32-bit signed type if the data model guarantees 32-bit).
| EthLink_SetParamIntValue | ||
| ( | ||
| ANSC_HANDLE hInsContext, | ||
| char* ParamName, | ||
| int iValue | ||
| ) | ||
| { | ||
| PDML_ETHERNET p_EthLink = (PDML_ETHERNET )hInsContext; | ||
|
|
||
| /* check the parameter name and set the corresponding value */ | ||
| if (strcmp(ParamName, "MACAddrOffSet") == 0) | ||
| { | ||
| p_EthLink->MACAddrOffSet = iValue; | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
p_EthLink->MACAddrOffSet is changed to LONG (see ethernet_apis.h), but the new getter/setter use int (int* / int). This can truncate on platforms where LONG is wider than int. Use a consistent signed type end-to-end (e.g., change these APIs to LONG*/LONG, or keep the struct field as a 32-bit signed type if the data model guarantees 32-bit).
|
|
||
| // Get MAC address with offset applied | ||
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | ||
| { | ||
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | ||
| // Create MACVLAN without setting custom MAC - kernel will assign one | ||
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | ||
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); | ||
|
|
||
| // Create MACVLAN interface with custom MAC | ||
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | ||
| } | ||
|
|
||
| v_secure_system("ip link set %s up", pEntry->Name); | ||
|
|
||
| CcspTraceInfo(("%s-%d: Successfully created MACVLAN untagged interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); |
There was a problem hiding this comment.
Creation path doesn't check v_secure_system(...) return values, but logs success unconditionally. This can report an interface as created/up even when ip link add/ip link set fails. Capture and validate command return codes, and set returnStatus accordingly (and avoid printing a success log unless all steps succeed).
| // Get MAC address with offset applied | |
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | |
| { | |
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | |
| // Create MACVLAN without setting custom MAC - kernel will assign one | |
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | |
| pEntry->BaseInterface, pEntry->Name); | |
| } | |
| else | |
| { | |
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | |
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); | |
| // Create MACVLAN interface with custom MAC | |
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | |
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | |
| } | |
| v_secure_system("ip link set %s up", pEntry->Name); | |
| CcspTraceInfo(("%s-%d: Successfully created MACVLAN untagged interface %s\n", | |
| __FUNCTION__, __LINE__, pEntry->Name)); | |
| int cmdStatus = -1; | |
| // Get MAC address with offset applied | |
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | |
| { | |
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | |
| // Create MACVLAN without setting custom MAC - kernel will assign one | |
| cmdStatus = v_secure_system("ip link add link %s name %s type macvlan mode bridge", | |
| pEntry->BaseInterface, pEntry->Name); | |
| } | |
| else | |
| { | |
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | |
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); | |
| // Create MACVLAN interface with custom MAC | |
| cmdStatus = v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | |
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | |
| } | |
| if (cmdStatus != 0) | |
| { | |
| CcspTraceError(("%s-%d: Failed to create MACVLAN untagged interface %s (base: %s), cmdStatus=%d\n", | |
| __FUNCTION__, __LINE__, pEntry->Name, pEntry->BaseInterface, cmdStatus)); | |
| returnStatus = ANSC_STATUS_FAILURE; | |
| } | |
| else | |
| { | |
| cmdStatus = v_secure_system("ip link set %s up", pEntry->Name); | |
| if (cmdStatus != 0) | |
| { | |
| CcspTraceError(("%s-%d: Failed to set MACVLAN interface %s up, cmdStatus=%d\n", | |
| __FUNCTION__, __LINE__, pEntry->Name, cmdStatus)); | |
| returnStatus = ANSC_STATUS_FAILURE; | |
| } | |
| } | |
| if (returnStatus == ANSC_STATUS_SUCCESS) | |
| { | |
| CcspTraceInfo(("%s-%d: Successfully created MACVLAN untagged interface %s\n", | |
| __FUNCTION__, __LINE__, pEntry->Name)); | |
| } |
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | ||
| } | ||
|
|
||
| v_secure_system("ip link set %s up", pEntry->Name); | ||
|
|
||
| CcspTraceInfo(("%s-%d: Successfully created MACVLAN untagged interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); |
There was a problem hiding this comment.
Creation path doesn't check v_secure_system(...) return values, but logs success unconditionally. This can report an interface as created/up even when ip link add/ip link set fails. Capture and validate command return codes, and set returnStatus accordingly (and avoid printing a success log unless all steps succeed).
| v_secure_system("ip link set %s down", pEntry->Name); | ||
| v_secure_system("ip link delete %s", pEntry->Name); | ||
|
|
||
| CcspTraceInfo(("%s-%d: Successfully deleted untagged VLAN interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); |
There was a problem hiding this comment.
Delete path also ignores v_secure_system(...) return values and always returns ANSC_STATUS_SUCCESS while logging success. Propagate failures (e.g., if interface doesn't exist or delete fails) by checking return codes and returning ANSC_STATUS_FAILURE (or a more specific status if available).
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name); |
There was a problem hiding this comment.
These ip link commands are constructed with %s values from pEntry (interface names, base interface, MAC string). If those strings are not strictly validated elsewhere, this pattern risks command injection or unexpected shell parsing. Prefer executing ip with an argv-style API (no shell), or enforce an allowlist validation for interface names (e.g., [A-Za-z0-9_.-]+) and MAC format before use.
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); |
There was a problem hiding this comment.
These ip link commands are constructed with %s values from pEntry (interface names, base interface, MAC string). If those strings are not strictly validated elsewhere, this pattern risks command injection or unexpected shell parsing. Prefer executing ip with an argv-style API (no shell), or enforce an allowlist validation for interface names (e.g., [A-Za-z0-9_.-]+) and MAC format before use.
| v_secure_system("ip link set %s down", pEntry->Name); | ||
| v_secure_system("ip link delete %s", pEntry->Name); |
There was a problem hiding this comment.
These ip link commands are constructed with %s values from pEntry (interface names, base interface, MAC string). If those strings are not strictly validated elsewhere, this pattern risks command injection or unexpected shell parsing. Prefer executing ip with an argv-style API (no shell), or enforce an allowlist validation for interface names (e.g., [A-Za-z0-9_.-]+) and MAC format before use.
| #elif !defined(COMCAST_VLAN_HAL_ENABLED) | ||
| // Delete untagged VLAN interface using ip link commands when both HALs are disabled |
There was a problem hiding this comment.
The comment says 'when both HALs are disabled', but this branch condition only checks !defined(COMCAST_VLAN_HAL_ENABLED) (and is an #elif, so it depends on the preceding #if). Either adjust the preprocessor condition to explicitly match 'both HALs disabled', or update the comment to accurately describe the actual condition.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Deletes it if it exists (works for both MACVLAN and bridge interfaces) by: Bringing the interface down with ip link set down Deleting it with ip link delete Creates the new MACVLAN interface as before
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CHAR Path[1024]; | ||
| CHAR MACAddress[18]; | ||
| ULONG MACAddrOffSet; | ||
| LONG MACAddrOffSet; // Changed to LONG to support negative offsets |
There was a problem hiding this comment.
Type inconsistency detected: MACAddrOffSet is declared as LONG in the structure, but the getter and setter functions use 'int' type for parameters. This mismatch can lead to issues on platforms where LONG and int have different sizes (e.g., 64-bit systems where LONG is 64-bit but int is 32-bit). The function signatures in ethernet_dml.h and ethernet_dml.c use 'int*' and 'int' respectively, while the struct field is LONG. All types should be consistent - either all should be LONG or all should be int.
| LONG MACAddrOffSet; // Changed to LONG to support negative offsets | |
| INT MACAddrOffSet; // Offset can be negative |
| v_secure_system("ip link set %s down", pEntry->Name); | ||
| v_secure_system("ip link delete %s", pEntry->Name); |
There was a problem hiding this comment.
No error handling for v_secure_system calls. The ip link commands to bring down and delete the interface can fail silently (e.g., if the interface doesn't exist or permissions are insufficient). The function returns ANSC_STATUS_SUCCESS regardless of whether the deletion actually succeeded. Consider checking return values and only returning success if the interface was actually deleted.
| if (EthLink_GetParamUlongValue(pNewEntry, "MACAddrOffSet", pOffSet) != TRUE) | ||
| if (EthLink_GetParamIntValue(pNewEntry, "MACAddrOffSet", pOffSet) != TRUE) | ||
| { | ||
| CcspTraceError(("%s - Failed to set Enable data model\n", __FUNCTION__)); |
There was a problem hiding this comment.
Misleading error message: The error message states "Failed to set Enable data model" but the actual operation is getting the MACAddrOffSet parameter, not setting Enable. The error message should reflect the actual operation that failed, e.g., "Failed to get MACAddrOffSet parameter".
| CcspTraceError(("%s - Failed to set Enable data model\n", __FUNCTION__)); | |
| CcspTraceError(("%s - Failed to get MACAddrOffSet parameter\n", __FUNCTION__)); |
| // Check if interface already exists (as MACVLAN or bridge) and delete it | ||
| CcspTraceInfo(("%s-%d: Checking if interface %s already exists\n", __FUNCTION__, __LINE__, pEntry->Name)); | ||
| v_secure_system("ip link show %s > /dev/null 2>&1 && (ip link set %s down; ip link delete %s)", | ||
| pEntry->Name, pEntry->Name, pEntry->Name); |
There was a problem hiding this comment.
Potential race condition: The interface existence check and deletion are not atomic. Between checking if the interface exists and deleting it, another process could delete it, causing the deletion command to fail. Similarly, between deletion and creation, another process could create an interface with the same name. While this is unlikely in practice, consider using a more robust approach or at least adding error checking to handle these edge cases gracefully.
| // Set MTU to default 1500 | ||
| CcspTraceInfo(("%s-%d: Setting MTU to 1500 for MACVLAN interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); | ||
| v_secure_system("ip link set %s mtu 1500", pEntry->Name); |
There was a problem hiding this comment.
Hardcoded MTU value: The MTU is hardcoded to 1500, which may not be appropriate for all network configurations. Some networks require jumbo frames (MTU > 1500) or have specific MTU requirements. Consider making the MTU configurable or deriving it from the base interface's MTU to ensure proper network operation in various deployment scenarios.
| v_secure_system("ip link show %s > /dev/null 2>&1 && (ip link set %s down; ip link delete %s)", | ||
| pEntry->Name, pEntry->Name, pEntry->Name); | ||
|
|
||
| // Get MAC address with offset applied | ||
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | ||
| { | ||
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | ||
| // Create MACVLAN without setting custom MAC - kernel will assign one | ||
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | ||
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); | ||
|
|
||
| // Create MACVLAN interface with custom MAC | ||
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | ||
| } | ||
|
|
||
| // Set MTU to default 1500 | ||
| CcspTraceInfo(("%s-%d: Setting MTU to 1500 for MACVLAN interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); | ||
| v_secure_system("ip link set %s mtu 1500", pEntry->Name); | ||
|
|
||
| v_secure_system("ip link set %s up", pEntry->Name); |
There was a problem hiding this comment.
Missing input validation: The interface name (pEntry->Name) and base interface (pEntry->BaseInterface) are passed directly to shell commands without validation. While v_secure_system provides some protection, malformed or malicious interface names could still cause issues. Consider validating that these strings contain only allowed characters (e.g., alphanumeric, dash, underscore) before using them in system commands to prevent potential command injection or unexpected behavior.
| v_secure_system("ip link show %s > /dev/null 2>&1 && (ip link set %s down; ip link delete %s)", | ||
| pEntry->Name, pEntry->Name, pEntry->Name); | ||
|
|
||
| // Get MAC address with offset applied | ||
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | ||
| { | ||
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | ||
| // Create MACVLAN without setting custom MAC - kernel will assign one | ||
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | ||
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); | ||
|
|
||
| // Create MACVLAN interface with custom MAC | ||
| v_secure_system("ip link add link %s name %s address %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress); | ||
| } | ||
|
|
||
| // Set MTU to default 1500 | ||
| CcspTraceInfo(("%s-%d: Setting MTU to 1500 for MACVLAN interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); | ||
| v_secure_system("ip link set %s mtu 1500", pEntry->Name); | ||
|
|
||
| v_secure_system("ip link set %s up", pEntry->Name); | ||
|
|
||
| CcspTraceInfo(("%s-%d: Successfully created MACVLAN untagged interface %s\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); |
There was a problem hiding this comment.
No error handling for v_secure_system calls. The ip link commands to delete and create MACVLAN interfaces can fail for various reasons (interface doesn't exist, permissions, invalid parameters, etc.), but failures are not detected or handled. This could lead to the function returning success when the interface creation actually failed. Consider checking return values and propagating errors appropriately. At minimum, verify the interface was created successfully before returning ANSC_STATUS_SUCCESS.
| CcspTraceInfo(("%s-%d: Creating MACVLAN untagged interface %s on base interface %s with MAC offset %ld\n", | ||
| __FUNCTION__, __LINE__, pEntry->Name, pEntry->BaseInterface, pEntry->MACAddrOffSet)); | ||
|
|
||
| // Check if interface already exists (as MACVLAN or bridge) and delete it | ||
| CcspTraceInfo(("%s-%d: Checking if interface %s already exists\n", __FUNCTION__, __LINE__, pEntry->Name)); | ||
| v_secure_system("ip link show %s > /dev/null 2>&1 && (ip link set %s down; ip link delete %s)", | ||
| pEntry->Name, pEntry->Name, pEntry->Name); | ||
|
|
||
| // Get MAC address with offset applied | ||
| if (ANSC_STATUS_SUCCESS != EthLink_GetMacAddr(pEntry)) | ||
| { | ||
| CcspTraceError(("%s-%d: Failed to get MAC address, creating MACVLAN without custom MAC\n", __FUNCTION__, __LINE__)); | ||
| // Create MACVLAN without setting custom MAC - kernel will assign one | ||
| v_secure_system("ip link add link %s name %s type macvlan mode bridge", | ||
| pEntry->BaseInterface, pEntry->Name); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceInfo(("%s-%d: Using MAC address: %s (offset: %ld)\n", | ||
| __FUNCTION__, __LINE__, pEntry->MACAddress, pEntry->MACAddrOffSet)); |
There was a problem hiding this comment.
Format specifier mismatch: MACAddrOffSet is declared as LONG in the structure but formatted with %ld. While this works on most systems, if the type is actually 'int' (as used in the getter/setter functions), this could be incorrect. The format specifier should match the actual type being used. If MACAddrOffSet is int, use %d; if it's LONG, use %ld (or %lld on some systems).
No description provided.