RDKB-65961 : MACVLAN - #39
Open
PalakshaOS wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TR-181 VLAN parameter (MACVLAN) and begins integrating it into untagged interface creation/deletion logic to support MACVLAN-based “truly untagged” traffic handling.
Changes:
- Added
MACVLANto the VLAN data model (XML + DML get/set) andDML_VLANstruct storage. - Loaded
macvlanconfiguration from PSM during VLAN termination initialization. - Added MACVLAN-based creation/deletion branches in
ethernet_apis.cfor untagged interface handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| source/TR-181/middle_layer_src/vlan_internal.c | Reads MACVLAN value from PSM into the VLAN termination config structure. |
| source/TR-181/middle_layer_src/vlan_dml.c | Exposes MACVLAN via TR-181 get/set handlers. |
| source/TR-181/middle_layer_src/ethernet_apis.c | Adds MACVLAN/bridge branching for untagged interface create/delete (currently has compile/API issues). |
| source/TR-181/include/vlan_apis.h | Extends DML_VLAN with a macvlan field. |
| config/RdkVlanManager.xml | Adds MACVLAN parameter definition to the data model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
875
to
876
| if (pEntry->MACVLAN) { | ||
| // Create untagged interface using MACVLAN for truly untagged traffic |
Comment on lines
+919
to
+920
| else if (pEntry->VLANId == -1) | ||
| { |
| return ANSC_STATUS_FAILURE; | ||
| } | ||
|
|
||
| if(Entry->MACVLAN) { |
Comment on lines
+967
to
+968
| CcspTraceInfo(("%s-%d: Deleting untagged without MACVLAN %s/n", | ||
| __FUNCTION__, __LINE__, pEntry->Name)); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
source/TR-181/middle_layer_src/ethernet_apis.c:943
EthLink_DeleteUnTaggedInterface()references undefined/invalid fields (Entry->MACVLANandpEntry->VLANId).PDML_ETHERNEThas noMACVLANorVLANIdmembers, so this won’t compile, and it also diverges from the current create path (which always creates an untagged MACVLAN interface when both HALs are disabled).
if(Entry->MACVLAN) {
CcspTraceInfo(("%s-%d: Deleting untagged VLAN interface %s\n",
__FUNCTION__, __LINE__, pEntry->Name));
v_secure_system("ip link set %s down", pEntry->Name);
Comment on lines
+405
to
+409
| else if (pEntry->VLANId == -1) | ||
| { | ||
| /* If the VLANID = -1, the VLAN is a bridge, delete the bridge and delete the interface from the bridge */ | ||
| if(strcmp(pEntry->BaseInterface, pEntry->Name)) | ||
| { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/TR-181/include/vlan_apis.h:95
DML_VLAN_INITdoes not initialize the newly addedmacvlanmember. Some call sites (e.g., rollback/new entry paths) callDML_VLAN_INITwithout necessarily zeroing the whole struct first, which can makemacvlanindeterminate and affect enable/disable branching. Initialize it explicitly to a known default (0).
UINT macvlan;
CHAR Path[1024];
}
DML_VLAN, *PDML_VLAN;
Comment on lines
+405
to
+409
| else if (pEntry->VLANId == -1) | ||
| { | ||
| /* If the VLANID = -1, the VLAN is a bridge, delete the bridge and delete the interface from the bridge */ | ||
| if(strcmp(pEntry->BaseInterface, pEntry->Name)) | ||
| { |
Comment on lines
+405
to
+409
| else if (pEntry->VLANId == -1) | ||
| { | ||
| /* If the VLANID = -1, the VLAN is a bridge, delete the bridge and delete the interface from the bridge */ | ||
| if(strcmp(pEntry->BaseInterface, pEntry->Name)) | ||
| { |
| pthread_exit(NULL); | ||
| } | ||
|
|
||
| CcspTraceError(("%s-%d: PALAKSHA MAIN VLAN_ENABLE VLAN ID CASE %d %d %s %s\n", __FUNCTION__, __LINE__, pEntry->VLANId,pEntry->macvlan, pEntry->BaseInterface, pEntry->Name)); |
| pthread_mutex_lock(&vlan_access_mutex); | ||
| //Create Vlan Tagged Interface | ||
| if(pEntry->VLANId > 0) { | ||
| CcspTraceError(("%s-%d: PALAKSHA MAIN VLAN_ENABLE VLAN ID GREATER CASE %d %d %s %s\n", __FUNCTION__, __LINE__, pEntry->VLANId,pEntry->macvlan, pEntry->BaseInterface, pEntry->Name)); |
Comment on lines
+405
to
+417
| else if (pEntry->VLANId == -1) | ||
| { | ||
| /* If the VLANID = -1, the VLAN is a bridge, delete the bridge and delete the interface from the bridge */ | ||
| if(strcmp(pEntry->BaseInterface, pEntry->Name)) | ||
| { | ||
| //v_secure_system("ifconfig %s down", pEntry->Name); | ||
| //v_secure_system("brctl delif %s %s", pEntry->Name, pEntry->BaseInterface); | ||
| //v_secure_system("brctl delbr %s", pEntry->Name); | ||
| v_secure_system("ip link set %s down", pEntry->Name); | ||
| v_secure_system("ip link set %s nomaster", pEntry->BaseInterface); | ||
| v_secure_system("ip link delete %s type bridge", pEntry->Name); | ||
| } | ||
| } |
Comment on lines
+405
to
+418
| else if (pEntry->VLANId == -1) | ||
| { | ||
| /* If the VLANID = -1, the VLAN is a bridge, delete the bridge and delete the interface from the bridge */ | ||
| if(strcmp(pEntry->BaseInterface, pEntry->Name)) | ||
| { | ||
| /* Remove interface from bridge */ | ||
| v_secure_system("ovs-vsctl --if-exists del-port %s", | ||
| pEntry->BaseInterface); | ||
|
|
||
| /* Delete bridge */ | ||
| v_secure_system("ovs-vsctl --if-exists del-br %s", | ||
| pEntry->Name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
source/TR-181/middle_layer_src/vlan_apis.c:680
- The
CcspTraceInfo("Successfully deleted...")log is executed unconditionally in the non-HAL build because the{ ... }block sits after#endif(theelseonly exists in the HAL branch). This can report a successful delete even when theip link deletepath fails or is skipped. Restructure the#if/#elseso the success log is only emitted on a known-success path, and use a different message (or check return codes) for the non-HAL path.
v_secure_system("ip link set %s down", pEntry->Name);
v_secure_system("ip link delete %s",pEntry->Name);
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:408
- This comment says the code will "delete the OVS bridge and remove the interface", but the implementation only triggers a sysevent. Please update the comment to reflect the actual behavior so future maintainers don't assume interface deletion happens here.
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
source/TR-181/middle_layer_src/vlan_apis.c:411
- The sysevent instance "23" is an unexplained magic number. Please derive it from configuration/instance context or at least centralize it as a named constant to avoid incorrect behavior if the instance changes.
v_secure_system("sysevent set multinet-down 23");
source/TR-181/middle_layer_src/vlan_apis.c:614
- This comment claims an OVS bridge is created here, but the function currently only signals
multinet-upand adjusts link/MAC state. Update the comment to match the actual behavior (or add the missing bridge creation if that's required).
/* If the VLANID = -1, create an OVS bridge and add the interface */
source/TR-181/middle_layer_src/vlan_internal.c:390
- The comment says "get cfg macvlan from psm" but this branch reads from
PSM_VLANMANAGER_MACVLAN(non-cfg key). Please update the comment to avoid confusion about which PSM namespace is used.
/* get cfg macvlan from psm */
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
source/TR-181/middle_layer_src/vlan_apis.c:661
- This log is error-level and contains a personal/debug marker; it should be removed or downgraded to an informational message to avoid misleading/error spam in normal operation.
CcspTraceError(("%s-%d: PALAKSHA MAIN VLAN_ENABLE VLAN ID GREATER CASE %d %d %s %s\n", __FUNCTION__, __LINE__, pEntry->VLANId,pEntry->macvlan, pEntry->BaseInterface, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:738
- This is error-level logging in the normal else branch and includes a personal/debug marker; it should be removed or downgraded to avoid misleading log severity and noise.
CcspTraceError(("%s-%d: PALAKSHA INSIDE FULL ELSE CASE calling Vlan_SetEthLink\n", __FUNCTION__, __LINE__));
source/TR-181/middle_layer_src/vlan_apis.c:686
- The current preprocessor/brace structure makes the success log unconditional in non-HAL builds (and conditional in HAL builds), which is confusing and can report success even when deletion fails. Restructure the #if/#else so the log is clearly associated with each branch.
v_secure_system("ip link set %s down", pEntry->Name);
v_secure_system("ip link delete %s",pEntry->Name);
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:412
- The comment says this branch deletes the OVS bridge/interface, but the code only triggers a sysevent. Either perform the deletion here or update the comment to reflect that teardown is handled elsewhere via the sysevent handler.
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-down 23");
}
source/TR-181/include/vlan_apis.h:93
- Field name
macvlanis inconsistent with the surrounding struct's naming (e.g., VLANId, TPId, BaseInterface). Consider renaming to something likeMacVlanorMacVlanEnabledfor consistency and readability.
CHAR BaseInterface[64];
INT VLANId;
UINT TPId;
UINT macvlan;
CHAR Path[1024];
| { | ||
| pEntry->Status = VLAN_IF_ERROR; | ||
| CcspTraceError(("[%s][%d]Failed to create VLAN Tagged interface \n", __FUNCTION__, __LINE__)); | ||
| CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__)); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
source/TR-181/include/vlan_apis.h:93
- The new struct field is named
macvlan, which is inconsistent with the surrounding DML_VLAN field naming (e.g.,VLANId,TPId). This makes the API harder to scan and can cause inconsistent naming across files. Consider renaming it to something likeMacVlanorMacVlanEnableand updating all references accordingly.
CHAR LowerLayers[1024];
CHAR BaseInterface[64];
INT VLANId;
UINT TPId;
UINT macvlan;
CHAR Path[1024];
source/TR-181/middle_layer_src/vlan_apis.c:412
- Hard-coded multinet instance
23is a magic number. This makes the behavior brittle and hard to maintain/operate across platforms where the multinet instance may differ. Please replace it with a named constant or derive it from configuration/instance context.
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-down 23");
}
source/TR-181/middle_layer_src/vlan_apis.c:624
- Hard-coded multinet instance
23is a magic number. This makes the behavior brittle and hard to maintain/operate across platforms where the multinet instance may differ. Please replace it with a named constant or derive it from configuration/instance context.
if(strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-up 23");
v_secure_system("ip link set %s up", pEntry->Name);
source/TR-181/middle_layer_src/vlan_apis.c:703
- This log line contains a personal identifier and an incorrect message (it says VLAN ID 0, but this branch is entered for VLANId == -1). Please correct the message and avoid logging it as an error.
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
source/TR-181/middle_layer_src/vlan_internal.c:390
- The comment says "get cfg macvlan from psm" but this code reads from
PSM_VLANMANAGER_MACVLAN(non-cfg namespace). Please correct the comment to avoid confusion during maintenance.
/* get cfg macvlan from psm */
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
source/TR-181/middle_layer_src/vlan_internal.c:390
- The comment says this is reading a "cfg" value, but the key used is the non-cfg PSM_VLANMANAGER_MACVLAN. Please update the comment to avoid confusion when debugging PSM layout.
/* get cfg macvlan from psm */
source/TR-181/middle_layer_src/vlan_apis.c:411
- This uses a hard-coded multinet instance id (23). As written, disabling any entry matching this condition will always signal multinet-down for instance 23, which is likely incorrect for other instances/interfaces. The id should be derived from configuration (e.g., instance/bridge index) rather than a constant.
v_secure_system("sysevent set multinet-down 23");
source/TR-181/middle_layer_src/vlan_apis.c:625
- This uses a hard-coded multinet instance id (23). If multiple untagged/bridge VLAN entries exist, they will all signal multinet-up for instance 23. The id should be derived from the relevant entry/configuration rather than a constant.
v_secure_system("sysevent set multinet-up 23");
source/TR-181/middle_layer_src/vlan_apis.c:705
- This log message is error-level, contains personal/debug text, and mentions VLAN ID 0 even though this branch is reached for VLANId == -1. Please make the message accurate (or remove it).
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
source/TR-181/middle_layer_src/vlan_apis.c:700
- This function now sets pEntry->Status = VLAN_IF_ERROR when interface creation fails, but later in Vlan_Enable() the status is unconditionally set to VLAN_IF_UP (outside this diff hunk). That will mask these failures from callers/telemetry.
pEntry->Status = VLAN_IF_ERROR;
CcspTraceError(("[%s][%d]Failed to create VLAN Tagged interface \n", __FUNCTION__, __LINE__));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
source/TR-181/middle_layer_src/vlan_apis.c:522
- Avoid calling system() with a constructed command string here; it executes via the shell and can be influenced by interface names coming from config/PSM. Use the project’s secure wrapper (v_secure_system) with a fixed format string instead.
snprintf(syscmd, sizeof(syscmd),"ovs-vsctl set bridge %s other-config:hwaddr=%s",pEntry->Name,macStr);
//v_secure_system("ovs-vsctl set bridge %s other-config:hwaddr=%s",pEntry->Name, macStr);
system(syscmd);
}
source/TR-181/middle_layer_src/vlan_apis.c:514
- This is a normal informational trace (computed MAC + interface name) and shouldn’t be logged as an error.
CcspTraceError(("%s-%d: macStr:%s,pEntry->Name:%s\n", __FUNCTION__, __LINE__, macStr, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:684
- In the non-HAL build, the extra braces after the #endif make the "Successfully deleted" log run unconditionally (even though delete commands might fail). Restructure the #if/#else so the success log is clearly tied to the delete operation.
#if defined(VLAN_MANAGER_HAL_ENABLED)
returnStatus = vlan_eth_hal_deleteInterface(pEntry->Name, pEntry->InstanceNumber);
if (ANSC_STATUS_SUCCESS != returnStatus)
{
CcspTraceError(("%s - Failed to delete the existing VLAN interface %s\n", __FUNCTION__, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:413
- The comment says the code deletes an OVS bridge and removes the interface, but the implementation only triggers a sysevent (multinet-down). Update the comment to match the actual behavior (or add the missing delete actions if that’s intended).
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-down 23");
}
source/TR-181/middle_layer_src/vlan_apis.c:632
- The comment says this branch "creates an OVS bridge and adds the interface", but the code only triggers a sysevent and sets the link up. Update the comment to reflect the actual behavior to avoid confusion during maintenance.
/* If the VLANID = -1, create an OVS bridge and add the interface */
if(strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-up 23");
v_secure_system("ip link set %s up", pEntry->Name);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/vlan_apis.c:413
- Hard-coding sysevent instance "23" makes this behavior device/config specific and risks collisions if multiple VLAN/bridge instances exist. Consider deriving the instance from configuration/PSM or using a named constant tied to the target interface.
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-down 23");
}
source/TR-181/middle_layer_src/vlan_apis.c:632
- Hard-coding sysevent instance "23" here makes the untagged/bridge bring-up path device-specific. Prefer deriving the instance from config/PSM or introducing a named constant documenting why 23 is correct.
/* If the VLANID = -1, create an OVS bridge and add the interface */
if(strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-up 23");
v_secure_system("ip link set %s up", pEntry->Name);
source/TR-181/middle_layer_src/vlan_internal.c:392
- Comment says "get cfg macvlan" but this code reads from
PSM_VLANMANAGER_MACVLAN(non-cfg namespace). Update the comment to match the actual key being read to avoid confusion during maintenance.
/* get cfg macvlan from psm */
snprintf( acPSMQuery, sizeof( acPSMQuery ), PSM_VLANMANAGER_MACVLAN, nIndex + 1 );
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
| v_secure_system("ip link set dev %s address %s",pEntry->Name, macStr); | ||
| snprintf(syscmd, sizeof(syscmd),"ovs-vsctl set bridge %s other-config:hwaddr=%s",pEntry->Name,macStr); | ||
| //v_secure_system("ovs-vsctl set bridge %s other-config:hwaddr=%s",pEntry->Name, macStr); | ||
| v_secure_system(syscmd); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
source/TR-181/middle_layer_src/vlan_apis.c:63
BUFLEN_1024is defined inside#ifdef FEATURE_MAPT, but it’s used byVlan_SetMacAddr()regardless of whetherFEATURE_MAPTis enabled. This will break compilation whenFEATURE_MAPTis not defined. DefineBUFLEN_1024unconditionally (or avoid the macro entirely).
#ifdef FEATURE_MAPT
#include "sysevent/sysevent.h"
#define IS_EMPTY_STRING(s) ((s == NULL) || (*s == '\0'))
#define SYSEVENT_WAN_IFACE_NAME "wan_ifname"
#define BUFLEN_64 64
#define PARAM_SIZE_32 32
#define PARAM_SIZE_64 64
#define BUFLEN_1024 1024
extern int sysevent_fd;
extern token_t sysevent_token;
#endif
source/TR-181/middle_layer_src/vlan_apis.c:527
- Running
ovs-vsctlviapopen()(with an interpolated command string) executes through a shell, increases command-injection risk, and returns-1from a function that should returnANSC_STATUS. Prefer the existingv_secure_system()wrapper and avoid parsing command output here.
FILE *fp = popen(syscmd, "r");
if (fp == NULL) {
perror("popen");
return -1;
}
source/TR-181/middle_layer_src/vlan_apis.c:731
- The log message says "VLAN ID 0" but this branch is entered when
VLANId <= 0(and currently appears intended for theVLANId == -1case). Update the message so it reflects the actual condition being handled.
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
source/TR-181/include/vlan_apis.h:92
- Struct members in
DML_VLANconsistently use PascalCase (e.g.,VLANId,TPId,BaseInterface), but the newly addedmacvlanuses lowercase. This inconsistency makes the struct harder to scan and is easy to propagate across the codebase.
UINT macvlan;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/vlan_apis.c:527
- Using
popen()to run anovs-vsctlcommand is unnecessary here and introduces shell-command injection risk viapEntry->Name, plus it relies onWEXITSTATUS()without including<sys/wait.h>. Prefer the existingv_secure_system()wrapper (already used throughout this file) to execute the command without the extra parsing/logging code.
FILE *fp = popen(syscmd, "r");
if (fp == NULL) {
perror("popen");
return -1;
}
source/TR-181/middle_layer_src/vlan_apis.c:514
- This is not an error path; logging the computed MAC as
CcspTraceErrorwill inflate error logs. UseCcspTraceInfo(as before) unless the operation fails.
CcspTraceError(("%s-%d: macStr:%s,pEntry->Name:%s\n", __FUNCTION__, __LINE__, macStr, pEntry->Name));
source/TR-181/middle_layer_src/vlan_internal.c:390
- This code path reads from
PSM_VLANMANAGER_MACVLAN(non-cfg) so the comment "get cfg macvlan" is misleading. Update the comment to match the actual PSM key being read.
/* get cfg macvlan from psm */
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
source/TR-181/include/vlan_apis.h:93
macvlanfield name is inconsistent with the struct’s existing PascalCase members (e.g.,TPId,VLANId,BaseInterface). This makes the API harder to scan and increases the chance of future typos; consider renaming it toMacVlan(and updating all references + the TR-181 param mapping accordingly).
CHAR LowerLayers[1024];
CHAR BaseInterface[64];
INT VLANId;
UINT TPId;
UINT macvlan;
CHAR Path[1024];
source/TR-181/middle_layer_src/vlan_apis.c:476
BUFLEN_1024is defined under#ifdef FEATURE_MAPT, butBUFLEN_1024is used inVlan_SetMacAddr()which is not guarded byFEATURE_MAPT. This will break builds whereFEATURE_MAPTis not enabled.
unsigned long long int number, new_mac;
char acTmpReturnValue[256] = {0};
char hex[32];
char macStr[32];
char syscmd[BUFLEN_1024] = {'\0'};
int i, j = 0;
source/TR-181/middle_layer_src/vlan_apis.c:735
- This message is misleading: the code enters this branch when
VLANId == -1(untagged/OVS-bridge case), but the log says "VLAN ID 0". This makes troubleshooting difficult.
else
{
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
returnStatus = Vlan_CreateUnTaggedInterface(pEntry);
source/TR-181/middle_layer_src/vlan_apis.c:776
Vlan_Enable()setspEntry->Status = VLAN_IF_ERRORon failures above, but then unconditionally overwrites it withVLAN_IF_UPat the end of the function. This can report a false "Up" status after a create/delete failure.
returnStatus = Vlan_CreateTaggedInterface(pEntry);
if (ANSC_STATUS_SUCCESS != returnStatus)
{
pEntry->Status = VLAN_IF_ERROR;
CcspTraceError(("[%s][%d]Failed to create VLAN Tagged interface \n", __FUNCTION__, __LINE__));
}
#if !defined(VLAN_MANAGER_HAL_ENABLED)
}
else
{
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
returnStatus = Vlan_CreateUnTaggedInterface(pEntry);
if (ANSC_STATUS_SUCCESS != returnStatus)
{
pEntry->Status = VLAN_IF_ERROR;
CcspTraceError(("[%s][%d]Failed to create VLAN UnTagged interface \n", __FUNCTION__, __LINE__));
}
}
#endif
//Get status of VLAN link
while(iIterator < 10)
{
if (ANSC_STATUS_FAILURE == Vlan_GetTaggedVlanInterfaceStatus(pEntry->Name, &status))
{
CcspTraceError(("%s-%d: Failed to get Tagged Vlan Interface=%s Status \n", __FUNCTION__, __LINE__, pEntry->Name));
}
if (VLAN_IF_UP == status)
{
EthLink_SendVirtualIfaceVlanStatus(pEntry->Path, "Up");
CcspTraceInfo(("%s-%d: Successfully Updated Vlan Status to WanManager for Interface(%s) \n", __FUNCTION__, __LINE__, pEntry->Name));
break;
}
iIterator++;
sleep(2);
CcspTraceInfo(("%s-%d: Interface Status(%d), retry-count=%d \n", __FUNCTION__, __LINE__, status, iIterator));
}
long uptime = 0;
get_uptime(&uptime);
pEntry->LastChange = uptime;
}
else
{
CcspTraceError(("%s-%d: PALAKSHA INSIDE FULL ELSE CASE calling Vlan_SetEthLink\n", __FUNCTION__, __LINE__));
//Enable EthLink and it will take care Creation of UnTagged Vlan Interface.
if (Vlan_SetEthLink(pEntry, TRUE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
}
pEntry->Status = VLAN_IF_UP;
source/TR-181/middle_layer_src/vlan_apis.c:545
Vlan_SetMacAddr()usespopen()with a command string built from interface/bridge name. This is a command-injection risk ifpEntry->Nameis not strictly validated, and the current error handling is inconsistent (usesperror/printf, returns-1from anANSC_STATUSfunction, and treatspclose()status incorrectly). Prefer using existingv_secure_system(...)patterns with input validation, and returnANSC_STATUS_FAILUREon failure.
if (pEntry->VLANId == -1 && pEntry->macvlan == 0) {
//CcspTraceError(("%s-%d: PALAKSHA PRINTING MAC SET COMMAND ovs-vsctl set bridge %s other-config:hwaddr=%s", __FUNCTION__, __LINE__, pEntry->Name, macStr));
snprintf(syscmd, sizeof(syscmd),
"/usr/bin/ovs-vsctl set bridge %s other-config:hwaddr=%s 2>&1",
pEntry->Name, macStr);
CcspTraceError(("%s-%d: PALAKSHA PRINTING COMMAND %s\n",__FUNCTION__,__LINE__,syscmd));
//v_secure_system("ovs-vsctl set bridge %s other-config:hwaddr=%s",pEntry->Name, macStr);
//v_secure_system(syscmd);
FILE *fp = popen(syscmd, "r");
if (fp == NULL) {
perror("popen");
return -1;
}
char buffer[256] ={'\0'};
while (fgets(buffer, sizeof(buffer), fp) != NULL) {
printf("%s", buffer);
}
CcspTraceError(("\nPRINTING BUFFER HERE %s\n", buffer));
int status = pclose(fp);
if (status == 0) {
CcspTraceError(("Command executed successfully.\n"));
} else {
CcspTraceError(("Command failed. Exit status: %d\n", WEXITSTATUS(status)));
}
source/TR-181/middle_layer_src/vlan_apis.c:414
- Hard-coding
23for themultinet-up/downevent introduces a magic number with no explanation, making future maintenance risky (it’s unclear what 23 represents and whether it’s platform-specific). Please define a named constant (or derive it from the VLAN instance/config) and use that instead.
#if !defined(VLAN_MANAGER_HAL_ENABLED)
else if(pEntry->VLANId == -1 && pEntry->macvlan == 0)
{
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-down 23");
}
}
Comment on lines
+526
to
+530
| FILE *fp = popen(syscmd, "r"); | ||
| if (fp == NULL) { | ||
| perror("popen"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
source/TR-181/middle_layer_src/vlan_apis.c:63
BUFLEN_1024is defined only under#ifdef FEATURE_MAPT, but it’s used unconditionally (e.g., forsyscmdinVlan_SetMacAddr). This will fail to compile whenFEATURE_MAPTis not enabled. DefineBUFLEN_1024outside the feature guard (or avoid the macro).
#ifdef FEATURE_MAPT
#include "sysevent/sysevent.h"
#define IS_EMPTY_STRING(s) ((s == NULL) || (*s == '\0'))
#define SYSEVENT_WAN_IFACE_NAME "wan_ifname"
#define BUFLEN_64 64
#define PARAM_SIZE_32 32
#define PARAM_SIZE_64 64
#define BUFLEN_1024 1024
extern int sysevent_fd;
extern token_t sysevent_token;
#endif
source/TR-181/middle_layer_src/vlan_apis.c:530
Vlan_SetMacAddrusespopen()/printf()/perror()and returns-1from a function that returnsANSC_STATUS. This also introduces a dependency onWEXITSTATUS/<sys/wait.h>. Prefer usingv_secure_system()and returnANSC_STATUS_FAILUREon errors.
FILE *fp = popen(syscmd, "r");
if (fp == NULL) {
perror("popen");
return -1;
}
source/TR-181/middle_layer_src/vlan_internal.c:392
- The comment says "get cfg macvlan from psm" but this branch reads from
PSM_VLANMANAGER_MACVLAN(non-cfg path). Updating the comment avoids confusion when maintaining the PSM migration logic.
/* get cfg macvlan from psm */
snprintf( acPSMQuery, sizeof( acPSMQuery ), PSM_VLANMANAGER_MACVLAN, nIndex + 1 );
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
source/TR-181/middle_layer_src/vlan_apis.c:773
pEntry->Statusis unconditionally set toVLAN_IF_UPafter the conditional branches, which can mask failures (e.g., ifVlan_SetEthLinkfails or interface creation above setVLAN_IF_ERROR). Also, the newly added log here is debug-only; it shouldn’t be at error level or include a developer name.
else
{
CcspTraceError(("%s-%d: PALAKSHA INSIDE FULL ELSE CASE calling Vlan_SetEthLink\n", __FUNCTION__, __LINE__));
//Enable EthLink and it will take care Creation of UnTagged Vlan Interface.
if (Vlan_SetEthLink(pEntry, TRUE, FALSE) == ANSC_STATUS_FAILURE)
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
source/TR-181/middle_layer_src/vlan_apis.c:59
- BUFLEN_1024 is defined only under FEATURE_MAPT, but it’s used unconditionally (e.g., Vlan_SetMacAddr). This will fail to compile when FEATURE_MAPT is not enabled. Also, this file now uses WIFEXITED/WEXITSTATUS and should include <sys/wait.h> in the non-conditional include set.
#define BUFLEN_1024 1024
source/TR-181/middle_layer_src/vlan_apis.c:530
- Vlan_SetMacAddr executes a shell command via popen(), writes to stdout with printf(), and returns -1 from a function that returns ANSC_STATUS. This is inconsistent with the rest of the component’s error handling and can also mis-handle command exit status (WEXITSTATUS without checking WIFEXITED). Prefer v_secure_system() (or an exec-style API) and return ANSC_STATUS_FAILURE on failure.
FILE *fp = popen(syscmd, "r");
if (fp == NULL) {
perror("popen");
return -1;
}
source/TR-181/middle_layer_src/vlan_apis.c:747
- This log message is inaccurate and looks like leftover debug output (it says VLAN ID 0, but this branch is for VLANId == -1). Please remove the personal/debug prefix and log a correct, actionable message at an appropriate severity.
CcspTraceError(("[%s][%d]PALAKSHA CREATING UNTAGGED VLAN WITH VLAN ID 0\n", __FUNCTION__, __LINE__));
source/TR-181/middle_layer_src/vlan_apis.c:655
- Hard-coding the multinet instance as "23" makes this behavior environment-specific and difficult to maintain (and risks tearing down the wrong multinet instance). This should be derived from configuration or from the interface/instance being managed rather than a literal constant.
v_secure_system("sysevent set multinet-up 23");
v_secure_system("ip link set %s up", pEntry->Name);
Comment on lines
+658
to
+668
| while(1) | ||
| { | ||
| int status = v_secure_system("/usr/bin/ovs-vsctl br-exists %s", pEntry->Name); | ||
| if (WIFEXITED(status) && (WEXITSTATUS(status) == 0)) | ||
| { | ||
| CcspTraceInfo(("Bridge erouter0 is present\n")); | ||
| break; | ||
| } | ||
| CcspTraceInfo(("Waiting for bridge erouter0...\n")); | ||
| sleep(1); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/vlan_apis.c:700
- The
#if/#elseblock here leaves a standalone{ ... }in the non-HAL build, so the "Successfully deleted" log prints unconditionally (even if the delete commands fail). It also makes the control flow harder to follow. Consider restructuring the preprocessor branches so the success log is emitted in the intended branch only.
#if defined(VLAN_MANAGER_HAL_ENABLED)
returnStatus = vlan_eth_hal_deleteInterface(pEntry->Name, pEntry->InstanceNumber);
if (ANSC_STATUS_SUCCESS != returnStatus)
{
CcspTraceError(("%s - Failed to delete the existing VLAN interface %s\n", __FUNCTION__, pEntry->Name));
}
else
#else
v_secure_system("ip link set %s down", pEntry->Name);
v_secure_system("ip link delete %s",pEntry->Name);
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
}
source/TR-181/middle_layer_src/vlan_apis.c:512
- This log prints the computed MAC and interface name on the normal success path, but it is logged as an error. That can generate false alarms in production logs and monitoring.
CcspTraceError(("%s-%d: macStr:%s,pEntry->Name:%s\n", __FUNCTION__, __LINE__, macStr, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:633
- The wait loop can time out (10 iterations) but the function still proceeds as if the bridge exists. That makes the subsequent MAC update and the overall enable flow unreliable; a timeout should fail the operation.
while(iIterator < 10)
{
int status = v_secure_system("/usr/bin/ovs-vsctl br-exists %s", pEntry->Name);
if (WIFEXITED(status) && (WEXITSTATUS(status) == 0))
{
| #if defined(VLAN_MANAGER_HAL_ENABLED) | ||
| if(pEntry->VLANId > 0 ) { | ||
| #else | ||
| if(pEntry->VLANId > 0 || (pEntry->VLANId == -1 && pEntry->macvlan == 0)) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
source/TR-181/middle_layer_src/vlan_apis.c:675
pEntry->macvlandoes not exist inDML_VLAN(the field added in this PR isMacVlanEnable). This will fail to compile and also prevents the new MACVLAN behavior gating from working as intended.
#if defined(VLAN_MANAGER_HAL_ENABLED)
if(pEntry->VLANId > 0 ) {
#else
if(pEntry->VLANId > 0 || (pEntry->VLANId == -1 && pEntry->macvlan == 0)) {
#endif
source/TR-181/middle_layer_src/vlan_apis.c:367
- This branch is disabling EthLink (
Vlan_SetEthLink(..., FALSE, ...)) but the error message says "Failed to Enable EthLink", which is misleading during troubleshooting.
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
source/TR-181/middle_layer_src/vlan_apis.c:367
- Vlan_Disable() now only disables EthLink when MacVlanEnable != 0, which skips disabling EthLink for the default case (MacVlanEnable == 0) and for tagged VLANs. This can leave the underlying EthLink enabled after disabling a VLAN. Also, the error log says "Enable" while disabling.
pthread_mutex_lock(&vlan_access_mutex);
//Set EthLink to False. it will take care UnTagged Created Vlan Interface
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
source/TR-181/middle_layer_src/vlan_apis.c:637
- Vlan_CreateUnTaggedInterface() uses WIFEXITED/WEXITSTATUS but vlan_apis.c does not include <sys/wait.h>, which can break the build depending on include order. If you only need a success/failure check from v_secure_system(), comparing the return value to 0 avoids the dependency.
int status = v_secure_system("/usr/bin/ovs-vsctl br-exists %s", pEntry->Name);
if (WIFEXITED(status) && (WEXITSTATUS(status) == 0))
{
CcspTraceInfo(("Bridge %s is present\n", pEntry->Name));
break;
}
source/TR-181/middle_layer_src/vlan_apis.c:685
- Vlan_Enable() logs an error when Vlan_GetTaggedVlanInterfaceStatus() fails but then immediately uses
statusin the next condition; if the call fails,statusmay be uninitialized. Set a safe value in the error path before using it.
if (Vlan_GetTaggedVlanInterfaceStatus(pEntry->Name, &status) != ANSC_STATUS_SUCCESS)
{
CcspTraceError(("[%s][%d]Failed to get vlan interface status \n", __FUNCTION__, __LINE__));
}
if ( ( status != VLAN_IF_NOTPRESENT ) && ( status != VLAN_IF_ERROR ) )
source/TR-181/middle_layer_src/vlan_apis.c:513
- Vlan_SetMacAddr() logs the computed MAC address as an error, even though this is normal informational output. Using error severity here can create noisy logs and false alarms.
CcspTraceError(("%s-%d: macStr:%s,pEntry->Name:%s\n", __FUNCTION__, __LINE__, macStr, pEntry->Name));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/vlan_apis.c:634
- In Vlan_CreateUnTaggedInterface(), the bridge-wait loop doesn’t fail the operation if the OVS bridge never appears; after 10 retries the function still proceeds and returns success. This can lead to later operations running against a non-existent bridge and the caller believing the interface was created.
while(iIterator < 10)
{
int status = v_secure_system("/usr/bin/ovs-vsctl br-exists %s", pEntry->Name);
if (status == 0)
{
source/TR-181/middle_layer_src/vlan_apis.c:700
- In the non-HAL path, the code logs "Successfully deleted" unconditionally after running the ip link commands, even if either command fails. This can mask failures and make troubleshooting difficult.
v_secure_system("ip link set %s down", pEntry->Name);
v_secure_system("ip link delete %s",pEntry->Name);
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:367
- This block is in Vlan_Disable(), but the log message says "Failed to Enable EthLink" and the comment implies EthLink is always disabled. After the change, EthLink is only disabled when MACVLAN is enabled, so the comment and error text should be updated to match the behavior.
//Set EthLink to False. it will take care UnTagged Created Vlan Interface
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/vlan_apis.c:367
- Vlan_Disable now only calls Vlan_SetEthLink() when MacVlanEnable != 0, but Vlan_Enable still enables EthLink for VLANId > 0 regardless of MacVlanEnable. This can leave the underlying EthLink enabled after disabling a non-macvlan VLAN. Also the error log says "Enable" while disabling.
//Set EthLink to False. it will take care UnTagged Created Vlan Interface
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
source/TR-181/middle_layer_src/vlan_apis.c:700
- In the non-HAL build, the "Successfully deleted" log is printed unconditionally after running
ip link set/down+ip link delete, even if those commands fail. Wrapping the log in a success check avoids misleading logs and makes failures visible.
v_secure_system("ip link set %s down", pEntry->Name);
v_secure_system("ip link delete %s",pEntry->Name);
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:633
- Vlan_CreateUnTaggedInterface waits up to 10 seconds for the OVS bridge to appear, but if it never does, the function still proceeds and returns success. This can cause subsequent operations (MAC set, status polling) to behave unpredictably. Treat the timeout as a failure.
while(iIterator < 10)
{
int status = v_secure_system("/usr/bin/ovs-vsctl br-exists %s", pEntry->Name);
if (status == 0)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
source/TR-181/middle_layer_src/vlan_apis.c:367
- Vlan_Disable now disables the underlying EthLink only when MacVlanEnable != 0. Since MacVlanEnable defaults to 0, this skips disabling EthLink for most tagged VLANs and can leave the Ethernet.Link state inconsistent with the VLAN teardown. Also, the error message says "Enable" even though this is the disable path.
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
source/TR-181/middle_layer_src/vlan_apis.c:413
- In the VLAN disable path, the telemetry marker used in the "Successfully deleted" log is VLAN_MARKER_VLAN_IF_CREATE, which misclassifies delete events. This should use VLAN_MARKER_VLAN_IF_DELETE to keep telemetry consistent with the enable path.
/* This will call ovs-vsctl for deleting erouter0 bridge, Instance 23 is for erouter0 bridge */
v_secure_system("sysevent set multinet-down 23");
}
source/TR-181/middle_layer_src/vlan_apis.c:711
- This log is always emitted in the non-HAL build regardless of whether the deletion commands succeeded, but it says "Successfully deleted". Either verify the command result before logging success or downgrade the message for the non-HAL path to avoid misleading operational logs.
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
}
source/TR-181/middle_layer_src/vlan_apis.c:627
- The multinet instance id is hard-coded as "23" (used for both multinet-up and multinet-down). This makes the OVS bridge handling fragile across platforms/configurations; consider introducing a named constant (or deriving it from configuration) and reusing it across all call sites in this file.
if(strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("sysevent set multinet-up 23");
v_secure_system("ip link set %s up", pEntry->Name);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
source/TR-181/middle_layer_src/vlan_apis.c:364
Vlan_Disable()now disables EthLink only whenMacVlanEnable != 0. HoweverVlan_Enable()callsVlan_SetEthLink()for tagged VLANs (and for the VLANId == -1 bridge case), so gating disable onMacVlanEnablewill leave EthLink enabled for the common case (MacVlanEnable == 0), causing the EthLink state to get out of sync with VLAN disable.
if (pEntry->MacVlanEnable != 0 && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
source/TR-181/middle_layer_src/vlan_apis.c:727
- In non-HAL builds, the "Successfully deleted VLAN interface" log block is currently a standalone block after the
#endif, so it executes unconditionally whenever the interface exists (even if the bridge/interface is still present after the delete attempt). Guard this success log (or restructure the block) so it only prints on verified success.
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
source/TR-181/middle_layer_src/vlan_internal.c:395
- MacVlanEnable is a macVlan_status_t enum, but the value loaded from PSM is assigned as a UINT. Cast to macVlan_status_t to match the field type.
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10) ;
}
source/TR-181/middle_layer_src/vlan_internal.c:289
- MacVlanEnable is a macVlan_status_t enum, but the value loaded from PSM is assigned as a UINT. This can trigger type warnings and makes it easier to accidentally store out-of-range values; cast to macVlan_status_t to match the field type.
This issue also appears on line 392 of the same file.
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10);
}
source/TR-181/middle_layer_src/vlan_dml.c:639
- MACVLAN is stored as an enum (macVlan_status_t), but the setter accepts any uint32 value without validation. Reject values outside the supported range (0..MACVLAN_PUBLIC) and cast to the enum type to prevent invalid states.
if (strcmp(ParamName, "MACVLAN") == 0)
{
p_Vlan->MacVlanEnable = uValue;
return TRUE;
}
source/TR-181/middle_layer_src/vlan_apis.c:367
- This log message reports "Failed to Enable EthLink" while the code is disabling EthLink (enable=FALSE). This makes troubleshooting confusing; update the message to reflect the actual operation.
//Set EthLink to False. it will take care UnTagged Created Vlan Interface
if (pEntry->MacVlanEnable != MACVLAN_DISABLED && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
{
CcspTraceError(("%s-%d: Failed to Enable EthLink\n", __FUNCTION__, __LINE__));
}
source/TR-181/middle_layer_src/vlan_apis.c:419
- The comment says "OVS bridge", but this code uses the Linux bridge utilities (brctl/ifconfig). Update the comment to match what the code actually does.
else if(pEntry->VLANId == -1 && pEntry->MacVlanEnable == MACVLAN_DISABLED)
{
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
if (strcmp(pEntry->BaseInterface, pEntry->Name))
{
if (pEntry->BaseInterface[0] != '\0')
{
v_secure_system("brctl delif %s %s", pEntry->Name, pEntry->BaseInterface);
}
v_secure_system("ifconfig %s down", pEntry->Name);
v_secure_system("brctl delbr %s", pEntry->Name);
source/TR-181/middle_layer_src/vlan_apis.c:626
- The comment says "OVS bridge", but this function uses brctl/ifconfig (Linux bridge). Update the comment so it matches the implementation.
/* If the VLANID = -1, create an OVS bridge and add the interface */
if(strcmp(pEntry->BaseInterface, pEntry->Name))
{
v_secure_system("ip link show %s > /dev/null 2>&1 || brctl addbr %s", pEntry->Name, pEntry->Name);
v_secure_system("brctl addif %s %s 2>/dev/null", pEntry->Name, pEntry->BaseInterface);
v_secure_system("ifconfig %s up", pEntry->Name);
source/TR-181/middle_layer_src/vlan_apis.c:673
- The current #if/#else structure makes control flow hard to follow: when VLAN_MANAGER_HAL_ENABLED is not defined, the success log executes unconditionally (and the "else" branch disappears), which can report successful deletion even if the commands fail. Restructure the conditional to keep the same semantics but make the success path explicit in both builds.
#if defined(VLAN_MANAGER_HAL_ENABLED)
returnStatus = vlan_eth_hal_deleteInterface(pEntry->Name, pEntry->InstanceNumber);
if (ANSC_STATUS_SUCCESS != returnStatus)
{
CcspTraceError(("%s - Failed to delete the existing VLAN interface %s\n", __FUNCTION__, pEntry->Name));
source/TR-181/middle_layer_src/vlan_apis.c:613
- Unused local variable iIterator in Vlan_CreateUnTaggedInterface; it is never read and can trigger warnings. Remove it.
ANSC_STATUS returnStatus = ANSC_STATUS_SUCCESS;
INT iIterator = 0;
Comment on lines
+79
to
+85
| typedef enum macVlan_status | ||
| { | ||
| MACVLAN_DISABLED = 0; | ||
| MACVLAN_PRIVATE = 1; | ||
| MACVLAN_PUBLIC = 2; | ||
| }macVlan_status_t; | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
source/TR-181/middle_layer_src/vlan_apis.c:364
- The new condition skips disabling EthLink for the common case where MACVLAN is disabled (default). That changes behavior for tagged VLAN disable and can leave Ethernet.Link.PriorityTagging enabled. Consider disabling EthLink for tagged VLANs and only skipping it for the bridge-based untagged case (VLANId == -1 && MACVLAN_DISABLED).
if (pEntry->MacVlanEnable != MACVLAN_DISABLED && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
source/TR-181/middle_layer_src/vlan_apis.c:408
- This comment says "OVS bridge" but the code uses brctl/ifconfig to manage a Linux bridge. Updating the comment will avoid confusion during ops/debugging.
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
source/TR-181/middle_layer_src/vlan_apis.c:621
- This comment says "OVS bridge" but the function uses brctl/ifconfig to create a Linux bridge. Please update the comment so it matches the implementation.
/* If the VLANID = -1, create an OVS bridge and add the interface */
source/TR-181/middle_layer_src/vlan_dml.c:639
MACVLANis modeled as an enum (MACVLAN_DISABLED/PRIVATE/PUBLIC) but the setter accepts any uint32. This can store invalid values and later drive unexpected behavior. Validate the range and cast to the enum type.
if (strcmp(ParamName, "MACVLAN") == 0)
{
p_Vlan->MacVlanEnable = uValue;
return TRUE;
}
source/TR-181/middle_layer_src/vlan_internal.c:395
- PSM values are parsed directly into
MacVlanEnablewithout validating the allowed enum range (0-2). If PSM contains an unexpected value, the system can enter an undefined MACVLAN mode. Clamp/validate and fall back to MACVLAN_DISABLED on invalid input.
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10) ;
}
source/TR-181/middle_layer_src/vlan_apis.c:613
iIteratoris declared but never used. If the build enables -Werror for unused variables, this will fail compilation; it also adds noise to the function.
This issue also appears on line 621 of the same file.
INT iIterator = 0;
source/TR-181/middle_layer_src/vlan_internal.c:289
- PSM values are parsed directly into
MacVlanEnablewithout validating the allowed enum range (0-2). If PSM contains an unexpected value, the system can enter an undefined MACVLAN mode. Clamp/validate and fall back to MACVLAN_DISABLED on invalid input.
This issue also appears on line 392 of the same file.
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10);
}
Signed-off-by: Palaksha Gowda <palaksha.gowda@sky.uk>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
source/TR-181/middle_layer_src/vlan_apis.c:364
Vlan_Disable()no longer disables the underlying EthLink whenMacVlanEnable == MACVLAN_DISABLED(the default). This changes previous behavior and leaves EthLink enabled for normal VLANs/bridge mode, which can prevent proper teardown and keep PriorityTagging/Enable set.
if (pEntry->MacVlanEnable != MACVLAN_DISABLED && Vlan_SetEthLink(pEntry, FALSE, FALSE) == ANSC_STATUS_FAILURE)
source/TR-181/middle_layer_src/vlan_apis.c:683
- The preprocessor/brace structure makes the "Successfully deleted" log run unconditionally (it is in a standalone block after
#endif) and theelseonly applies to the HAL build. This is likely unintended and also misleads logs when deletion fails.
}
#endif
{
CcspTraceInfo(("%s - %s:Successfully deleted VLAN interface %s\n", __FUNCTION__, VLAN_MARKER_VLAN_IF_DELETE, pEntry->Name));
}
source/TR-181/middle_layer_src/vlan_apis.c:408
- This comment says "OVS bridge" but the implementation uses
brctl(Linux bridge). The mismatch is confusing when debugging untagged interface behavior.
/* If the VLANID = -1, delete the OVS bridge and remove the interface */
source/TR-181/middle_layer_src/vlan_apis.c:620
- This comment says "OVS bridge" but the implementation uses
brctl(Linux bridge). Please align the comment with what the code actually does.
/* If the VLANID = -1, create an OVS bridge and add the interface */
source/TR-181/middle_layer_src/vlan_dml.c:639
MACVLANis a writable TR-181 parameter but the setter accepts anyuValueand stores it into the enum field. This allows invalid values to persist (e.g., 99) and can lead to undefined behavior in MACVLAN mode handling.
if (strcmp(ParamName, "MACVLAN") == 0)
{
p_Vlan->MacVlanEnable = uValue;
return TRUE;
}
source/TR-181/middle_layer_src/vlan_internal.c:395
- PSM
macvlanis parsed as an integer and assigned directly. If PSM is corrupted/out of range,MacVlanEnablecan become an invalid enum value. Consider bounding it to the supported range (0..MACVLAN_PUBLIC) and defaulting toMACVLAN_DISABLEDon invalid input.
snprintf( acPSMQuery, sizeof( acPSMQuery ), PSM_VLANMANAGER_MACVLAN, nIndex + 1 );
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10) ;
}
source/TR-181/middle_layer_src/vlan_apis.c:657
- For the new
VLANId == -1 && MacVlanEnable == MACVLAN_DISABLEDcase, the code now enters the "tagged" branch and callsVlan_SetEthLink(..., PriTag=TRUE), which conflicts with the previous untagged path (usedPriTag=FALSE) and risks enabling PriorityTagging for an untagged/bridge interface.
if(pEntry->VLANId > 0 || (pEntry->VLANId == -1 && pEntry->MacVlanEnable == MACVLAN_DISABLED)) {
if (Vlan_SetEthLink(pEntry, TRUE, TRUE) == ANSC_STATUS_FAILURE)
{
source/TR-181/middle_layer_src/vlan_internal.c:289
- PSM
macvlanis parsed as an integer and assigned directly. If PSM is corrupted/out of range,MacVlanEnablecan become an invalid enum value. Consider bounding it to the supported range (0..MACVLAN_PUBLIC) and defaulting toMACVLAN_DISABLEDon invalid input.
This issue also appears on line 391 of the same file.
if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
{
pVlan[nIndex].MacVlanEnable = (UINT)strtoul(acPSMValue, NULL, 10);
}
| #endif | ||
|
|
||
| #if !defined(VLAN_MANAGER_HAL_ENABLED) | ||
| static ANSC_STATUS Vlan_CreateUnTaggedInterface(PDML_VLAN pEntry) |
Contributor
There was a problem hiding this comment.
Do we need separate API? Instead can we make the logic based on valid VLANID?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.