vrf: add fixes and sync changes with bonding.c/bridge.c#79
Open
Linaro1985 wants to merge 6 commits into
Open
Conversation
Linaro1985
force-pushed
the
vrf_fixes
branch
2 times, most recently
from
3320822 to
01708db
Compare
Linaro1985
changed the title
Contributor
Author
|
ping @systemcrash @howels |
systemcrash
approved these changes
May 6, 2026
systemcrash
left a comment
systemcrash
left a comment
Contributor
There was a problem hiding this comment.
These look like clean additions/changes.
Linaro1985
changed the title
Linaro1985
force-pushed
the
vrf_fixes
branch
3 times, most recently
from
39c9a06 to
384b068
Compare
Contributor
Author
|
@robimarko, @nbd168, are there any additional changes needed for this to be accepted? |
Linaro1985
force-pushed
the
vrf_fixes
branch
3 times, most recently
from
1812eeb to
804f312
Compare
Commits taken as a basis: - device: fix build error on 32 bit systems - debug: remove newline from debug messages - bridge: skip present toggle in bridge_free_member() when device is active - bridge: remove kernel member on teardown regardless of device claim state - bridge: attempt delbr unconditionally on bridge destroy Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
Unlike bridge members, VRF enslaved interfaces remain full L3 devices. Disabling IPv6 on them was copied from bridge.c but is semantically wrong for VRF: link-local addresses on VRF ports are needed for IPv6 routing protocols (BGP, OSPFv3) to function within the VRF domain. The Linux kernel VRF documentation confirms that enslaved interfaces participate in IPv4 and IPv6 stacks normally, with their routes automatically moved to the VRF-associated routing table. Fixes: openwrt/openwrt#23829 Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
vrf_empty is set unconditionally to true in vrf_apply_settings() and has no corresponding UCI attribute, so it can never be false. Its only observable effect is to always trigger force_active=true and device_set_present() in vrf_config_init(), and to make vrf_remove_member() return early so that force_active is never cleared and device_set_present(false) is never called when all ports are removed. This reflects the intended semantics: a VRF without ports is a valid and useful configuration. Replace the dead conditional with direct unconditional assignments, and drop the now-unreachable force_active/device_set_present block from vrf_remove_member() Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
Rename the static device_type descriptor from vrf_state_type to vrf_device_type for consistency with bridge_device_type and bonding_device_type. The _state suffix is misleading as it implies a connection to struct vrf_state, while this is a static type descriptor unrelated to per-instance state. Also remove a spurious blank line in vrf_member_update(). Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
Add the standard GPLv2 license header consistent with other netifd modules. Co-authored-by Felix Fietkau is noted as the bridge and bonding device management patterns were derived from his bridge.c and bonding.c. Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
The previous implementation using goto had three bugs: 1. The debug message was printed unconditionally, before checking ret, so "Failed to add" was logged even on success. 2. After a successful system_vrf_if() call (ret == 0), the code still jumped back to retry instead of exiting, causing up to 3 unnecessary redundant kernel calls. 3. The off-by-one in the tries guard (tries <= 3 after post-increment) allowed 4 iterations instead of 3. Rewrite using a for loop with break on success, mirroring the structure of system_bridge_addif(). Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
What was the actual testing ask for this? I have a basic 2x1Gb bond working with LACP (on recent snapshots) but I did observe that I had to restart /etc/init.d/network TWICE in order to get the bond to take a new config. First restart would go from a LACP bond to basic round-robin type, then second restart of the network would re-enable the bond with LACP again. Guessing this is due to a need to reload kernel modules for new LACP parameters? |
Contributor
Author
|
@howels this PR contains only fixes for VRF functionality. I'm tagging you since you previously reviewed this functionality in #38 (comment) and might be interested in these fixes. I'm not sure this is related to LACP in any way, though. |
Ah, ok. I saw the bonding title and misunderstood the use case. |
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
3 participants
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.
VRF support was originally based on bonding.c/bridge.c. Since being added to netifd, fixes have been made that did not make it into vrf.c. Therefore, let's bring in the missing changes.
Commits taken as a basis:
Also this PR contains next fixes:
Fixes: openwrt/openwrt#23829