More networking fixes#2241
Conversation
|
""" WalkthroughThe updates introduce persistent tracking of network interface assignments via a statics file and marker files, add new functions for managing these records, and refine logic for interface and gateway management. Several scripts are modified to ensure accurate address assignment, removal, and monitoring, particularly for shim and vhost interfaces. UI changes adjust gateway checkbox logic to depend on both metric and gateway presence. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant rc.docker
participant statics.ini
participant Interface
User->>rc.docker: Start/Stop Docker network
rc.docker->>Interface: Assign/remove IP address
rc.docker->>statics.ini: add_entry()/del_entry() (update watch list)
rc.docker->>Interface: Add/remove default route
rc.docker->>statics.ini: Update .down marker files as needed
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
etc/rc.d/rc.docker (1)
61-72: Well-designed persistent tracking mechanismThe
add_entryanddel_entryfunctions provide a clean interface for managing the watch list. The use of marker files (.down) effectively prevents race conditions with the monitoring script.Consider adding file locking to prevent concurrent access issues:
# add entry to watch list add_entry(){ rm -f /var/tmp/${1%% *}.down - [[ -e $STA ]] && echo "$1" >>$STA + [[ -e $STA ]] && { + exec 200>>$STA + flock 200 + echo "$1" >&200 + exec 200>&- + } } # delete enty from watch list del_entry(){ touch /var/tmp/$1.down - [[ -e $STA ]] && sed -i "/^$1 .*/d" $STA + [[ -e $STA ]] && { + exec 200>>$STA + flock 200 + sed -i "/^$1 .*/d" $STA + exec 200>&- + } sleep 0.5 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
etc/rc.d/rc.docker(5 hunks)etc/rc.d/rc.inet1(3 hunks)sbin/create_network_ini(4 hunks)sbin/monitor_interface(1 hunks)
🔇 Additional comments (6)
sbin/create_network_ini (1)
166-168: Good defensive programming practice!The changes correctly prevent empty gateway route lines from being written to the statics file by checking if the route command output is non-empty before appending. This pattern is consistently applied across all gateway route handling (IPv4/IPv6, main/VLAN interfaces).
Also applies to: 191-193, 224-226, 250-252
sbin/monitor_interface (3)
61-61: Good optimization using base interface nameUsing
${PORT%.*}strips VLAN suffixes, preventing redundant carrier state checks for multiple VLAN interfaces on the same physical interface.Also applies to: 98-98
67-71: Excellent addition of IP presence checks before route creationThe changes correctly verify that a global primary IP address exists on the interface before attempting to add default gateway routes. The IPv6 check appropriately excludes deprecated addresses with the
-deprecatedflag.Also applies to: 75-79
82-89:Details
❓ Verification inconclusive
Integration with persistent tracking mechanism
The special handling for
.downmarker files integrates with the new watch list mechanism. However, there's a potential race condition between checking the file existence and the IP operations.Consider adding error handling for the race condition:
- if [[ -f /var/tmp/$PORT.down ]]; then - # Special treatment for shim and vhost interfaces - [[ -n $IPA ]] && ip addr del dev ${TASK[$i]} - else - # IP address not present? create it - [[ -z $IPA ]] && ip addr add dev ${TASK[$i]} - fi + if [[ -f /var/tmp/$PORT.down ]]; then + # Special treatment for shim and vhost interfaces + [[ -n $IPA ]] && ip addr del dev ${TASK[$i]} 2>/dev/null + else + # IP address not present? create it + [[ -z $IPA ]] && ip addr add dev ${TASK[$i]} 2>/dev/null || true + fiVerify the timing between marker file creation and IP operations:
🏁 Script executed:
#!/bin/bash # Check for any existing .down files and their timestamps echo "=== Checking for .down marker files ===" ls -la /var/tmp/*.down 2>/dev/null || echo "No .down files found" # Check the statics.ini file for context echo -e "\n=== Current statics.ini entries ===" cat /var/local/emhttp/statics.ini 2>/dev/null | head -20 || echo "statics.ini not found"Length of output: 300
Confirm concurrency safety for
.downmarker checks
- File:
sbin/monitor_interfacelines 82–89- There’s a small window between testing
-f /var/tmp/$PORT.downand runningip addrwhere another process could create/remove the marker, leading to unexpected failures or script exits.Suggested mitigation (redirect errors and prevent
set -efrom exiting):- if [[ -f /var/tmp/$PORT.down ]]; then - [[ -n $IPA ]] && ip addr del dev ${TASK[$i]} - else - [[ -z $IPA ]] && ip addr add dev ${TASK[$i]} - fi + if [[ -f /var/tmp/$PORT.down ]]; then + [[ -n $IPA ]] && ip addr del dev ${TASK[$i]} 2>/dev/null || true + else + [[ -z $IPA ]] && ip addr add dev ${TASK[$i]} 2>/dev/null || true + fiOptionally, wrap the marker-file check + IP command in a lock (e.g.,
flock) to serialize access and fully eliminate the race.etc/rc.d/rc.docker (2)
427-448: Consistent IP assignment handling with metric 0The changes properly manage IP assignments for both shim and vhost interfaces, using metric 0 to ensure high priority. The conditional logic correctly handles the DOCKER_ALLOW_ACCESS setting.
Also applies to: 461-472
494-508: Proper cleanup sequence in network stopGood practice to retrieve the IPv4 address before network removal, ensuring accurate cleanup of IP assignments from interfaces.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/Eth0.page (1)
213-214: LGTM! Gateway checkbox logic is now consistent across all interfaces.The removal of the special case for eth0 index 0 and the direct reading of checkbox states improves consistency. The logic correctly resets metrics to '0' when gateway checkboxes are unchecked, which aligns with the broader network management improvements mentioned in the PR summary.
Consider adding existence checks for robustness:
- var gw4 = $(form).find('input[name="USE_GW4:'+i+'"]').prop('checked'); - if (!gw4) $(form).find('input[name="METRIC:'+i+'"]').val('0'); + var gw4Element = $(form).find('input[name="USE_GW4:'+i+'"]'); + var gw4 = gw4Element.length ? gw4Element.prop('checked') : false; + if (!gw4) $(form).find('input[name="METRIC:'+i+'"]').val('0');And similarly for the IPv6 section.
Also applies to: 223-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Eth0.page(1 hunks)sbin/monitor_interface(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sbin/monitor_interface
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Eth0.page(3 hunks)emhttp/plugins/dynamix/EthX.page(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/Eth0.page
🔇 Additional comments (2)
emhttp/plugins/dynamix/EthX.page (2)
285-285: LGTM! IPv4 VLAN gateway checkbox logic is correct.The variable references and logical conditions are properly implemented for VLAN interfaces. The checkbox will only be checked when both metric is non-zero and gateway is specified.
314-314: LGTM! IPv6 VLAN gateway checkbox logic is correct.The variable references and logical conditions are properly implemented for VLAN interfaces. The checkbox will only be checked when both metric is non-zero and gateway is specified.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sbin/monitor_interface(3 hunks)
🔇 Additional comments (4)
sbin/monitor_interface (4)
21-23: Good generalization of interface checkingReplacing the specific
wlanfunction with a genericportfunction improves code reusability and allows monitoring of any interface type, not just wireless interfaces.
62-62: Correct handling of VLAN interfacesThe change to use
${PORT%.*}properly handles VLAN interfaces by checking the carrier state of the base interface. This is correct since VLAN interfaces inherit their physical state from the parent interface.Also applies to: 89-89
65-76: Improved gateway route managementThe enhanced gateway handling for both IPv4 and IPv6 adds important safeguards:
- Only adds routes when interface is up
- Verifies primary IP address exists
- Prevents duplicate default routes
- IPv6 correctly filters deprecated addresses
These changes prevent routing issues when interfaces are in transitional states.
79-87: Clear state-based IP address managementThe explicit state handling improves clarity:
- Interface down: removes IP address if present
- Interface up: adds IP address if missing (unless
/var/tmp/$PORT.downexists)The
.downmarker file provides manual control to prevent IP assignment on specific interfaces.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes