Wireless support - fine tuning#2002
Conversation
WalkthroughThis update refines network interface management across multiple components. The Docker management files now include logic for wireless interfaces alongside wired ones, with new helper functions to conditionally render UI elements. The network name generation has been improved and method signatures standardized. In addition, the VM form now allows all network options without disabling any, and the system startup scripts incorporate wireless support by updating interface detection, route addition, and virtual link management. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as DockerSettings.page
participant Helper as Helper Functions (hide_wlan/hide_eth)
participant DockerUtil as DockerClient.php
User ->> UI: Request Docker settings page
UI ->> UI: Gather network interfaces (bond, eth, wlan)
UI ->> Helper: Check interface type (wlan vs. others)
Helper -->> UI: Return visibility status
UI ->> DockerUtil: Validate network port (using mgmt_port & lan_port)
DockerUtil -->> UI: Provide port configuration
UI ->> User: Render updated network UI
sequenceDiagram
participant System as Startup Script (rc.docker)
participant Wireless as rc.wireless
System ->> System: Detect available interfaces (eth and wlan)
System ->> Wireless: Check and create virtual wireless link (LINK)
Wireless ->> Wireless: Bring up primary interface and virtual LINK
Wireless ->> System: Execute standardized wireless startup commands
System ->> Wireless: Update container routes for wlan0
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (3)
etc/rc.d/rc.wireless (1)
217-219: Use safer cleanup approach for wireless stop.The current implementation uses
pkillwhich might be too aggressive. Consider usingkillallwith specific process name orpidofto ensure only the intended process is terminated.- run pkill wpa_supplicant + if pid=$(pidof wpa_supplicant); then + run kill $pid + fietc/rc.d/rc.docker (1)
510-512: Enhance cleanup of wireless network interfaces.The cleanup of wireless network interfaces should be more thorough to prevent resource leaks.
- [[ -e $SYSTEM/$NETWORK ]] && run ip addr flush dev $NETWORK + if [[ -e $SYSTEM/$NETWORK ]]; then + run ip addr flush dev $NETWORK + run ip link set $NETWORK down + fiemhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
1099-1100: LGTM with a minor formatting suggestion.The wireless interface detection is correctly implemented and properly internationalized. However, there's a minor formatting inconsistency.
Consider standardizing the spacing after
--to match other conditions:- $name .= ' -- '._('Wireless interface'); + $name .= ' -- '._('Wireless interface');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/plugins/dynamix.docker.manager/DockerSettings.page(14 hunks)emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(3 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(0 hunks)etc/rc.d/rc.docker(11 hunks)etc/rc.d/rc.wireless(3 hunks)
💤 Files with no reviewable changes (1)
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
🔇 Additional comments (9)
etc/rc.d/rc.docker (2)
19-21: LGTM! Network interface types properly updated.The script now correctly includes wireless interfaces in the stock network types and network interface detection.
411-416: Verify virtual interface handling for wireless networks.The script creates a shim interface for wireless networks, but we should verify that the interface naming doesn't conflict with existing interfaces.
emhttp/plugins/dynamix.docker.manager/DockerSettings.page (3)
116-118: LGTM! Well-designed wireless interface visibility check.The
hide_wlanfunction properly checks for wireless interface visibility based on management port status.
120-123: LGTM! Ethernet interface visibility management.The
hide_ethfunction correctly handles the visibility of ethernet interfaces when wireless is the primary interface.
312-362: Verify UI state consistency.The UI visibility logic for network interfaces should maintain consistency across page reloads and Docker service restarts.
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (3)
43-43: LGTM! Clear management port definition.The management port array is well-defined and includes all necessary interface types.
1084-1121: LGTM! Improved method signature formatting.The method signatures are now consistently formatted and maintain proper return type declarations.
46-47: Verify port selection logic for edge cases.The port selection logic should be tested with various network configurations to ensure proper fallback behavior.
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
1095-1095: LGTM! Improved robustness in description handling.The change from direct boolean evaluation to
empty()check makes the code more robust by properly handling all falsy values.
| run iw dev $PORT connect "$SSID" auth open | ||
| elif [[ $SECURITY =~ "WEP" ]]; then | ||
| # WEP encryption | ||
| iw dev $PORT connect "$SSID" auth shared key 0:$(hex "$PASSWORD") 2>/dev/null | ||
| run iw dev $PORT connect "$SSID" auth shared key 0:$(hex "$PASSWORD") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for network connection.
The network connection commands lack error handling and timeout mechanisms. This could lead to indefinite waits if the network is unreachable.
- run iw dev $PORT connect "$SSID" auth open
+ if ! timeout 30 iw dev $PORT connect "$SSID" auth open; then
+ log "Failed to connect to open network $SSID"
+ return 1
+ fi📝 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.
| run iw dev $PORT connect "$SSID" auth open | |
| elif [[ $SECURITY =~ "WEP" ]]; then | |
| # WEP encryption | |
| iw dev $PORT connect "$SSID" auth shared key 0:$(hex "$PASSWORD") 2>/dev/null | |
| run iw dev $PORT connect "$SSID" auth shared key 0:$(hex "$PASSWORD") | |
| if ! timeout 30 iw dev $PORT connect "$SSID" auth open; then | |
| log "Failed to connect to open network $SSID" | |
| return 1 | |
| fi | |
| elif [[ $SECURITY =~ "WEP" ]]; then | |
| # WEP encryption | |
| run iw dev $PORT connect "$SSID" auth shared key 0:$(hex "$PASSWORD") |
| LINK=shim-$PORT | ||
| [[ -e $SYSTEM/$LINK ]] || run ip link add link $PORT name $LINK type ipvlan mode l2 bridge |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify virtual link creation for wireless interface.
The script creates a virtual link using ipvlan in bridge mode. This is a good approach for integrating wireless interfaces with Docker networking, but we should ensure the link is properly cleaned up when stopping the wireless interface.
🏁 Script executed:
#!/bin/bash
# Description: Check if shim interface is properly cleaned up on wireless stop
# Test: Search for any leftover shim interfaces
ip link show | grep -P '^shim-wlan'Length of output: 130
🏁 Script executed:
#!/bin/bash
# Script to search for any commands that might clean up the virtual "shim" interface
echo "=== Searching for commands that delete a shim interface ==="
grep -R "ip link delete" etc/rc.d/rc.wireless || echo "No 'ip link delete' commands found."
echo ""
echo "=== Searching for shutdown/stop sections that may include cleanup ==="
grep -R "stop)" etc/rc.d/rc.wireless || echo "No 'stop' cases found in the script."
echo ""
echo "=== Searching for any cleanup-related functions with 'cleanup' keyword ==="
grep -R "cleanup" etc/rc.d/rc.wireless || echo "No 'cleanup' functions or references found."Length of output: 869
Issue: Missing cleanup for virtual interface shim created with ipvlan.
Our review of the wireless management script (etc/rc.d/rc.wireless) confirms that while the code creates a virtual link with:
LINK=shim-$PORT
[[ -e $SYSTEM/$LINK ]] || run ip link add link $PORT name $LINK type ipvlan mode l2 bridge
no corresponding cleanup is implemented. Searches for commands like “ip link delete”, references to “stop)” in the script, or any “cleanup” functions yielded no results. This indicates that the virtual interface is not properly removed when the wireless interface is stopped.
- Ensure cleanup logic is added (for example, within the wireless stop routine) to delete the shim interface, e.g., using "ip link delete $LINK".
- Verify that this cleanup action is coordinated with other network interface management routines.
2 participants
Summary by CodeRabbit