Backport networking fixes to 7.1#2214
Conversation
|
""" WalkthroughThis set of changes unifies and refines network interface metric handling across scripts and UI components, introducing dynamic metric calculation based on interface indices. It adds a new background script for monitoring static IP assignments, updates Docker and wireless scripts for improved interface management, and enhances address/route assignment logic. Scripts and UI now consistently use system-derived interface indices for metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebGUI
participant rc.inet1
participant rc.docker
participant monitor_interface
User->>WebGUI: Configure static IP / metric
WebGUI->>rc.inet1: Apply config (metric = 1000 + ifindex)
rc.inet1->>System: Assign IP with calculated metric
rc.inet1->>sbin/create_network_ini: Update statics.ini
sbin/create_network_ini->>monitor_interface: statics.ini updated
monitor_interface->>System: Monitor interface operstate
monitor_interface->>System: Add/remove static IPs as interfaces go up/down
rc.docker->>System: Use active() and index() for interface/metric logic
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
sbin/monitor_interface (1)
1-70: Well-designed interface monitoring script with room for minor improvements.The script provides a solid foundation for dynamic static IP management with appropriate debouncing and change detection. However, consider these enhancements for production robustness:
- Add error handling for IP commands - The
ip addr addandip addr flushcommands should include error checking- Validate file operations - Add existence checks before reading the statics file in the main loop
- Add logging - Consider adding syslog entries for interface state changes and IP operations
Example improvements:
+# Add logging function +log() { + logger -t monitor_interface "$*" +} init(){ PORT=(); STATE=(); if [[ -r $FILE ]]; then + log "Loading interface configuration from $FILE" # initialize values from file, maintained by 'create_network_ini' while IFS=$'\n' read -r ROW; do PORT+=("$ROW") STATE+=($(state ${ROW%% *})) done <$FILE fi MD5=$(md5) } if [[ $NEW == up ]]; then - ip addr add dev ${PORT[$i]} + if ip addr add dev ${PORT[$i]} 2>/dev/null; then + log "Added static IP to ${INT}: ${PORT[$i]}" + else + log "Failed to add static IP to ${INT}: ${PORT[$i]}" + fi elif [[ $NEW == down && $INT != $LAST ]]; then - ip addr flush scope global dev $INT + if ip addr flush scope global dev $INT 2>/dev/null; then + log "Flushed addresses from ${INT}" + else + log "Failed to flush addresses from ${INT}" + fi fisbin/create_network_ini (1)
252-268: Well-implemented Docker interface address management.The code correctly handles different interface types and DHCP events, with proper metric calculation and runtime checks.
Consider adding error handling for the
ip addrcommands:- [[ -S /var/run/docker.sock ]] && ip addr add $IPV4 metric $INDEX dev $LINK + [[ -S /var/run/docker.sock ]] && ip addr add $IPV4 metric $INDEX dev $LINK 2>/dev/null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
emhttp/plugins/dynamix/Eth0.page(8 hunks)emhttp/plugins/dynamix/EthX.page(6 hunks)emhttp/plugins/dynamix/include/RoutingTable.php(1 hunks)emhttp/plugins/dynamix/scripts/reload_services(0 hunks)emhttp/plugins/dynamix/scripts/update_services(1 hunks)etc/rc.d/rc.docker(12 hunks)etc/rc.d/rc.inet1(28 hunks)etc/rc.d/rc.library.source(3 hunks)etc/rc.d/rc.local(2 hunks)etc/rc.d/rc.nginx(3 hunks)etc/rc.d/rc.wireless(6 hunks)sbin/create_network_ini(10 hunks)sbin/monitor_interface(1 hunks)sbin/monitor_nchan(2 hunks)
💤 Files with no reviewable changes (1)
- emhttp/plugins/dynamix/scripts/reload_services
🔇 Additional comments (43)
emhttp/plugins/dynamix/scripts/update_services (1)
18-19: LGTM: Logic change aligns with service reload refactoring.The modification to embed the sleep delay in the shell command rather than passing it as an argument correctly supports the refactoring described in the AI summary, where
reload_servicesno longer handles initial delays internally.emhttp/plugins/dynamix/include/RoutingTable.php (1)
29-30: LGTM: Improved route filtering logicThe addition of "linkdown" and "broadcast" filtering enhances the route display by excluding non-operational and broadcast routes, making the routing table more relevant and cleaner for users.
etc/rc.d/rc.library.source (3)
8-8: Minor: Updated timestampThe timestamp update to May 2025 reflects recent modifications to the script.
59-60: LGTM: Improved IP address filteringChanging from
-temporarytoprimaryscope filtering provides more consistent and reliable IP address selection by focusing on primary addresses rather than temporary/privacy addresses.
163-163: LGTM: Consistent primary address filteringThis change aligns with the modification in the
show()function, ensuring consistent primary address filtering throughout the script.sbin/monitor_nchan (2)
10-12: LGTM: Clean subscriber count extractionThe new
nchan_subs()function properly handles the curl request with timeout and error handling for reliable subscriber count retrieval.
67-77: LGTM: Improved idle detection logicThe refactored main loop using
nchan_idle()provides more reliable subscriber monitoring with proper spacing between checks and cleaner hourly logging.etc/rc.d/rc.local (3)
83-86: LGTM: Proper interface monitoring integrationThe new interface state monitoring is properly integrated with executable checks and background execution, aligning with the networking improvements theme of this PR.
90-90: LGTM: Consistent background executionModifying the nchan monitoring to run in background with output redirection maintains consistency with the new interface monitoring approach.
192-192: LGTM: Clean whitespaceMinor cleanup removing trailing spaces improves code consistency.
etc/rc.d/rc.nginx (4)
9-9: Documentation update looks good.The header comment date update from October 2023 to May 2025 properly reflects the modification timeframe.
540-541: Improved IP address fetching with scope filtering.The addition of
scope global primaryto theipcommand enhances the accuracy of IP address retrieval by filtering out link-local and other non-global addresses and selecting primary addresses. This aligns well with the networking improvements across the codebase.
544-545: Consistent wireless interface IP fetching improvements.The same
scope global primaryimprovements are appropriately applied to the wireless interface fallback logic, maintaining consistency with the main interface handling.
719-719: Clean command formatting.Removing the trailing space from the monitor_nchan command improves code cleanliness.
emhttp/plugins/dynamix/EthX.page (1)
189-189: Simplified metric function calls align with refactored logic.The removal of IP version and index parameters from all
metric()function calls is consistent across base interfaces, VLAN interfaces, and templates. This simplification aligns with the metric function refactoring that now calculates metrics based solely on interface names using system-derived interface indices.Also applies to: 216-216, 296-296, 323-323, 386-386, 407-407
emhttp/plugins/dynamix/Eth0.page (3)
58-65: Excellent metric function refactoring with improved logic.The simplified
metric()function is well-designed with several improvements:
- Interface-based calculation: Uses only interface name, making it more intuitive
- Active port detection: Properly handles bridge/bond scenarios by checking for corresponding interfaces
- System integration: Reads actual interface index from
/sys/class/net/$port/ifindex- Fallback logic: Gracefully handles non-existent interfaces with
+ 1logic- Consistent metrics:
1000 + indexformula ensures predictable metric assignmentThis refactoring aligns perfectly with the broader networking improvements across the codebase.
477-479: Improved DOM traversal in removeVLAN function.The change from ID-based selector to
closest('form')traversal makes the function more robust and less dependent on specific DOM structure, which is a good practice for maintainable JavaScript.
730-730: Consistent metric function call simplification.All
metric()function calls have been properly updated to use the new single-parameter signature, maintaining consistency across base interfaces, VLAN configurations, and templates. The changes align with the refactored metric calculation logic.Also applies to: 753-753, 831-831, 860-860, 922-922, 943-943
etc/rc.d/rc.wireless (4)
33-42: LGTM! Well-implemented helper functions.The
var()andindex()functions are cleanly implemented with proper error handling.
144-150: Good implementation of Docker shim interface handling.The code correctly checks both the configuration and Docker runtime state before managing the shim interface address. The metric calculation ensures proper routing priority.
168-179: Clean refactoring of ipaddr_down function.The function now elegantly handles both IPv4 and IPv6 through a single parameter, improving code maintainability.
274-281: Appropriate service restart logic for static DNS configurations.The code correctly tracks DNS configuration state and conditionally restarts services only when static assignments are used, avoiding unnecessary service disruptions.
Also applies to: 289-290, 334-335, 348-349
etc/rc.d/rc.inet1 (4)
131-132: Improved loopback interface address checks.The refactored code uses a more concise and efficient approach with
ip -br addrandawk.
153-155: Consistent implementation of index() function.The function matches the implementation in rc.wireless, maintaining consistency across scripts.
290-290: Appropriate IPv6 handling for macvtap interfaces.Disabling IPv6 on macvtap interfaces is a sensible default for VM networking scenarios.
Also applies to: 296-296
634-634: Proper route command syntax updates.The addition of the 'to' keyword improves command clarity and follows current
ipcommand best practices.Also applies to: 646-646, 660-660, 670-670, 711-711, 717-717, 729-729, 735-735
sbin/create_network_ini (3)
27-30: Consistent helper function implementations.The
var()andindex()functions match the implementations in other scripts, maintaining good consistency.Also applies to: 32-35
73-73: Well-designed static IP assignments tracking.The code properly tracks static IP assignments in a separate file and uses atomic moves to ensure consistency. The IP filtering with "scope global primary" is correct.
Also applies to: 163-166, 185-188, 215-217, 238-241, 249-250
151-151: Correct IP address filtering improvements.The consistent use of "scope global primary" filtering ensures only relevant addresses are processed, and the IPv6 "-deprecated" flag properly excludes outdated addresses.
Also applies to: 164-164, 171-171, 186-186, 203-203, 216-216, 224-224, 239-239, 255-255, 281-282, 288-289
etc/rc.d/rc.docker (14)
13-13: LGTM!The header comment update correctly reflects the modification date.
38-53: Well-designed helper functions for interface management.The
index()andactive()functions provide clean abstractions for:
- Retrieving interface indices from sysfs
- Resolving active interface names (eth → br/bond conversion)
Good use of error suppression and efficient pattern substitution.
68-68: Consistent use of the newactive()helper function.The interface name resolution is now standardized across the script.
Also applies to: 74-74
115-115: Improved route query specificity.The change to check for default IPv6 routes specifically is more precise than the previous generic route query.
177-178: Appropriate driver selection for wireless interfaces.The forced ipvlan selection for wlan interfaces ensures compatibility, as wireless interfaces may not support macvlan.
186-189: Clean implementation for IPv4 address existence check.The
ipv4_exist()function provides a concise way to check for existing IPv4 addresses on interfaces.
232-234: Consistent IP and route query improvements.The addition of
scope global primaryand the use oftosyntax aligns with the unified approach across the codebase.
265-265: Improved Docker command clarity.Using the full
container lscommand is more explicit and follows Docker's recommended command structure.
319-330: Simplified and consistent network management.The changes improve network handling by:
- Using
active()for consistent interface resolution- Streamlining network removal conditions
- Making network recreation unconditional for reliability
Also applies to: 342-343
347-363: Comprehensive IP and route query improvements.The changes provide:
- Consistent use of
scope global primaryfor IPv4- Proper route query syntax with
to- Improved IPv6 address filtering (excluding /128 networks)
- Better handling of deprecated addresses
394-402: Improved network recreation logic.The unconditional network removal and recreation ensures a clean state, preventing issues from stale configurations. The VHOST logic properly differentiates between interface types.
424-459: Excellent refactoring of shim/vhost interface management.The changes provide:
- Dynamic metric calculation based on interface indices (preventing conflicts)
- Clear separation between br, wlan, and other interface types
- Proper IPv6 disabling on shim/vhost interfaces
- Conditional IPv4 address management based on access settings
- Use of
ipv4_exist()to prevent duplicate addressesThe metric calculation strategy (base 1000/3000 + interface index) ensures unique metrics.
474-481: Proper cleanup in network stop function.The changes ensure:
- Docker configuration is read before stopping networks
- Consistent use of
active()function- Special cleanup for wlan0 shim interfaces
Also applies to: 489-498
515-515: Process management change noted.The removal of
disownallows the parent shell to maintain job control over the background process. Ensure this aligns with the intended process management strategy.
| INDEX=$(index $PORT) | ||
| INDEX=$((3000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))})) |
There was a problem hiding this comment.
Fix potential glob expansion issue in metric calculation.
The glob pattern * in $(index * | sort -n | tail -1) could expand to all files in the current directory rather than network interfaces, potentially causing incorrect metric calculation.
Use the proper path to enumerate network interfaces:
INDEX=$(index $PORT)
-INDEX=$((3000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))}))
+INDEX=$((3000 + ${INDEX:-$(($(for i in /sys/class/net/*; do index "${i##*/}"; done | sort -n | tail -1) + 1))}))🤖 Prompt for AI Agents
In etc/rc.d/rc.wireless at lines 141-142, the glob pattern '*' used in the
metric calculation can expand to all files in the current directory instead of
network interfaces, causing incorrect results. Replace the '*' with the correct
path or pattern that specifically lists network interface names, such as using
'/sys/class/net/*' or another appropriate directory, to ensure only network
interfaces are enumerated for the index calculation.
| INDEX=$(index $IFACE) | ||
| INDEX=$((1000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))})) |
There was a problem hiding this comment.
Fix potential glob expansion issue in metric calculation.
Similar to rc.wireless, the glob pattern could cause issues.
Apply the same fix as suggested for rc.wireless:
INDEX=$(index $IFACE)
-INDEX=$((1000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))}))
+INDEX=$((1000 + ${INDEX:-$(($(for i in /sys/class/net/*; do index "${i##*/}"; done | sort -n | tail -1) + 1))}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INDEX=$(index $IFACE) | |
| INDEX=$((1000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))})) | |
| INDEX=$(index $IFACE) | |
| INDEX=$((1000 + ${INDEX:-$(($(for i in /sys/class/net/*; do index "${i##*/}"; done | sort -n | tail -1) + 1))})) |
🤖 Prompt for AI Agents
In etc/rc.d/rc.inet1 at lines 397-398, the metric calculation uses a glob
pattern that may expand unexpectedly, causing errors. To fix this, modify the
glob pattern to prevent unintended expansion by quoting or escaping it,
following the same approach used in rc.wireless. This ensures the pattern is
treated literally during the index calculation.
| # convert netmask to prefix | ||
| mask2cidr(){ | ||
| [[ -z $1 ]] && return | ||
| local MASK=$(eval eval echo "'\$((('{"${1//./,}"}'>>'{7..0}')%2))'") | ||
| eval echo '$(('"${MASK// /+}"'))' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the mask2cidr implementation.
The current implementation uses complex nested eval statements that are difficult to understand and maintain.
Consider a clearer implementation:
-mask2cidr(){
- [[ -z $1 ]] && return
- local MASK=$(eval eval echo "'\$((('{"${1//./,}"}'>>'{7..0}')%2))'")
- eval echo '$(('"${MASK// /+}"'))'
-}
+mask2cidr(){
+ [[ -z $1 ]] && return
+ local bits=0
+ local IFS='.'
+ for octet in $1; do
+ case $octet in
+ 255) bits=$((bits + 8));;
+ 254) bits=$((bits + 7)); break;;
+ 252) bits=$((bits + 6)); break;;
+ 248) bits=$((bits + 5)); break;;
+ 240) bits=$((bits + 4)); break;;
+ 224) bits=$((bits + 3)); break;;
+ 192) bits=$((bits + 2)); break;;
+ 128) bits=$((bits + 1)); break;;
+ 0) break;;
+ esac
+ done
+ echo $bits
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # convert netmask to prefix | |
| mask2cidr(){ | |
| [[ -z $1 ]] && return | |
| local MASK=$(eval eval echo "'\$((('{"${1//./,}"}'>>'{7..0}')%2))'") | |
| eval echo '$(('"${MASK// /+}"'))' | |
| } | |
| # convert netmask to prefix | |
| mask2cidr(){ | |
| [[ -z $1 ]] && return | |
| local bits=0 | |
| local IFS='.' | |
| for octet in $1; do | |
| case $octet in | |
| 255) bits=$((bits + 8));; | |
| 254) bits=$((bits + 7)); break;; | |
| 252) bits=$((bits + 6)); break;; | |
| 248) bits=$((bits + 5)); break;; | |
| 240) bits=$((bits + 4)); break;; | |
| 224) bits=$((bits + 3)); break;; | |
| 192) bits=$((bits + 2)); break;; | |
| 128) bits=$((bits + 1)); break;; | |
| 0) break;; | |
| esac | |
| done | |
| echo $bits | |
| } |
🤖 Prompt for AI Agents
In sbin/create_network_ini around lines 37 to 42, the mask2cidr function uses
overly complex nested eval commands that reduce readability and maintainability.
Simplify the function by replacing the nested evals with a straightforward
method to convert the netmask to CIDR notation, such as splitting the mask into
octets, converting each to binary, counting the number of set bits, and summing
them to get the prefix length.
|
You can ignore the codeRabbit comments and merge this PR (same as is done for 7.2). |
|
Thanks @bergware I agree these fixes should be made in 7.2 first and then backported. Please consider making the fixes in 7.2, the fixes for update_services and monitor_nchan seems most important to me |
|
Made updates for update_services and monitor_nchan as per codeRabbit recommendations. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores