Miscellanous network fixes and improvements#2186
Conversation
## Walkthrough
This update introduces significant improvements to network interface management and metric assignment across Unraid OS scripts and plugins. Key changes include dynamic calculation of interface metrics based on sysfs indices, refined address and route handling for Docker and wireless interfaces, and the introduction of a new monitoring script for static IP assignments. Several helper functions were added, and command invocations were updated for precision and consistency.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|----------------|
| emhttp/plugins/dynamix/Eth0.page, emhttp/plugins/dynamix/EthX.page | Simplified the `metric()` function to use only interface name; updated all metric calls to match new signature and concatenation style for VLANs. Adjusted JavaScript event handling for VLAN removal. |
| emhttp/plugins/dynamix/include/RoutingTable.php | Extended route filtering in IP route queries to exclude `linkdown` and `broadcast` (IPv4) and `linkdown` (IPv6). |
| emhttp/plugins/dynamix/scripts/reload_services, emhttp/plugins/dynamix/scripts/update_services | Changed service reload scheduling to use explicit `sleep` in shell before execution, removing direct delay argument passing and immediate queue checking. |
| etc/rc.d/rc.docker | Added `index()` and `active()` helpers; refined Docker network interface setup and teardown with improved address and route handling, conditional IP assignment using interface indices, disabled IPv6 on shim interfaces, and enhanced output suppression. Removed complex route metric adjustments and network update checks. |
| etc/rc.d/rc.inet1 | Updated metadata; added `index()` helper; renamed and simplified carrier check; improved static IP metric assignment using interface indices; disabled IPv6 on macvtap interfaces. |
| etc/rc.d/rc.library.source | Changed `ip` command options from `-temporary` to `primary` for address filtering in interface address display and checks. |
| etc/rc.d/rc.local | Added silent background launch of `monitor_interface` if present; redirected output of `monitor_nchan` to `/dev/null`. |
| etc/rc.d/rc.nginx | Refined LAN IP address extraction in `build_ssl` to use `scope global primary` and reordered filtering commands for precision. Updated file header. |
| etc/rc.d/rc.wireless | Added `index()` and `var()` helpers; dynamic metric calculation for IP assignment; unified IP version handling with explicit parameters; added Docker shim interface IPv4 address management; improved cleanup and conditional service restarts based on DNS assignment. |
| sbin/create_network_ini | Added `statics.ini.new` for static IP storage; introduced helpers (`var`, `index`, `mask2cidr`); refined IP queries to filter primary addresses; added Docker interface IP management triggered by DHCP events; updated welcome message IP queries. |
| sbin/monitor_interface | New script to monitor interfaces and manage static IPs from `statics.ini`, dynamically adding/removing addresses based on link state changes with stability checks and file change detection. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant WebGUI
participant rc.inet1
participant rc.wireless
participant rc.docker
participant monitor_interface
participant statics.ini
User->>WebGUI: Configures network settings
WebGUI->>rc.inet1: Applies static IPs (uses index() for metric)
WebGUI->>rc.wireless: Sets wireless IPs (uses index() for metric)
rc.inet1->>statics.ini: Writes static IP assignments
rc.wireless->>statics.ini: Writes static IP assignments
Note over rc.docker: Docker interface setup uses index() for metrics
monitor_interface->>statics.ini: Reads static IPs and metrics
monitor_interface->>Network Interfaces: Adds/removes static IPs on link state changePossibly related PRs
Suggested reviewers
Poem
|
This reverts commit ea2ba28.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/EthX.page (1)
215-217: Same early-evaluation issue for IPv6 template — see previous comment.etc/rc.d/rc.docker (1)
535-538:⚠️ Potential issue
docker_network_stop: sameip -br … primaryissueThe cleanup pulls an IPv4 string using the unsupported selector:
NET=$(ip -4 -br addr show scope global primary dev shim-wlan0 …)If the command prints nothing, the subsequent
ip addr delis skipped—leaking addresses and leaving shim-wlan0 dirty. Please fix as in earlier comment.
🧹 Nitpick comments (8)
etc/rc.d/rc.nginx (1)
9-9: Update header year once, avoid churn
The header already carries a LimeTech + Bergware stamp; adding a new dated line every micro-change quickly clutters the prologue. Recommend replacing the date with a single “Last-modified:” line that is updated automatically by the build.emhttp/plugins/dynamix/EthX.page (1)
186-191: Server-side metric default works, but loses IPv4/IPv6 separation
metric('ethX')now yields a single value for both IPv4 and IPv6 default-gateway rows.
If users need independent tweaking (e.g. higher IPv6 metric to prefer IPv4) they’ve lost the knob.Provide separate defaults or expose one combined “metric” input only when both protocols share the gateway.
sbin/monitor_interface (1)
52-71: Multiple monitors possible – add PID/lockfile
rc.localstarts this script in the background but nothing prevents a second instance after reboot or manual invocation, leading to duplicateip addr addexecutions.Create
/var/run/monitor_interface.pidand exit when already running.etc/rc.d/rc.inet1 (1)
385-396: Carrier check good, but DHCP fallback ignoresDHCP_METRICremoval
When DHCP is chosen andDHCP_METRICis empty (now common after automatic metrics), the prior logic to drop-mis fine, yet you removed the explicit-Gguard for IPv6. Ensuredhcpcdstill honours a default metric when none supplied.emhttp/plugins/dynamix/Eth0.page (1)
476-480:removeVLAN– make sure the change event is fired on the correct select elementAfter removing a VLAN row you now resolve the form with
closest('form')– good – but you still triggerchangeon.first()any select. If the first select is disabled/hidden the UI might not refresh.Recommend narrowing the selector to the protocol field that drives visibility:
- form.find('select').first().trigger('change'); + form.find('select[name^="PROTOCOL:"]').first().trigger('change');This guarantees the re-validation logic (
selectProtocol) runs.sbin/create_network_ini (1)
37-49:mask2cidr/cidr2maskrely on nestedeval– fragile & unsafeBoth helpers use double-evaluated arithmetic strings; a malformed or malicious netmask/prefix could craft arbitrary arithmetic, and future shell-option hardening (
set -euo pipefail,nounset,errexit) will break on expansion errors.A more robust, readable implementation avoids
evalentirely:mask2cidr() { local IFS=. n=0 oct for oct in $1; do case $oct in 255) n=$((n+8));; 254) n=$((n+7));; 252) n=$((n+6));; 248) n=$((n+5));; 240) n=$((n+4));; 224) n=$((n+3));; 192) n=$((n+2));; 128) n=$((n+1));; 0) break;; *) return 1;; # invalid mask esac done echo "$n" } cidr2mask() { local n=$1 mask= for ((i=0;i<4;i++)); do if ((n>=8)); then mask+=255; n=$((n-8)) else mask+=$((256-2**(8-n))); n=0 fi ((i<3)) && mask+=. done echo "$mask" }Sacrificing a few lines yields auditability and eliminates
eval.etc/rc.d/rc.wireless (1)
138-144: Quote variables inip addr/route addto survive spaces & emptiesThe static path now looks like:
[[ -n $IP4 && -n $MASK4 ]] && run ip -4 addr add $(unzero $IP4)/$MASK4 metric $INDEX dev $PORTIf
$IP4is empty the test prevents execution, but unquoted$MASK4or$INDEXcontaining unexpected whitespace still cause word-splitting. Safer:- run ip -4 addr add $(unzero $IP4)/$MASK4 metric $INDEX dev $PORT + run ip -4 addr add "$(unzero "$IP4")/$MASK4" metric "$INDEX" dev "$PORT"Apply the same pattern to IPv6 pathways.
etc/rc.d/rc.docker (1)
457-465: Metric computation may collide and overrun
metric $((1000 - 1 + $(index $NETWORK)))gives 999 + ifindex.
Two different shim interfaces can end up with the same metric when underlying NICs share the same ifindex modulo 1000 (e.g. wlan0 = 5, eth0 = 2 produce 1004 vs 1001 – fine, but eth1 with ifindex = 1003 → 2002 exceeds Linux metric limit 1024).
Additionally, negative or blank values break the arithmetic (see first comment).Recommend:
- Cap the metric ≤ 1024.
- Use a dedicated base per NIC family to avoid collisions.
Example:
base=800 # shim metric base idx=$(index "$NETWORK") (( idx=idx?idx:1 )) metric=$(( base + idx )) (( metric>1024 )) && metric=$((1024 - idx))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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(7 hunks)etc/rc.d/rc.inet1(28 hunks)etc/rc.d/rc.library.source(3 hunks)etc/rc.d/rc.local(1 hunks)etc/rc.d/rc.nginx(2 hunks)etc/rc.d/rc.wireless(6 hunks)sbin/create_network_ini(10 hunks)sbin/monitor_interface(1 hunks)
💤 Files with no reviewable changes (1)
- emhttp/plugins/dynamix/scripts/reload_services
🔇 Additional comments (9)
etc/rc.d/rc.local (2)
83-86: Effective addition of background interface state monitoring.This new block adds continuous monitoring of network interface states, which aligns well with the PR objective of allowing static IP assignments to coexist with wireless connections. Running it silently with output redirected to
/dev/nullis appropriate for a background process.
90-90: Consistent handling of monitor process outputs.Redirecting the output to
/dev/nullfor both monitoring processes ensures consistent behavior and prevents unnecessary output in logs.emhttp/plugins/dynamix/include/RoutingTable.php (1)
29-30: Improved route filtering for better network visibility.The enhanced filtering patterns now exclude
linkdownandbroadcastroutes from IPv4 output andlinkdownfrom IPv6 output. This provides a cleaner, more relevant routing table display by removing non-operational or system-internal routes.These changes align with the PR's objective of improving network configuration flexibility and reliability by ensuring that only meaningful routes are displayed to users.
emhttp/plugins/dynamix/scripts/update_services (1)
18-18: Improved service reload scheduling mechanism.This change separates the delay logic from the reload_services script, making the process more modular. Now the script schedules a job that performs the delay before calling reload_services without arguments.
This approach provides better separation of concerns - the delay handling is managed by the scheduling system rather than being embedded in the reload_services script itself.
etc/rc.d/rc.library.source (3)
8-8: Updated timestamp reflecting recent modifications.The timestamp update to May 2025 properly documents when the script was last modified.
59-60: Enhanced IP address selection using primary flag.Replacing
-temporarywithprimarywhen listing global scoped IP addresses improves how primary IP addresses are filtered in theshow()function. This is a more precise selection criterion for identifying the main IP addresses that should be used for services.This change aligns with the PR's objective of improving IP selection on network interfaces.
163-163: Consistent address filtering in interface checks.This change ensures consistent IP address filtering by using the same
primaryflag in thecheck()function, maintaining a uniform approach to IP address selection throughout the script.The change supports the PR's objective of improving automatic metric assignments and IP selection by ensuring that the most appropriate addresses are consistently identified.
etc/rc.d/rc.nginx (1)
540-545: Verifyipflag order – risk of empty LANIP/LANIP6
ip -4 -br addr show scope global primary dev $DEVis valid, however the IPv6 variant
ip -6 -br addr show scope global primary -deprecated dev $DEVrelies on a very recentiproute2where the-deprecatednegation flag can follow other selectors.
On older releases the parser errors out and nothing is returned, leaving both LANIP/LANIP6 empty and causing the self-signed certificate logic to mis-behave.Consider a safer, backwards-compatible form:
LANIP6=$(ip -6 -br addr show dev "$DEV" scope global primary | awk '{print $3;exit}' | sed 's/\/[0-9]*//') # …then filter deprecated locally: [[ $(ip -6 addr show dev "$DEV" primary deprecated scope global) ]] && LANIP6=Or guard the command with
|| trueplus a fallback.sbin/monitor_interface (1)
60-62: Missingprefixlenwill fail on older kernels
ip addr add 192.0.2.5 metric 1000 dev eth1is rejected unless the address includes the CIDR suffix.
Make sure$IPV4[$i]and$IPV6[$i]carry/xx; otherwise append the correct prefix or abort gracefully.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
emhttp/plugins/dynamix/EthX.page (1)
384-408:⚠️ Potential issue
metric()still called with literal placeholder – breaks new signature & VLAN cloning
metric('ethX.INDEX')is evaluated server-side before JS replacesINDEX, sometric()receives the non-existing interface nameethX.INDEX.
After the recent refactor,metric()expects a real interface (e.g.ethXorethX.100). The current code therefore returnsnull/empty, leaving the Metric box blank and confusing users cloning VLANs.Same issue for the IPv6 field a few lines below.
- <input type="text" name="METRIC:INDEX" ... value="<?=metric('ethX.INDEX')?>" ... + <!-- defer metric calculation until JS knows the actual VLAN id --> + <input type="text" name="METRIC:INDEX" + data-iface="ethX" + value="{{metric_placeholder}}" + ... + >Then, in
addVLAN()(or equivalent), replace{{metric_placeholder}}with
metric('ethX.' + vlanId)once the new row is instantiated.This restores correct defaults and resolves the long-standing bug reported earlier.
sbin/monitor_interface (1)
25-33: 🛠️ Refactor suggestion
switch()returns too early – flaps mis-detected after first secondThe loop exits on the first match, so even if the interface toggles back
within the 5-second window the change is acted upon.-for n in {1..5}; do - [[ $(state $1) == $2 ]] && return 1 || sleep 1 -done -return 0 +local stable=0 +for n in {1..5}; do + [[ $(state $1) == $2 ]] && stable=$((stable+1)) || stable=0 + sleep 1 +done +(( stable == 5 )) && return 1 || return 0This keeps monitoring until the new state stays unchanged for the full grace
period, eliminating false positives on transient carrier drops.etc/rc.d/rc.docker (1)
38-41: Guardindex()against missing /sys entriesIf the interface disappears (USB NIC removed, race condition)
catreturns
non-zero and the arithmetic expansion using its output aborts the script.-index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null -} +index(){ + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" +}emhttp/plugins/dynamix/Eth0.page (2)
58-65: Metric helper still shells-out & is injection-prone – previous concerns remain
The newmetric()implementation keeps usingexec("cat …")with raw$ethinterpolation. The performance hit (forkingcattwice per call) and the potential for command-injection that I pointed out earlier are still present. Please consider replacing the subshells withfile_get_contents()(or similar) and hardening the input as suggested in the earlier review.
922-944: Server-side callmetric('eth0.INDEX')is evaluated beforeINDEXis replaced → duplicate metrics
Because the placeholder hasn’t been substituted yet, every newly added VLAN receives the same pre-calculated metric. This was reported in the previous review and is still unresolved.etc/rc.d/rc.inet1 (3)
401-402: Wildcard fallback still expands to filenames – duplicate index collision risk
$(index *)expands*in the current directory, not to interface names, so the arithmetic still collapses to “highest index + 1 == 1001” for most cases, recreating the collision reported earlier.
407-408:ip addr add … metric …is not accepted by iproute2 (metric belongs to routes)
The address is accepted but the metric is silently ignored (or yields “either ‘peer’ or ‘broadcast’ expected” on older kernels). You need to set the metric on the route, not on the address – see the previous review for two safe patterns.Also applies to: 414-415
156-160:⚠️ Potential issue
index()returns empty on race → arithmetic expansion will abort script
If/sys/class/net/<iface>/ifindexis momentarily unreadable the function echoes nothing.
Later,INDEX=$((1000 + ${INDEX:- … }))will receive an empty operand and bash aborts with “value too great for base” (or similar). Provide a default of0and guard against read errors:index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local f="$SYSTEM/$1/ifindex" + [[ -r $f ]] && cat "$f" || echo 0 }sbin/create_network_ini (1)
32-36: Same fragileindex()helper as in rc.inet1
Empty output whenifindexis unreadable will break the arithmetic later in the script (lines 258 ff). Please apply the same fallback logic proposed forrc.inet1.etc/rc.d/rc.wireless (3)
15-15:⚠️ Potential issue
$SERVICEvs$SERVICES– variable name still inconsistentYou export
SERVICES="/usr/local/emhttp/webGui/scripts/update_services"but later invoke$SERVICE(singular). The typo prevents the script responsible for (re)loading services from ever running.-[[ $SRV4 == no && (-z $SRV6 || $SRV6 == no) ]] && $SERVICE 5 +[[ $SRV4 == no && (-z $SRV6 || $SRV6 == no) ]] && $SERVICES 5Do the same replacement in every occurrence (lines 278 & 337).
135-136:⚠️ Potential issueWildcard misuse (
index *) still produces unreliable metricsPassing
*toindexexpands to all filenames in the working directory.
Only the first arg is honoured, so the highest interface index is never found, and the resulting metric may collide with an existing one.-INDEX=$(index $PORT) -INDEX=$((3000 + ${INDEX:-$(($(index * | sort -n | tail -1) + 1))})) +# First try the real ifindex of $PORT ; otherwise derive the max index + 1 +INDEX=$(index "$PORT") +if [[ -z $INDEX ]]; then + INDEX=$(ls -1 "$SYSTEM"/*/ifindex | xargs -n1 cat | sort -n | tail -1) + INDEX=$((INDEX + 1)) +fi +INDEX=$((3000 + INDEX))
278-279:⚠️ Potential issueUndefined
$SERVICEstill used when restarting servicesSame typo as previously reported — replace with
$SERVICESor rename the variable consistently across the script to ensure conditional service restarts work.Also applies to: 337-338
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/Eth0.page (1)
730-731:min="0"accepts metric 0 but UI text says “lower is more preferred” – still confusing
Allowing0here while VLAN templates start at1keeps the old inconsistency. It also collides with Linux’ default‐route metric if other tools later normalise to0. Consider aligning all numeric inputs (METRIC,METRIC6) to the same lower bound.Also applies to: 753-754
etc/rc.d/rc.wireless (1)
32-35: Functionindex()needs quoting & safer path handling
$1is interpolated into a path without quotes, so interface names with glob/IFS characters could break.catwill emit an error when$SYSTEM/$1/ifindexis missing; consider returning a default or propagating a clear error.Minimal hardening:
-index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null +index() { + local ifname=$1 + [[ -r "$SYSTEM/$ifname/ifindex" ]] && cat "$SYSTEM/$ifname/ifindex" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (13)
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(7 hunks)etc/rc.d/rc.inet1(28 hunks)etc/rc.d/rc.library.source(3 hunks)etc/rc.d/rc.local(1 hunks)etc/rc.d/rc.nginx(2 hunks)etc/rc.d/rc.wireless(6 hunks)sbin/create_network_ini(10 hunks)sbin/monitor_interface(1 hunks)
💤 Files with no reviewable changes (1)
- emhttp/plugins/dynamix/scripts/reload_services
🔇 Additional comments (8)
etc/rc.d/rc.local (2)
83-86: Monitor interface addition improves network stabilityThis new code block integrates the interface state monitoring system into the Unraid OS startup process, which is essential for dynamically managing static IP assignments as interfaces change state.
90-90: Output redirection is properly aligned with previous changeModifying the monitor_nchan invocation to redirect output to /dev/null keeps error handling consistent with the newly added monitor_interface invocation.
emhttp/plugins/dynamix/scripts/update_services (1)
18-18: Improved separation of concerns in service reloadingThis modification properly decouples delay handling from the reload_services script by using the 'at' command to schedule execution after the specified delay. This approach is more flexible and follows better scripting practices.
emhttp/plugins/dynamix/include/RoutingTable.php (1)
29-30: Enhanced routing table filtering improves UI relevanceThe extended grep patterns remove unnecessary routes like linkdown and broadcast entries from the routing table display. This makes the routing table more focused on relevant, active routes that administrators need to see.
These improvements align well with the PR objective of enhancing IP selection on network interfaces by ensuring only relevant route entries are shown in the UI.
etc/rc.d/rc.library.source (3)
8-8: Version comment updated appropriatelyThe timestamp in the comment has been updated to reflect the latest changes to the file.
59-60: Improved IP address filtering for better network managementSwitching from
-temporarytoprimaryflag in the IP command focuses on primary addresses, which is a more appropriate filtering approach for interface management.This change is crucial for the PR objective of allowing static IP assignments to coexist with wireless connections, as it ensures that the system correctly identifies and uses primary addresses for network operations.
163-163: Consistent primary address filtering in network interface detectionThis change maintains consistency with the earlier modifications to use primary address filtering instead of temporary address exclusion.
The approach of focusing on primary addresses across all IP address retrieval operations in the library creates a more reliable basis for the automatic metric assignments mentioned in the PR objectives.
emhttp/plugins/dynamix/Eth0.page (1)
476-480: Nice simplification ofremoveVLANselector handling
Usingclosest('form')followed by a singlechangetrigger makes the intent clearer and removes brittle ID parsing. ✔️
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
etc/rc.d/rc.docker (6)
38-41:⚠️ Potential issueGuard
index()against missing /sys entriesThe
index()function will fail if the interface doesn't exist or if the ifindex file is inaccessible, which could cause script failures when the output is used in arithmetic operations (e.g., line 427, 451).Apply this fix:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }This keeps the script resilient when an interface disappears unexpectedly.
221-221:⚠️ Potential issueFix incompatible
primaryfilter with brief output formatThe
ip -br addr showcommand does not support theprimaryflag, causing$THISIPto be empty and breaking downstream logic.Replace with:
- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev "$LAN" | awk 'NR==1{print $2}' | cut -d/ -f1)
223-223:⚠️ Potential issueFix incorrect route lookup syntax
The current command doesn't include the
tokeyword needed for proper routing lookups.Update to:
- local NETWORK=$(ip -4 route show "$THISIP" dev "$LAN" | awk '{print $1;exit}') + local NETWORK=$(ip -4 route show to "$THISIP" dev "$LAN" | awk '{print $1;exit}')
349-349:⚠️ Potential issueFix incompatible
primaryfilter with brief output formatSame issue as on line 221 - the
ip -br addr showcommand doesn't supportprimaryfiltering.Replace with:
- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev "$NETWORK" | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=
359-359: 🛠️ Refactor suggestionUpdate IPv6 address retrieval to match IPv4 pattern
The IPv6 address retrieval also uses
-brwhich is inconsistent with the needed pattern forprimary.Replace with:
- [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev "$NETWORK" | awk 'NR==1{print $2}' | grep -Pvm1 '^.+/128|^$') || IPV6=
506-508:⚠️ Potential issueFix incompatible
primaryfilter in shim-wlan0 cleanupThe
primaryfilter doesn't work with brief output format here either.Replace with:
- NET=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + NET=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{print $2,$3,$4}' | cut -d/ -f1)
🧹 Nitpick comments (1)
etc/rc.d/rc.docker (1)
457-458: Consider quoting variables in command substitutionWhen running commands in a subshell, consider quoting paths and variable references.
- IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" + IPS="$(ip -4 -br addr show scope global dev "$VHOST" | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(9 hunks)
🔇 Additional comments (3)
etc/rc.d/rc.docker (3)
444-451: Good enhancement: Interface-specific metric calculationsNice improvement to handle different interface types with appropriate base metrics (3000 for wireless, 1000 for others). This helps prioritize routing correctly between wired and wireless interfaces.
454-459: Good implementation of smart IP assignmentThe code now only adds the parent's IPv4 address if not already present, which avoids duplicate addresses. Using
metric $INDEXensures consistent routing priorities.
427-428: Add guard against invalid interface metricsThe metric calculation depends on the
index()function which currently doesn't handle errors properly.If the
index()function is updated as recommended, this line should work correctly. Otherwise, ensure that a default value is used if$(index $NETWORK)returns empty:- NET="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" + local net_index=$(index $NETWORK) + net_index=${net_index:-0} + NET="$IPV4 metric $((1000 - 1 + net_index))"
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
etc/rc.d/rc.docker (3)
38-41: Guardindex()against missing /sys entriesThe new
index()function doesn't handle cases where the interface doesn't exist. This could lead to empty values being used in arithmetic expressions, causing "value required" errors that abort the script.Apply this fix to make the script more resilient when interfaces disappear:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
221-223: Fix incorrect IP commands using brief mode with primary filterThe
ip -4 -br addr showcommand with theprimaryflag doesn't work correctly as the brief mode doesn't support theprimaryfilter. Additionally, the route lookup syntax is incorrect.Apply these fixes:
- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') - local NETWORK=$(ip -4 route show $THISIP dev $LAN | awk '{print $1;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1) + local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}')
505-508: 🛠️ Refactor suggestionFix shim-wlan0 cleanup command
The cleanup for shim-wlan0 uses the problematic brief mode with primary filter that doesn't work correctly.
Apply this fix:
if [[ -e $SYSTEM/shim-wlan0 ]]; then - NET=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + NET=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{split($2,a,"/"); print a[1]" metric "$NF}') [[ -n $NET ]] && run ip addr del $NET dev shim-wlan0 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(9 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)
404-406: Network removal logic is now unconditionalThe code now removes and recreates Docker networks unconditionally, which simplifies logic but could cause momentary network disruptions during restarts.
499-503: Improved condition for selective interface cleanupThis change now properly excludes wlan interfaces from address flushing, which aligns with the dynamic metric approach and prevents potential connectivity issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
etc/rc.d/rc.docker (11)
221-221:⚠️ Potential issueFix the IP address retrieval to work correctly with 'primary' flag
The
ip -brcommand doesn't support theprimaryfilter, causing this command to return nothing and breaking downstream logic.-local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') +local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1)This change uses the full-format command which properly supports the
primaryflag.
223-223:⚠️ Potential issueAdd the
tokeyword to ensure correct route lookupThe current route lookup syntax is incorrect and may fail to retrieve the network.
-local NETWORK=$(ip -4 route show $THISIP dev $LAN | awk '{print $1;exit}') +local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}')Without the
tokeyword,$NETWORKmay be empty and subsequent operations will fail.
348-348:⚠️ Potential issueFix IPv4 retrieval for custom networks
Same issue as line 221 - the brief format doesn't support
primaryfiltering.-[[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= +[[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=
359-359:⚠️ Potential issueFix IPv6 retrieval for custom networks
Same issue as with the IPv4 retrieval - brief format doesn't support
primaryfiltering.-[[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= +[[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '/inet6/{print $2}' | grep -Pvm1 '^.+/128|^$') || IPV6=
427-428: 🛠️ Refactor suggestionAdd safeguards to the metric calculation for bridge networks
The current implementation could fail if the index function returns nothing.
-NET="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" +local idx=$(index $NETWORK) +idx=${idx:-0} +NET="$IPV4 metric $((1000 - 1 + idx))"This change stores the index in a variable with a default value of 0 if empty.
431-431: 🛠️ Refactor suggestionAdd existence check before writing to IPv6 configuration
The current implementation assumes the IPv6 configuration directory exists.
-echo 1 >$CONF6/$LINK/disable_ipv6 +[[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6This change prevents errors if the directory doesn't exist.
451-451: 🛠️ Refactor suggestionAdd safeguards to the metric calculation for non-bridge interfaces
Same issue as line 427 - needs protection against empty index values.
-INDEX=$(($INDEX - 1 + $(index $NETWORK))) +local idx=$(index $NETWORK) +idx=${idx:-0} +INDEX=$(($INDEX - 1 + idx))
454-454: 🛠️ Refactor suggestionAdd existence check before writing to IPv6 configuration
Same issue as line 431 - needs to check if the directory exists.
-echo 1 >$CONF6/$VHOST/disable_ipv6 +[[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6
457-457: 🛠️ Refactor suggestionFix IP address retrieval for vhost interface
The current implementation might not correctly extract IP addresses.
-IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" +IPS="$(ip -4 addr show scope global dev $VHOST | awk '{for(i=1;i<=NF;i++) if($i~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+$/) {print $i}}' | sed -r 's/ metric [0-9]+//g')"This change uses a more robust pattern matching approach to extract IPv4 addresses.
506-506:⚠️ Potential issueFix IP address retrieval for shim-wlan0 cleanup
Same issue as other IP retrieval lines - brief format doesn't support
primaryfiltering.-NET=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') +NET=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{print $2,$3,$4}' | sed 's|/[0-9]*||')
38-41: 🛠️ Refactor suggestionStrengthen the
index()function against missing interfacesThis new helper function reads interface indices from sysfs, but lacks error handling when interfaces don't exist. When used in arithmetic expressions (lines 427, 451), it could cause script failures.
Apply this defensive implementation:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }This ensures a default value of 0 when an interface doesn't exist or is temporarily unavailable, preventing arithmetic errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
etc/rc.d/rc.docker (10)
221-226:⚠️ Potential issueFix broken
ipcommand with unsupportedprimaryflag in brief modeThe
ip -br addr showcommand does not support theprimaryflag, causing the command to potentially return nothing, which breaks downstream logic.Apply this fix:
- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1)
38-41:⚠️ Potential issueImplement error handling for the index() function
The
index()function could return an empty string if the interface doesn't exist or the file isn't readable, which would cause arithmetic errors when used in expressions like$((1000 - 1 + $(index $NETWORK))).Apply this fix to make the function resilient to missing interfaces:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
227-229:⚠️ Potential issueAdd missing
tokeyword in route lookupThe route lookup command is missing the
tokeyword, which can cause$NETWORKto be empty when callingip route addinside the container.Apply this fix:
- local NETWORK=$(ip -4 route show $THISIP dev $LAN | awk '{print $1;exit}') + local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}')
341-342:⚠️ Potential issueFix unsupported
primaryflag in brief IP command (IPv4)The
ip -br addr showcommand does not support theprimaryflag here too, causing the command to potentially return nothing.Apply this fix:
- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=
352-353:⚠️ Potential issueFix unsupported
primaryflag in brief IP command (IPv6)The
ip -br addr showcommand does not support theprimaryflag for IPv6 address selection either.Apply this fix:
- # get IPv6 address - ignore any /128 networks - [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + # get IPv6 address - ignore any /128 networks + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '!/\/128/ && /inet6/{print $2; exit}') || IPV6=
354-354:⚠️ Potential issueAdd missing
tokeyword in IPv6 route lookupThe IPv6 route lookup command is missing the
tokeyword, which can cause incorrect subnet determination.Apply this fix:
- SUBNET6=$(ip -6 route show $IPV6 dev $NETWORK | awk '{print $1;exit}') + SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}')
420-428:⚠️ Potential issueAdd safeguards to dynamic metric calculation for bridge networks
The metric calculation could fail if the index function returns nothing, and the IPv6 configuration lacks an existence check.
Apply these fixes:
- NET="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" + local idx=$(index $NETWORK) + idx=${idx:-0} + NET="$IPV4 metric $((1000 - 1 + idx))" # create shim interface [[ -e $SYSTEM/$LINK ]] || run ip link add link $NETWORK name $LINK type $ATTACH mode $MODE # disable IPv6 on shim interface - echo 1 >$CONF6/$LINK/disable_ipv6 + [[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6
437-452:⚠️ Potential issueAdd safeguards to dynamic metric calculation for non-bridge interfaces
Similar to the bridge networks, the metric calculation and IPv6 configuration for non-bridge interfaces need safeguards.
Apply these fixes:
if [[ $TYPE == wlan ]]; then VHOST=shim-$NETWORK INDEX=3000 else VHOST=vhost${NETWORK//[^0-9.]/} INDEX=1000 fi - INDEX=$(($INDEX - 1 + $(index $NETWORK))) + local idx=$(index $NETWORK) + idx=${idx:-0} + INDEX=$(($INDEX - 1 + idx)) if [[ $DOCKER_ALLOW_ACCESS == yes && -n $IPV4 ]]; then # disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 run ip -6 addr flush dev $VHOST # copy parent IPv4 address to vhost interface - IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" + IPS="$(ip -4 addr show scope global dev $VHOST | awk '{for(i=1;i<=NF;i++) if($i~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+$/) {print $i}}' | sed -r 's/ metric [0-9]+//g')"
447-448:⚠️ Potential issueAdd existence check before manipulating $VHOST IPv6 settings
There's a potential issue when $DOCKER_ALLOW_ACCESS toggles from yes to no, where $VHOST may not exist yet the script attempts to manipulate it.
Add an existence guard for the IPv6 operations:
# disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 - run ip -6 addr flush dev $VHOST + if [[ -e $SYSTEM/$VHOST ]]; then + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 + run ip -6 addr flush dev $VHOST + fi
492-495:⚠️ Potential issueFix unsupported
primaryflag in brief IP command for shim-wlan0 cleanupAnother instance of the same issue with the
primaryflag in brief mode, this time in the shim-wlan0 cleanup.Apply this fix:
if [[ -e $SYSTEM/shim-wlan0 ]]; then - NET=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + NET=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{print $2" "$3" "$4}' | cut -d/ -f1) [[ -n $NET ]] && run ip addr del $NET dev shim-wlan0 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(12 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)
42-52: Well-implemented active() functionThe new
active()function correctly handles interface substitution, resolving physical interfaces to their bridge or bond counterparts when they exist. This improves the network setup logic throughout the script.
398-399: Improved network management strategyUnconditionally removing and recreating Docker networks ensures a clean state and helps prevent inconsistencies that might arise from partial updates.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
etc/rc.d/rc.docker (9)
516-516: Wrapcontainer_add_routewith therunhelper for consistent error handlingThe
container_add_routefunction itself suppresses errors and doesn't log failures, so invoking it directly will miss both error trapping and logging provided by therunwrapper.- container_add_route $CONTAINER + run container_add_route $CONTAINERLet's verify if this change is needed:
#!/bin/bash echo "Analyzing container_add_route function:" sed -n '/container_add_route/,/^}/p' etc/rc.d/rc.docker | grep -n "run " echo echo "Verify if function handles its own output:" sed -n '/container_add_route/,/^}/p' etc/rc.d/rc.docker
38-41:⚠️ Potential issueImprove the index() function to handle missing interfaces
The function doesn't handle cases where the interface doesn't exist or the read fails, which could lead to errors when using the returned value in arithmetic contexts (lines 424, 448).
Apply this fix to make the script more resilient when an interface disappears:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
232-232:⚠️ Potential issueFix unsupported "primary" filter in brief IP commands
The
ip -br addr showcommand does not support theprimaryflag, which could cause the command to return nothing and break downstream logic.Replace with:
- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1)
234-234:⚠️ Potential issueAdd the
tokeyword to ensure correct IPv4 route lookupThe current command syntax for route lookup doesn't specify the "to" keyword, which may cause issues with route identification.
- local NETWORK=$(ip -4 route show $THISIP dev $LAN | awk '{print $1;exit}') + local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}')
347-348:⚠️ Potential issueFix multiple occurrences of the "primary" filter issue
The ip command with
-brandprimarydoesn't work together here and in several other places.- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=
358-360:⚠️ Potential issueFix IPv6 address retrieval and route lookup
Both the IPv6 address retrieval and route lookup have the same issues as the IPv4 ones.
- [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk 'NR==1{print $2}' | grep -Pvm1 '^.+/128|^$') || IPV6= if [[ -n $IPV6 ]]; then - SUBNET6=$(ip -6 route show $IPV6 dev $NETWORK | awk '{print $1;exit}') + SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}')
424-432: 🛠️ Refactor suggestionDynamic metric calculation for bridge networks needs safeguards
The metric calculation could fail if the index function returns nothing, and the IPv6 configuration lacks an existence check.
- IPV4="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" + local idx=$(index $NETWORK) + idx=${idx:-0} + IPV4="$IPV4 metric $((1000 - 1 + idx))" # create shim interface [[ -e $SYSTEM/$LINK ]] || run ip link add link $NETWORK name $LINK type $ATTACH mode $MODE # disable IPv6 on shim interface - echo 1 >$CONF6/$LINK/disable_ipv6 + [[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6
441-456: 🛠️ Refactor suggestionDynamic metric calculation for non-bridge interfaces needs safeguards
Similar to the bridge network case, the metric calculation and IPv6 configuration need safeguards.
if [[ $TYPE == wlan ]]; then VHOST=shim-$NETWORK INDEX=3000 else VHOST=vhost${NETWORK//[^0-9.]/} INDEX=1000 fi - INDEX=$(($INDEX - 1 + $(index $NETWORK))) + local idx=$(index $NETWORK) + idx=${idx:-0} + INDEX=$(($INDEX - 1 + idx)) if [[ $DOCKER_ALLOW_ACCESS == yes && -n $IPV4 ]]; then # disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 run ip -6 addr flush dev $VHOST # copy parent IPv4 address to vhost interface - IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" + IPS="$(ip -4 addr show scope global dev $VHOST | awk '{for(i=1;i<=NF;i++) if($i~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+$/) {print $i}}' | sed -r 's/ metric [0-9]+//g')"
497-498:⚠️ Potential issueFix broken IP address retrieval on shim-wlan0
This uses the problematic brief mode for IP address retrieval with
primaryfilter.- IPV4=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + IPV4=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{print $2}')
🧹 Nitpick comments (1)
etc/rc.d/rc.docker (1)
187-189: New ipv4_exists() function is clean but could use improved commandThe function uses
-br addr show towhich is good, but could be more robust.Consider using grep for more accurate matching:
ipv4_exists(){ - ip -4 -br addr show to $2 dev $1 | awk '{print $3;exit}' + ip -4 addr show dev $1 | grep -q "inet $2/" + return $? }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(13 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)
43-52: Good addition of the active() helper functionThis function properly resolves eth interfaces to their active form (br or bond) when applicable, improving consistency throughout the script.
178-179: Good improvement for wireless interfacesThe forced ipvlan driver for wlan interfaces ensures compatibility with wireless networks that don't support promiscuous mode.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
etc/rc.d/rc.docker (12)
38-41: Guardindex()against missing /sys entriesThe function retrieves the interface index but doesn't handle the case where the interface doesn't exist, which could break arithmetic operations that use this value.
Apply this defensive coding fix:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
232-233: Use fullip addr show- brief mode dropsprimaryfilterThe
-br(brief) output format does not support theprimaryfilter, causing this command to return nothing and breaking downstream logic.- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1)
234-235: Fix incorrectiproute lookupThe current command is missing the
tokeyword, causing$NETWORKto be potentially empty.- local NETWORK=$(ip -4 route show $THISIP dev $LAN | awk '{print $1;exit}') + local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}')
347-348: Use fullip addr showfor IPv4 address retrievalThe
-br(brief) output format does not support theprimaryfilter, causing this command to return nothing and breaking Docker network setup.-[[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= +[[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=
358-360: Fix IPv6 address retrieval and route lookupThe IPv6 address retrieval has two issues:
- The
-brformat doesn't supportprimaryfiltering- The route lookup needs the
tokeyword-# get IPv6 address - ignore any /128 networks -[[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= -SUBNET6=$(ip -6 route show $IPV6 dev $NETWORK | awk '{print $1;exit}') +# get IPv6 address - ignore any /128 networks +[[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '/inet6/{for(i=1;i<=NF;i++) if($i~/^[0-9a-f]+:[0-9a-f:]+\/[0-9]+$/ && !($i~/\/128$/)) {print $i; exit}}') || IPV6= +SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}')
424-425: Ensure metrics calculation handles missing indicesThe dynamic metric calculation could fail if the index function returns nothing.
-IPV4="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" +local idx=$(index $NETWORK) +idx=${idx:-0} +IPV4="$IPV4 metric $((1000 - 1 + idx))"
441-449: Fix the dynamic metric calculation for wireless and other interfacesSimilar to the bridge interface case, the metric calculation needs safeguards against missing indices.
if [[ $TYPE == wlan ]]; then VHOST=shim-$NETWORK INDEX=3000 else VHOST=vhost${NETWORK//[^0-9.]/} INDEX=1000 fi -INDEX=$(($INDEX - 1 + $(index $NETWORK))) +local idx=$(index $NETWORK) +idx=${idx:-0} +INDEX=$(($INDEX - 1 + idx))
451-452: Add existence check for IPv6 operations on vhost interfacesThe script doesn't check if the IPv6 configuration directory exists before accessing it.
-echo 1 >$CONF6/$VHOST/disable_ipv6 -run ip -6 addr flush dev $VHOST +[[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 +run ip -6 addr flush dev $VHOST
454-455: Improve IP address retrieval and comparison on vhost interfaceThe current implementation uses a brittle awk approach and a regex comparison that may not reliably detect the address.
-IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" -[[ ! $IPS =~ $IPV4 ]] && run ip addr add $IPV4 metric $INDEX dev $VHOST +if ! ip -4 addr show dev $VHOST | grep -q "$IPV4"; then + run ip addr add $IPV4 metric $INDEX dev $VHOST +fi
496-499: Fix IPv4 address retrieval for shim-wlan0 cleanupThe same issue with
-br addr show primarynot supporting the primary filter applies here.if [[ -e $SYSTEM/shim-wlan0 ]]; then - IPV4=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + IPV4=$(ip -4 addr show scope global primary dev shim-wlan0 | awk 'NR==1{for(i=1;i<=NF;i++) if($i~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+$/) {print $i}}') [[ -n $IPV4 ]] && run ip addr del $IPV4 dev shim-wlan0 fi
516-516: Wrapcontainer_add_routewith therunhelperThe
container_add_routefunction itself suppresses errors and doesn't log failures. Invoking it directly will miss both error trapping and logging.-container_add_route $CONTAINER +run container_add_route $CONTAINER
428-429:⚠️ Potential issueAdd existence check before IPv6 operations
The script doesn't check if the IPv6 configuration directory exists before accessing it.
-echo 1 >$CONF6/$LINK/disable_ipv6 -run ip -6 addr flush dev $LINK +[[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6 +run ip -6 addr flush dev $LINK
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(12 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)
43-52: LGTM!The new
active()helper function provides a clean abstraction to map Ethernet interfaces to their corresponding bridge or bond interfaces, supporting the correct handling of interface references throughout the script.
457-459: LGTM!The code properly removes parent IPv4 addresses from vhost interfaces when Docker access is disabled but the address exists.
in accordance with CodeRabbit
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
sbin/create_network_ini (2)
149-156: Repeated issues already reported: unsupportedprimaryfilter in-brmode andmetriconip addr addThe code still:
- uses
ip -[46] -br addr show … primary …–-brdoesn’t support the
primaryselector, the command returns nothing;- passes
metrictoip addr add/del, whichiproute2ignores (or errors)
because metrics belong to routes, not addresses;- calculates
INDEXwith$(index …)without a default, risking a crash.These concerns were raised in earlier reviews; please address or justify.
Also applies to: 171-177, 204-208, 224-226, 253-269, 280-291
33-35:⚠️ Potential issue
index()returns empty on failure – guard to avoid arithmetic aborts
index()directlycats the sysfs file and prints nothing when the interface
doesn’t exist (USB NIC removed, race-condition, typo).
Down-stream arithmetic such asINDEX=$((… + $(index $interface)))then receives
an empty string and the whole script exits with “value required”.index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }etc/rc.d/rc.inet1 (2)
152-156:index()helper still lacks fallback valueSame risk as previously flagged: when
/sys/class/net/$iface/ifindex
vanishes, arithmetic with an empty string aborts the script.
398-404: Metrics applied to addresses andindex *fallback remain unresolved•
ip addr add … metric $INDEXis still used – unsupported byiproute2;
• fallbackINDEX=$((1000 + ${INDEX:-$(($(index * | …))}))expands*to
filenames, producing nonsense.Issues already noted in prior reviews.
Also applies to: 410-411
etc/rc.d/rc.docker (4)
38-42:index()without default value can crash arithmetic laterIdentical problem as in other scripts – please add a safe default.
187-189:ipv4_exists()uses incorrect & unsupported command combo
ip -4 -br addr show to …is syntactically wrong (tofilters routes, not
addresses) and-brdoesn’t understandto. See previous review.
232-235: Brief-modeip … primary+ metric-on-address issues persistThe same two issues (unsupported
primaryfilter with-brand illegal
metriconip addr add/del) appear multiple times in these blocks.Also applies to: 348-361, 425-456, 497-499
516-516:container_add_routeshould be wrapped withrunfor uniform loggingEarlier feedback highlighted the loss of error capture when calling this helper
directly; it hasn’t been changed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
etc/rc.d/rc.docker(12 hunks)etc/rc.d/rc.inet1(28 hunks)sbin/create_network_ini(10 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
etc/rc.d/rc.docker (9)
38-41: Protect index() from non-existent interfacesThe index() function doesn't handle the case when an interface doesn't exist, which could cause arithmetic errors later when used in calculations like
$((1000 - 1 + $(index $NETWORK))).index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
221-236: Fix unsupported primary filter in brief modeThe
ip -br addr showcommand doesn't support theprimaryflag, causing the command to return empty results.if [[ $PID -gt 0 && "eth0 br0 bond0 wlan0" =~ $LAN ]]; then - local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN | awk 'NR==1{print $2}' | cut -d/ -f1) for CFG in /etc/wireguard/wg*.cfg ; do local NETWORK=$(ip -4 route show to $THISIP dev $LAN | awk '{print $1;exit}') [[ -n $NETWORK ]] && nsenter -n -t $PID ip -4 route add $NETWORK via ${THISIP%/*} dev $LAN 2>/dev/null done fi
347-350: Fix unsupported primary filter in brief mode (IPV4 retrieval)Same issue here -
ip -br addr showdoesn't support theprimaryflag.- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}') || IPV4= if [[ -n $IPV4 ]]; then SUBNET=$(ip -4 route show to $IPV4 dev $NETWORK | awk '{print $1;exit}')
358-361: Fix unsupported primary filter in brief mode (IPV6 retrieval)Same issue - also, the current IPv6 address retrieval could be simplified.
- # get IPv6 address - ignore any /128 networks - [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + # get IPv6 address - ignore any /128 networks + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '/inet6/ && !/:128$/ {print $2; exit}') || IPV6= if [[ -n $IPV6 ]]; then SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}')
441-456: Guard dynamic metric calculation for non-bridge interfacesSame issue with the index calculation, plus missing existence check for IPv6 configuration.
else if [[ $TYPE == wlan ]]; then VHOST=shim-$NETWORK INDEX=3000 else VHOST=vhost${NETWORK//[^0-9.]/} INDEX=1000 fi - INDEX=$(($INDEX - 1 + $(index $NETWORK))) + local idx=$(index $NETWORK) + idx=${idx:-0} + INDEX=$(($INDEX - 1 + idx)) if [[ -n $IPV4 && $DOCKER_ALLOW_ACCESS == yes ]]; then # disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 run ip -6 addr flush dev $VHOST # copy parent IPv4 address to vhost interface - IPS="$(ip -4 -br addr show scope global dev $VHOST | awk '{$1="";$2="";print;exit}' | sed -r 's/ metric [0-9]+//g')" + IPS="$(ip -4 addr show scope global dev $VHOST | awk '{for(i=1;i<=NF;i++) if($i~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+$/) {print $i}}' | sed -r 's/ metric [0-9]+//g')" [[ ! $IPS =~ $IPV4 ]] && run ip addr add $IPV4 metric $INDEX dev $VHOST
496-499: Fix unsupported primary filter in brief mode (shim-wlan0 cleanup)Another instance of using brief mode with the
primaryflag.if [[ -e $SYSTEM/shim-wlan0 ]]; then - IPV4=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + IPV4=$(ip -4 addr show scope global primary dev shim-wlan0 | awk '/inet / {print $2, $3, $4; exit}') [[ -n $IPV4 ]] && run ip addr del $IPV4 dev shim-wlan0 fi
516-517: Wrap container_add_route with run helperThe container_add_route function should be wrapped with the run helper for consistent error handling.
- container_add_route $CONTAINER + run container_add_route $CONTAINER log "$CONTAINER: started successfully!" &
423-432:⚠️ Potential issueGuard against arithmetic errors in metric calculation
Use a safe approach to handle potentially empty index values to prevent arithmetic errors.
if [[ $DOCKER_ALLOW_ACCESS == yes && -n $IPV4 ]]; then - IPV4="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" + local idx=$(index $NETWORK) + idx=${idx:-0} + IPV4="$IPV4 metric $((1000 - 1 + idx))" # create shim interface [[ -e $SYSTEM/$LINK ]] || run ip link add link $NETWORK name $LINK type $ATTACH mode $MODE # disable IPv6 on shim interface - echo 1 >$CONF6/$LINK/disable_ipv6 + [[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6 run ip -6 addr flush dev $LINK # copy parent IPv4 address to shim interface run ip addr add $IPV4 dev $LINK
187-189:⚠️ Potential issueFix parameter order in ipv4_exists() function
The current function has parameters in an unusual order and the command syntax is incorrect. It tries to filter addresses by the second parameter but uses "show $2" which doesn't work for filtering addresses.
ipv4_exists(){ - ip -4 -br addr show $2 dev $1 | awk '{print $3;exit}' + ip -4 addr show dev $1 | grep -q "$2" && echo "$2" || echo "" }
🧹 Nitpick comments (1)
etc/rc.d/rc.docker (1)
401-403: Improve network operation messages and error handlingThe current implementation suppresses all error output when removing networks, which could hide important errors during network setup.
# delete and recreate unconditionally log "Processing... $NETWORK" - docker network rm $NETWORK &>/dev/null + docker network rm $NETWORK 2>/dev/null || true docker network create -d $ATTACH $SUBNET $GATEWAY $SERVER $RANGE $SUBNET6 $GATEWAY6 $SERVER6 -o parent=$VHOST $NETWORK | xargs docker network inspect -f "created network $ATTACH {{.Name}} with subnets: {{range .IPAM.Config}}{{.Subnet}}; {{end}}" 2>/dev/null | log
This reverts commit 018d9fe.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
etc/rc.d/rc.docker (10)
221-236:⚠️ Potential issueFix unsupported "primary" filter in brief IP commands
The
ip -br addr showcommand does not support theprimaryflag, so calls like this return nothing and break downstream logic.- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev $LAN" | awk 'NR==1{print $2}' | cut -d/ -f1)Also, good job adding the
tokeyword in the route lookup below, but make sure the above IP retrieval works first.
38-41:⚠️ Potential issueGuard
index()against missing /sys entriesThe function retrieves an interface's index from sysfs but doesn't handle errors when interfaces don't exist. This could cause arithmetic errors later when used in metric calculations.
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
187-190:⚠️ Potential issueFix parameter order in
ipv4_exists()functionThe
ip -4 -br addr show to $2 dev $1command has a logical issue - thetoparameter filters routes, not addresses. This won't work as intended for verifying IPv4 address existence.ipv4_exists(){ - ip -4 -br addr show to $2 dev $1 | awk '{print $3;exit}' + ip -4 addr show dev $1 | grep -q "$2" && echo "$2" || echo "" }
348-364:⚠️ Potential issueFix IPv4/IPv6 address retrieval in multiple places
Similar to earlier issues, these commands using
-brwith theprimaryflag will fail since brief mode doesn't support theprimaryfilter.- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}' | cut -d/ -f1) || IPV4=- [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '/inet6/{print $2}' | grep -Pvm1 '^.+/128|^$') || IPV6=The IPv6 route lookup is correctly using the
tokeyword.
425-426:⚠️ Potential issueDynamic metric calculation for bridge networks needs safeguards
The metric calculation could fail if the index function returns nothing. Add error handling to avoid arithmetic errors.
- IPV4="$IPV4 metric $((1000 - 1 + $(index $NETWORK)))" + local idx=$(index $NETWORK) + idx=${idx:-0} + IPV4="$IPV4 metric $((1000 - 1 + idx))"
429-431:⚠️ Potential issueMissing existence check before manipulating IPv6 configuration
The script tries to disable IPv6 and flush addresses without checking if the interface exists.
# disable IPv6 on shim interface - echo 1 >$CONF6/$LINK/disable_ipv6 - run ip -6 addr flush dev $LINK + [[ -d $CONF6/$LINK ]] && echo 1 >$CONF6/$LINK/disable_ipv6 + [[ -e $SYSTEM/$LINK ]] && run ip -6 addr flush dev $LINK
442-456:⚠️ Potential issueDynamic metric calculation for non-bridge interfaces needs safeguards
Similar to the bridge network case, the metric calculation and IPv6 configuration need safeguards.
INDEX=$(($INDEX - 1 + $(index $NETWORK))) + local idx=$(index $NETWORK) + idx=${idx:-0} + INDEX=$(($INDEX - 1 + idx)) if [[ $DOCKER_ALLOW_ACCESS == yes && -n $IPV4 ]]; then # disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6
496-499:⚠️ Potential issueFix IPv4 address retrieval for shim-wlan0 cleanup
This also uses the problematic brief mode with primary filter.
if [[ -e $SYSTEM/shim-wlan0 ]]; then - IPV4=$(ip -4 -br addr show scope global primary dev shim-wlan0 | awk '{print $3,$4,$5;exit}') + IPV4=$(ip -4 addr show scope global primary dev shim-wlan0 | awk '/inet /{print $2,$3,$4}' | head -1) [[ -n $IPV4 ]] && run ip addr del $IPV4 dev shim-wlan0 fi
516-516:⚠️ Potential issueWrap
container_add_routewith therunhelper for consistent error handlingThe
runwrapper was removed when callingcontainer_add_route. This could impact error handling and logging.- container_add_route $CONTAINER + run container_add_route $CONTAINER
361-361:⚠️ Potential issueAdd the
tokeyword to ensure correct IPv6 route lookupSimilar to the IPv4 route lookup, this needs the
tokeyword.- SUBNET6=$(ip -6 route show $IPV6 dev $NETWORK | awk '{print $1;exit}') + SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(12 hunks)
🔇 Additional comments (3)
etc/rc.d/rc.docker (3)
43-52: Solid implementation of theactive()helper functionThe function properly handles interface name resolution by checking for corresponding bridge or bond interfaces, which improves network handling flexibility.
116-116: Correct route lookup syntax withtokeywordGood improvement using the correct syntax for IPv6 route lookup by adding the
tokeyword, which helps retrieve the proper route information.
179-180: Proper handling for wlan interfacesForcing
ipvlanattachment mode for wireless interfaces is appropriate as it allows network connectivity without requiring wireless interfaces to operate in promiscuous mode.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
etc/rc.d/rc.docker (8)
38-41:⚠️ Potential issueFix potential arithmetic expansion error when interface doesn't exist
The current
index()function doesn't handle the case when an interface doesn't exist, which could cause arithmetic errors when used in calculations.Apply this fix to make the script resilient:
index(){ - cat $SYSTEM/$1/ifindex 2>/dev/null + local idx + idx=$(cat "$SYSTEM/$1/ifindex" 2>/dev/null) || idx=0 + echo "$idx" }
515-516: 🛠️ Refactor suggestionWrap
container_add_routewithrunhelperThe
container_add_routefunction suppresses errors and doesn't log failures. Using it directly bypasses error trapping and logging.Apply this fix:
- container_add_route $CONTAINER + run container_add_route $CONTAINER
442-449:⚠️ Potential issueAdd safety checks for index in non-bridge interface metric calculation
Similar to the bridge case, this metric calculation needs a fallback value.
Apply this fix:
if [[ $TYPE == wlan ]]; then VHOST=shim-$NETWORK INDEX=3000 else VHOST=vhost${NETWORK//[^0-9.]/} INDEX=1000 fi - INDEX=$(($INDEX - 1 + $(index $NETWORK))) + local idx=$(index $NETWORK) + idx=${idx:-0} + INDEX=$(($INDEX - 1 + idx))
347-350:⚠️ Potential issueFix multiple unsupported primary filter instances
Same issue with
-brnot supportingprimaryfilter. This will cause$IPV4to be empty.Apply this fix:
- [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 -br addr show scope global primary dev $NETWORK | awk '{print $3;exit}') || IPV4= + [[ -z ${!AUTO} || ${!AUTO} =~ "4" ]] && IPV4=$(ip -4 addr show scope global primary dev $NETWORK | awk 'NR==1{print $2}') || IPV4=And for the next line:
- SUBNET=$(ip -4 route show to $IPV4 dev $NETWORK | awk '{print $1;exit}') + SUBNET=$(ip -4 route show to "${IPV4%/*}" dev $NETWORK | awk '{print $1;exit}')
358-360:⚠️ Potential issueFix IPv6 address retrieval and route lookup
The IPv6 address retrieval has the same issue with
-brandprimary, and the route lookup should include thetokeyword.Apply this fix:
- [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 -br addr show scope global primary -deprecated dev $NETWORK | awk -v RS='[[:space:]]+' '(NR>2){print}' | grep -Pvm1 '^.+/128|^$') || IPV6= + [[ -z ${!AUTO} || ${!AUTO} =~ "6" ]] && IPV6=$(ip -6 addr show scope global primary -deprecated dev $NETWORK | awk '{if(NR>1 && $0 !~ "/128") {print $2; exit}}') || IPV6=And for the next line:
- SUBNET6=$(ip -6 route show to $IPV6 dev $NETWORK | awk '{print $1;exit}') + SUBNET6=$(ip -6 route show to "${IPV6%/*}" dev $NETWORK | awk '{print $1;exit}')
187-189:⚠️ Potential issueFix incorrect parameter order in
ipv4_exist()The current implementation uses
towhich is for route filtering, not address filtering. This won't correctly detect IP address existence.Apply this fix:
ipv4_exist(){ - ip -4 -br addr show to $2 dev $1 | awk '{print $3;exit}' + ip -4 addr show dev "$1" | grep -w "${2}/" | awk '{print $2;exit}' }
450-455:⚠️ Potential issueAdd existence check before manipulating IPv6 configuration
When
$DOCKER_ALLOW_ACCESSchanges, the directory might not exist yet causing errors.Apply this fix:
if [[ -n $IPV4 && $DOCKER_ALLOW_ACCESS == yes ]]; then # disable IPv6 on vhost interface - echo 1 >$CONF6/$VHOST/disable_ipv6 + [[ -d $CONF6/$VHOST ]] && echo 1 >$CONF6/$VHOST/disable_ipv6 run ip -6 addr flush dev $VHOST # copy parent IPv4 address to vhost interface - [[ -z $(ipv4_exist $VHOST ${IPV4%/*}) ]] && run ip addr add $IPV4 metric $INDEX dev $VHOST + [[ -z $(ipv4_exist "$VHOST" "${IPV4%/*}") ]] && run ip addr add "$IPV4" metric $INDEX dev "$VHOST"
232-235:⚠️ Potential issueFix unsupported primary filter in brief IP command
The
ip -br addr showcommand does not support theprimaryflag, which causes the command to return nothing.Apply this fix:
- local THISIP=$(ip -4 -br addr show scope global primary dev $LAN | awk '{print $3;exit}') + local THISIP=$(ip -4 addr show scope global primary dev "$LAN" | awk 'NR==1{print $2}' | cut -d/ -f1)
🧹 Nitpick comments (1)
etc/rc.d/rc.docker (1)
456-459: Properly quote variables in address removalEnsure variables are properly quoted to avoid word splitting with spaces or special characters.
Apply this fix:
- run ip addr del $IPV4 metric $INDEX dev $VHOST + run ip addr del "$IPV4" metric $INDEX dev "$VHOST"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
etc/rc.d/rc.docker(12 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)
44-52: LGTM - Good mapping implementation for active interfacesThe
active()function correctly handles the mapping of ethernet interfaces to their corresponding bridge or bonded interfaces.
221-223: Functionread_dom()is using IFS correctlyThe XML parsing helper function correctly sets IFS and reads entity content.
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores