RDKB-65961, RDKCOM-5619, RDKBNETWOR-99:Untagged VLAN, unified interface type framework

[S-Parthiban-Selvaraj]

Consolidates vlan-manager PRs #39 and #40 into a single implementation.

Changes:

• Add untagged_vlan_type_t enum with all kernel macvlan modes (private, vepa, bridge, passthru, source), brctl simple bridge, 802.1Q tag-0, and TAGGED_VLAN marker for conventional tagged VLANs
• Add UnTaggedVlanType field to DML_VLAN and DML_ETHERNET structs
• PSM key dmsb.vlanmanager.%d.untaggedvlantype for non-HUB4 platforms; HUB4 regional flow always uses UNTAGGED_SIMPLE_BRIDGE (brctl)
• Set UnTaggedVlanType = TAGGED_VLAN(7) at init when VLANId > 0, so the enum drives the Enable/Disable if/else instead of VLANId > 0
• EthLink_CreateUnTaggedInterface: switch on type for create/delete, covering bridge (brctl), all five macvlan modes, and vlan tag-0
• EthLink_DeleteUnTaggedInterface: symmetric type-aware teardown
• Vlan_SetEthLink: always sync type to EthLink entry before enable
• Extract Vlan_WaitForInterfaceUp() common helper shared by both tagged and untagged paths in Vlan_Enable
• Add status check loop for untagged path (mirrors tagged path)
• UntaggedVlanType DM param: read-only, uint32/mapped string
• Add .gitignore to suppress autoconf/libtool generated files

Merges: #39
#40

[Copilot]

Pull request overview

This PR consolidates prior work to support multiple “untagged VLAN” realisation mechanisms (bridge/macvlan/tag-0) under a unified UnTaggedVlanType framework, wires that type through VLAN↔EthLink flows, and exposes it as a read-only TR-181 parameter.

Changes:

• Introduces untagged_vlan_type_t and propagates UnTaggedVlanType through VLAN and Ethernet data structures, PSM loading, and DM getter support.
• Refactors VLAN enable/disable to use a shared interface-up polling helper and to sync the VLAN type into the EthLink entry before enabling.
• Implements type-aware create/delete logic for untagged interfaces (bridge/macvlan modes/tag-0) and adds a .gitignore for generated artifacts.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
source/TR-181/middle_layer_src/vlan_internal.c	Initializes/loads UnTaggedVlanType alongside existing VLAN config (including PSM-backed type for non-HUB4 flow).
source/TR-181/middle_layer_src/vlan_dml.c	Exposes UntaggedVlanType as a TR-181 uint32 getter.
source/TR-181/middle_layer_src/vlan_apis.c	Syncs VLAN type into EthLink, refactors enable/disable branching, and adds a shared interface-up polling helper.
source/TR-181/middle_layer_src/ethernet_apis.c	Adds type-driven create/delete for untagged interfaces (bridge/macvlan/tag-0).
source/TR-181/include/vlan_apis.h	Adds PSM key for untagged type and extends DML_VLAN with UnTaggedVlanType.
source/TR-181/include/ethernet_apis.h	Adds untagged_vlan_type_t enum and stores type in DML_ETHERNET.
config/RdkVlanManager.xml	Adds read-only UntaggedVlanType parameter to the data model.
.gitignore	Ignores autoconf/automake and dm_pack generated files.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (7)

source/TR-181/middle_layer_src/vlan_apis.c:649

• Vlan_Enable() selects tagged vs untagged behavior based on UnTaggedVlanType, but VLANID can be updated at runtime without updating UnTaggedVlanType (e.g., Vlan_SetParamIntValue only assigns p_Vlan->VLANId). This can misclassify a tagged VLAN (VLANId > 0) as untagged and call the wrong EthLink path. Keep the enum in sync with VLANId (or fall back to VLANId > 0) before branching.

    //Create Vlan Tagged or UnTagged Interface, selected by UnTaggedVlanType
    if (pEntry->UnTaggedVlanType == TAGGED_VLAN) {

source/TR-181/middle_layer_src/vlan_apis.c:378

• Vlan_Disable() deletes the tagged interface only when UnTaggedVlanType == TAGGED_VLAN. If VLANID was changed to a tagged VLAN at runtime but UnTaggedVlanType was not updated, the tagged interface may be left behind. Consider keying tagged teardown off VLANId > 0 (or syncing UnTaggedVlanType first).

    //Delete Created Tagged Vlan Interface
    if (pEntry->UnTaggedVlanType == TAGGED_VLAN)
    {

source/TR-181/middle_layer_src/vlan_internal.c:46

• Typo/grammar in this TODO update comment (e.g., “dicovery”, “boottime”, and stray punctuation) makes it hard to understand the intended migration plan.

 * Update: The unification is finalised, however to remove the below code, the respective platform have to use VLAN dicovery or boottime VLAN configuration based. on the region to replace this dynamic country based VLAN configuration. 

source/TR-181/middle_layer_src/vlan_internal.c:392

• Same as above: TAGGED_VLAN is represented as the literal 7 here. Using enum constants would avoid duplicating the mapping and improve readability/maintainability.

        /* Set UntaggedVlanType: tagged VLANs always use TAGGED_VLAN (7).
         * For untagged VLANs load the type from PSM; falls back to 0 (UNTAGGED_SIMPLE_BRIDGE). */
        if (pVlan[nIndex].VLANId > 0)
        {
            pVlan[nIndex].UnTaggedVlanType = 7; /* TAGGED_VLAN */
        }

source/TR-181/middle_layer_src/ethernet_apis.c:990

• In the MACVLAN creation path, the return code from the critical ip link add ... command is assigned to rc but then ignored. If interface creation fails, the function continues to run subsequent ip link set ... commands and still returns success, which can leave state inconsistent and make failures harder to detect upstream.

                rc = EXEC_CMD("ip link add link %s name %s address %s type macvlan mode %s",
                              pEntry->BaseInterface, pEntry->Name, pEntry->MACAddress, macvlanMode);
            }
            (void)rc;
            EXEC_CMD("ip link set %s allmulticast on", pEntry->Name);

source/TR-181/middle_layer_src/vlan_internal.c:286

• This assigns UnTaggedVlanType using raw numeric values (0/7). Since the enum values are now part of the public interface (untagged_vlan_type_t), hardcoding numbers risks divergence if the enum changes. Prefer using the enum constants (e.g., TAGGED_VLAN / UNTAGGED_SIMPLE_BRIDGE) and including the header that defines them.

This issue also appears on line 387 of the same file.

            /* Set UntaggedVlanType: tagged VLANs use TAGGED_VLAN (7); untagged VLANs
             * default to UNTAGGED_SIMPLE_BRIDGE (0) for the HUB4 regional flow. */
            pVlan[nIndex].UnTaggedVlanType = (pVlan[nIndex].VLANId > 0) ? 7 /* TAGGED_VLAN */ : 0 /* UNTAGGED_SIMPLE_BRIDGE */;

source/TR-181/include/ethernet_apis.h:143

• UnTaggedVlanType is a new field but DML_ETHERNET_INIT() does not initialize it. This becomes risky because EthLink initialization currently does not fully zero the allocated table (see ethernet_internal.c using memset(pEthCfg, 0, sizeof(DML_ETHERNET))), so this enum can be indeterminate for entries beyond the first. Initialize this field explicitly to UNTAGGED_SIMPLE_BRIDGE (0) in the struct init routine and/or fix the zeroing logic.

    LONG                 MACAddrOffSet;  // Changed to LONG to support negative offsets
    BOOLEAN              PriorityTagging;
    untagged_vlan_type_t UnTaggedVlanType; // Untagged VLAN realisation type (bridge/macvlan/tag0)
    UINT                 NumberofMarkingEntries;

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (6)

source/TR-181/middle_layer_src/vlan_internal.c:47

• The updated TODO comment has multiple spelling/grammar issues (e.g., “dicovery”, “boottime”, “platform have to”, “based. on”), which makes the guidance hard to read/maintain.

/*TODO
 *Need to be Reviewed after Unification is finalised.
 * Update: The unification is finalised, however to remove the below code, the respective platform have to use VLAN dicovery or boottime VLAN configuration based. on the region to replace this dynamic country based VLAN configuration. 
 */

source/TR-181/middle_layer_src/ethernet_apis.c:939

• Untagged interface creation/deletion now shells out with EXEC_CMD() using interface names sourced from configuration/PSM. Without validating pEntry->Name/BaseInterface to be a safe ifname, this is vulnerable to shell injection or command breakage if those strings contain whitespace/metacharacters.

    CcspTraceInfo(("%s-%d: Creating untagged interface %s on base interface %s (type=%d, MAC offset=%ld)\n",
                   __FUNCTION__, __LINE__, pEntry->Name, pEntry->BaseInterface,
                   pEntry->UnTaggedVlanType, pEntry->MACAddrOffSet));

config/RdkVlanManager.xml:210

• For mapped uint32 parameters in this XML, the file consistently uses type starting with string: (see Status). Using unsignedInt: here is inconsistent and may not be interpreted as a mapped type by tooling (dm_pack), potentially breaking the new parameter’s representation.

      <parameter>
       <name>UntaggedVlanType</name>
       <type> unsignedInt: Bridge(0),MacvlanPrivate(1),MacvlanVepa(2),MacvlanBridge(3),MacvlanPassthru(4),MacvlanSource(5),VlanTag0(6),Tagged(7) </type>
       <syntax>uint32/mapped</syntax>
       <writable>false</writable>

source/TR-181/middle_layer_src/vlan_internal.c:400

• UntaggedVlanType is loaded from PSM via atoi() without any range validation. Invalid values (negative / >7) can silently drive unexpected interface creation/deletion behavior when later cast to untagged_vlan_type_t.

            snprintf( acPSMQuery, sizeof( acPSMQuery ), PSM_VLANMANAGER_UNTAGGEDVLANTYPE, nIndex + 1 );
            if ( CCSP_SUCCESS == DmlVlanGetPSMRecordValue( acPSMQuery, acPSMValue ) )
            {
                 pVlan[nIndex].UnTaggedVlanType = atoi(acPSMValue) ;
            }
        }

source/TR-181/middle_layer_src/ethernet_apis.c:1004

• The enum includes UNTAGGED_VLAN_TAG_0 (6), but EthLink_CreateUnTaggedInterface() doesn’t handle it and will fall through to the bridge/default path. That’s inconsistent with the type definition (and with the function’s delete-path comment mentioning tag-0), and can create the wrong kind of interface if type=6 is ever selected.

        case UNTAGGED_SIMPLE_BRIDGE:
        default:
        {
            /* Default: Linux bridge via brctl. The base interface is enslaved
             * to a bridge that carries untagged traffic. */

source/TR-181/middle_layer_src/ethernet_apis.c:1058

• EthLink_DeleteUnTaggedInterface() doesn’t handle UNTAGGED_VLAN_TAG_0 (6). If a tag-0 interface is ever created via the untagged path, the current delete logic will treat it like a bridge (default case) and won’t delete the VLAN netdev.

        case UNTAGGED_MACVLAN_PRIVATE:
        case UNTAGGED_MACVLAN_VEPA:
        case UNTAGGED_MACVLAN_BRIDGE:
        case UNTAGGED_MACVLAN_PASSTHRU:
        case UNTAGGED_MACVLAN_SOURCE:
        {
            /* macvlan and vlan tag-0 devices are removed with ip link. */

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

source/TR-181/middle_layer_src/vlan_internal.c:46

• Spelling/grammar in this comment is unclear (e.g., “dicovery”, “boottime”, and awkward sentence structure). Consider rephrasing so it’s readable and unambiguous.

 * Update: The unification is finalised, however to remove the below code, the respective platform have to use VLAN dicovery or boottime VLAN configuration based. on the region to replace this dynamic country based VLAN configuration. 

source/TR-181/middle_layer_src/ethernet_apis.c:121

• EthLink_RunCmd() executes formatted strings via popen(), which invokes a shell. Several call sites pass user/PSM-provided strings (e.g., interface names) into %s without validation, so shell metacharacters could be interpreted. Prefer executing without a shell (fork/exec with argv) or add strict allow-list validation for interface tokens before building shell commands.

static int EthLink_RunCmd(const char *caller, int line, const char *fmt, ...)
{
    char    cmd[512]   = {0};
    char    redir[544] = {0};
    va_list args;
    FILE   *fp;
    char    buf[256]   = {0};
    int     status;

    va_start(args, fmt);
    vsnprintf(cmd, sizeof(cmd) - 1, fmt, args);
    va_end(args);

    CcspTraceInfo(("%s-%d: exec: %s\n", caller, line, cmd));

    snprintf(redir, sizeof(redir) - 1, "%s 2>&1", cmd);
    fp = popen(redir, "r");

source/TR-181/middle_layer_src/vlan_apis.c:588

• The VLAN netdev is created on pEntry->Alias (ip link add link ...), but the base interface for VLAN termination is stored in pEntry->BaseInterface (and is used in the HAL path). Using Alias here is inconsistent with the rest of the code and with the config docs (Alias is a label/ethlink name), and can attach the VLAN device to the wrong lower interface.

    v_secure_system("ip link add link %s name %s type vlan id %u", pEntry->Alias, pEntry->Name, pEntry->VLANId);

source/TR-181/middle_layer_src/vlan_internal.c:286

• Avoid hard-coding enum values (7/0) for UntaggedVlanType here; this risks drifting from the canonical untagged_vlan_type_t definitions and makes the intent harder to read.

            pVlan[nIndex].UnTaggedVlanType = (pVlan[nIndex].VLANId > 0) ? 7 /* TAGGED_VLAN */ : 0 /* UNTAGGED_SIMPLE_BRIDGE */;

source/TR-181/middle_layer_src/ethernet_apis.c:1054

• This comment mentions “vlan tag-0 devices”, but UNTAGGED_VLAN_TAG_0 (6) is not handled in this switch and is not an untagged interface type in this code path. Update the comment to match the actual behavior to avoid confusion.

            /* macvlan and vlan tag-0 devices are removed with ip link. */

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (9)

source/TR-181/middle_layer_src/vlan_internal.c:47

• Spelling/grammar issues in the updated TODO comment (e.g., “dicovery”, sentence structure) make it hard to understand and search for later; please correct the wording.

/*TODO
 *Need to be Reviewed after Unification is finalised.
 * Update: The unification is finalised, however to remove the below code, the respective platform have to use VLAN dicovery or boottime VLAN configuration based. on the region to replace this dynamic country based VLAN configuration. 
 */

source/TR-181/middle_layer_src/vlan_apis.c:588

• ip link add link should use the underlying base interface name, not the VLAN entry Alias (which is also used as a label/marking key). Using Alias here can break interface creation when Alias != kernel netdev name.

    v_secure_system("ip link add link %s name %s type vlan id %u", pEntry->Alias, pEntry->Name, pEntry->VLANId);

source/TR-181/middle_layer_src/ethernet_apis.c:940

• EXEC_CMD() uses popen() (i.e., a shell) and interpolates interface names directly into command strings. If Name/BaseInterface ever contain unexpected characters, this becomes a command-injection vector. Validate these strings before building/executing shell commands.

    CcspTraceInfo(("%s-%d: Creating untagged interface %s on base interface %s (type=%d, MAC offset=%d)\n",
                   __FUNCTION__, __LINE__, pEntry->Name, pEntry->BaseInterface,
                   pEntry->UnTaggedVlanType, pEntry->MACAddrOffSet));

    switch (pEntry->UnTaggedVlanType)

source/TR-181/middle_layer_src/ethernet_apis.c:1036

• Same command-injection concern as create path: EthLink_DeleteUnTaggedInterface passes Name/BaseInterface into shell commands. Validate the strings before executing to avoid interpreting unexpected characters.

    CcspTraceInfo(("%s-%d: Deleting untagged VLAN interface %s (type=%d)\n",
                   __FUNCTION__, __LINE__, pEntry->Name, pEntry->UnTaggedVlanType));

    switch (pEntry->UnTaggedVlanType)

source/TR-181/middle_layer_src/ethernet_apis.c:973

• If UnTaggedVlanType is accidentally set to UNTAGGED_VLAN_TAG_0 on the untagged path, the current default: will silently create a bridge instead. Make this case explicit and fail fast so misconfiguration doesn’t create the wrong netdev type.

        case UNTAGGED_SIMPLE_BRIDGE:

source/TR-181/middle_layer_src/ethernet_apis.c:1057

• EthLink_DeleteUnTaggedInterface doesn’t include UNTAGGED_VLAN_TAG_0 in the ip-link delete case list, so if it ever reaches this function it will fall into the bridge teardown path. Treat tag-0 like other ip-link deletions to avoid deleting the wrong type.

        {

source/TR-181/middle_layer_src/vlan_internal.c:391

• Hard-coded TAGGED_VLAN value (7) makes the type mapping harder to maintain. Prefer using TAGGED_VLAN from untagged_vlan_type_t (and include the defining header) to avoid accidental mismatches.

        /* Set UntaggedVlanType: tagged VLANs always use TAGGED_VLAN (7).
         * For untagged VLANs load the type from PSM; falls back to 0 (UNTAGGED_SIMPLE_BRIDGE). */
        if (pVlan[nIndex].VLANId > 0)
        {
            pVlan[nIndex].UnTaggedVlanType = 7; /* TAGGED_VLAN */

source/TR-181/middle_layer_src/vlan_internal.c:286

• Hard-coded VLAN type values (7/0) reduce readability and can drift from the enum definition. Prefer using the shared untagged_vlan_type_t constants (TAGGED_VLAN / UNTAGGED_SIMPLE_BRIDGE) so the mapping stays explicit.

This issue also appears on line 387 of the same file.

            /* Set UntaggedVlanType: tagged VLANs use TAGGED_VLAN (7); untagged VLANs
             * default to UNTAGGED_SIMPLE_BRIDGE (0) for the HUB4 regional flow. */
            pVlan[nIndex].UnTaggedVlanType = (pVlan[nIndex].VLANId > 0) ? 7 /* TAGGED_VLAN */ : 0 /* UNTAGGED_SIMPLE_BRIDGE */;

source/TR-181/middle_layer_src/ethernet_apis.c:121

• EthLink_RunCmd doesn’t detect vsnprintf/snprintf truncation. If a command is truncated it may execute an unintended (partial) command string; it’s safer to refuse execution when the formatted command doesn’t fit.

This issue also appears in the following locations of the same file:

• line 973
• line 1057

    va_start(args, fmt);
    vsnprintf(cmd, sizeof(cmd) - 1, fmt, args);
    va_end(args);

    CcspTraceInfo(("%s-%d: exec: %s\n", caller, line, cmd));