Skip to content

Miscellaneous fixes and updates#2235

Merged
limetech merged 10 commits into
unraid:masterfrom
bergware:master
Jun 8, 2025
Merged

Miscellaneous fixes and updates#2235
limetech merged 10 commits into
unraid:masterfrom
bergware:master

Conversation

@bergware

@bergware bergware commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Improved file deletion logic to ensure files are properly removed when necessary.
  • New Features
    • VM network interface MAC address field now auto-fills and disables editing when using the host’s wireless interface.
  • Enhancements
    • Simplified and clarified network interface display and configuration in Docker settings.
    • Improved accuracy and consistency in network route and IP address handling across interfaces.
    • Enhanced network interface monitoring with more reliable link state detection and stable state confirmation.
    • Updated default gateway checkbox behavior for Ethernet interfaces based on metric values.
    • Streamlined static IP configuration file generation with added gateway route entries.
    • Refined IP address and route assignment commands for wireless and Docker shim interfaces.

@coderabbitai

coderabbitai Bot commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

A conditional check was modified in the KeyUpload.php file to allow a file at a specified path to be deleted if it exists, regardless of the value of a related variable. The previous dependency on the variable's truthiness was removed, simplifying the condition for file deletion.

Multiple network-related scripts and pages were refined for clarity and correctness: DockerSettings.page removed functions hiding interfaces and simplified UI logic; libvirt_helpers.php adjusted network interface regex matching; Custom.form.php pre-fills and disables MAC input for wlan0; Eth0.page and EthX.page changed gateway checkbox logic to depend on metric values; rc.docker and rc.inet1 scripts simplified IP and metric handling; rc.wireless reordered metric option in commands; create_network_ini improved static IP and gateway writing; monitor_interface replaced operstate with carrier checks and refined IP/route management.

Changes

File(s) Change Summary
emhttp/plugins/dynamix/include/KeyUpload.php Modified conditional to unlink a file if it exists, removing dependency on variable truthiness.
emhttp/plugins/dynamix.docker.manager/DockerSettings.page Removed hide_wlan and hide_eth functions; simplified network interface UI logic; refined network command usage and formatting.
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php Adjusted getValidNetworks() interface regex and removed unshift of virbr0; minor formatting in vm_clone().
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php Added reading of wlan0 MAC address; updated JS to disable MAC input and pre-fill with wlan0 MAC when wlan0 selected.
emhttp/plugins/dynamix/Eth0.page Reformatted PHP functions; changed metric() to remove added offset; refined JS checkbox logic for default gateway based on interface/index.
emhttp/plugins/dynamix/EthX.page Changed gateway checkbox checked state logic to depend on metric values instead of USE_GW flags.
etc/rc.d/rc.docker Removed index() function; simplified route and address selection logic; standardized metric assignment to 0 for shim/vhost interfaces.
etc/rc.d/rc.inet1 Reduced ipv6_ra(), ipv6_conf(), ipv6_addr() arguments; reverted static IP metric assignment to fixed value; simplified function calls.
etc/rc.d/rc.wireless Moved metric option to end of ip commands for address and route additions; no logic changes.
sbin/create_network_ini Removed USE_GW4/USE_GW6 output lines; replaced single-line conditionals with explicit blocks adding static IPs and default gateways; formatting improvements.
sbin/monitor_interface Replaced operstate with carrier-based stable state detection; removed switch function; added wlan presence check; refined IP and route management based on carrier state.

Possibly related PRs

Suggested labels

7.2

Poem

🐇 A hop, a skip through code anew,
Files deleted when their time is due.
Networks clearer, no hides in sight,
MACs aligned, and metrics right.
Routes and states now dance with grace,
The bunny’s code finds its place! 🐾✨

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@bergware bergware changed the title Encryption: fix regression error - keyfile not deleted Miscellaneous fixes and updates Jun 7, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
sbin/create_network_ini (1)

226-226: Remove redundant DEV assignment.

The DEV variable is already assigned on line 203. This redundant assignment should be removed.

-      DEV=$IFACE.${VLANID[$i,$j]}
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (2)

44-45: Add MAC address format validation for robustness.

The code correctly checks for file existence before reading, but doesn't validate the MAC address format. Consider adding validation to ensure the read content is a valid MAC address format.

 // get MAC address of wireless interface (if existing)
-$mac = file_exists('/sys/class/net/wlan0/address') ? trim(file_get_contents('/sys/class/net/wlan0/address')) : '';
+$mac = '';
+if (file_exists('/sys/class/net/wlan0/address')) {
+    $mac = trim(file_get_contents('/sys/class/net/wlan0/address'));
+    // Validate MAC address format (XX:XX:XX:XX:XX:XX)
+    if (!preg_match('/^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$/', $mac)) {
+        $mac = '';
+    }
+}

2029-2041: Escape MAC address when embedding in JavaScript to prevent potential XSS.

While MAC addresses typically have a safe format, it's best practice to escape data when embedding it directly in JavaScript to prevent any potential security issues.

 function updateMAC(index, port) {
-	var wlan0 = '<?=$mac?>'; // mac address of wlan0
+	var wlan0 = '<?=htmlspecialchars($mac, ENT_QUOTES, 'UTF-8')?>'; // mac address of wlan0
 	var mac = $('input[name="nic['+index+'][mac]"');
 	mac.prop('disabled', port=='wlan0');
 	$('i.mac_generate.'+index).prop('disabled', port=='wlan0');
 	$('span.wlan0').removeClass('hidden');
 	if (port == 'wlan0') {
 		mac.val(wlan0);
 	} else {
 		$('span.wlan0').addClass('hidden');
 		if (wlan0 && mac.val()==wlan0) $('i.mac_generate.'+index).click();
 	}
 }
emhttp/plugins/dynamix/Eth0.page (1)

5-5: Use standard PHP tags for consistency

Consider using standard lowercase <?php tags instead of uppercase <?PHP and short tags <?. Short tags may not be enabled on all PHP installations, and lowercase is the conventional style.

-<?PHP
+<?php
-<?
+<?php

Also applies to: 17-17

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 846dca9 and 194f972.

📒 Files selected for processing (10)
  • emhttp/plugins/dynamix.docker.manager/DockerSettings.page (10 hunks)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2 hunks)
  • emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (2 hunks)
  • emhttp/plugins/dynamix/Eth0.page (8 hunks)
  • emhttp/plugins/dynamix/EthX.page (4 hunks)
  • etc/rc.d/rc.docker (4 hunks)
  • etc/rc.d/rc.inet1 (5 hunks)
  • etc/rc.d/rc.wireless (2 hunks)
  • sbin/create_network_ini (4 hunks)
  • sbin/monitor_interface (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • etc/rc.d/rc.wireless
🧰 Additional context used
🧬 Code Graph Analysis (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
emhttp/plugins/dynamix/include/Browse.php (1)
  • write (21-24)
🔇 Additional comments (31)
emhttp/plugins/dynamix/EthX.page (5)

176-176: Logic change looks correct for IPv4 gateway checkbox.

The checkbox state is now determined by the metric value instead of the USE_GW4 variable. When METRIC:0 is 0, the checkbox is unchecked; otherwise, it's checked.


205-205: Logic change looks correct for IPv6 gateway checkbox.

Consistent with the IPv4 change - checkbox state is now determined by METRIC6:0 value instead of USE_GW6 variable.


285-285: Logic change looks correct for VLAN IPv4 gateway checkbox.

The pattern is consistently applied to VLAN interfaces using METRIC:$i instead of USE_GW4:$i.


314-314: Logic change looks correct for VLAN IPv6 gateway checkbox.

The pattern is consistently applied to VLAN interfaces using METRIC6:$i instead of USE_GW6:$i.


1-434: Verify consistency with PR objectives.

The file changes appear to be network gateway configuration logic, but the PR title mentions "Encryption: fix regression error - keyfile not deleted". Please verify that this file is part of the correct PR.

Likely an incorrect or invalid review comment.

sbin/create_network_ini (6)

164-167: Good refactor to explicit if-block and added gateway route logging.

The change from a single-line conditional to an explicit if-block improves readability. Adding the gateway route information to statics.ini provides better network state tracking.


188-191: Consistent IPv6 implementation matching IPv4 changes.

The IPv6 static assignment follows the same pattern as IPv4, maintaining consistency in the codebase.


198-198: Minor formatting improvement.

Adding space after semicolon in for-loop header improves code style consistency.


220-223: Good VLAN IPv4 handling with interface suffix cleanup.

The change properly handles VLAN static assignments and strips the "@$IFACE" suffix before writing to statics.ini, which is the correct approach.


245-248: Consistent VLAN IPv6 implementation.

The IPv6 VLAN handling matches the IPv4 pattern, maintaining consistency and proper interface suffix handling.


1-304: Verify file relevance to encryption PR.

This script appears to handle network configuration, but the PR title indicates it's about encryption and keyfile deletion. Please confirm this file is part of the correct changeset.

Likely an incorrect or invalid review comment.

etc/rc.d/rc.docker (4)

344-344: Verify the IPv4 subnet selection logic change

The subnet selection logic has been changed from filtering routes by a specific IP address to selecting the first non-default route. This is a more general approach but may behave differently when multiple subnets exist on the interface.

Please confirm this change is intentional and won't cause issues with multi-subnet configurations.


353-353: Appropriate IPv6 address filtering

The change to exclude IPv6 addresses starting with "f[a-f]" (Unique Local Addresses) is correct for selecting globally routable addresses for Docker network configuration.


355-355: Consistent IPv6 subnet filtering

The subnet selection logic correctly excludes both default routes and ULA prefixes (f[a-f]), maintaining consistency with the IPv6 address filtering approach.


425-425: Metric value standardized to 0

The metric value has been standardized to 0 across all IP address operations on shim and vhost interfaces. This ensures these addresses have the highest routing priority, which is appropriate for Docker network interfaces.

Also applies to: 445-445, 449-449

emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)

2029-2041: LGTM! Well-implemented wlan0 MAC address handling.

The logic correctly:

  • Disables MAC input and generate button when wlan0 is selected
  • Sets the MAC value to the host's actual wlan0 MAC address
  • Triggers new MAC generation when switching away from wlan0
  • Properly shows/hides the wlan0 warning message

This implementation ensures VMs using the wireless interface have the correct MAC address while preventing user modification.

etc/rc.d/rc.inet1 (4)

329-332: Good fix for removing duplicate parameter

The change correctly removes the duplicate parameter assignment by using $2 instead of $3 for the autoconf setting. This aligns with the PR objective of removing duplicate IPv6 parameters.


335-338: Correct update to match ipv6_ra signature

The function calls have been properly updated to pass only 2 arguments to ipv6_ra, matching the simplified function signature.


341-351: Consistent parameter reduction in IPv6 configuration

All function calls have been correctly updated to match the simplified signatures, removing redundant parameters throughout the IPv6 configuration flow.


373-373: Correct simplification of ipv6_addr call

The function call has been properly updated to pass only the required argument.

emhttp/plugins/dynamix/Eth0.page (4)

40-56: LGTM - Compact function formatting

The functions have been reformatted to be more compact without changing their logic.


58-65: Important: Metric calculation simplified

The removal of the 1000 offset from the metric calculation aligns with the changes in rc.inet1. This will result in lower metric values (e.g., 1-10 instead of 1001-1010).

Ensure this change is compatible with any existing configurations that may have hardcoded expectations about metric values being in the 1000+ range.


213-213: Good refinement of gateway checkbox logic

The change correctly limits the forced gateway checkbox to only the primary interface (eth0 index 0), not VLAN interfaces. This provides better control over gateway configuration.

Also applies to: 223-223


829-829: Clean approach using metric value for gateway state

The change from checking USE_GW4/USE_GW6 variables to using the metric value (0 = disabled, >0 = enabled) is a cleaner approach that reduces configuration complexity.

Also applies to: 860-860

emhttp/plugins/dynamix.docker.manager/DockerSettings.page (3)

44-44: Correct conditional logic for ethernet interfaces

The added condition ensures ethernet interfaces are only added to custom networks when bridge mode is enabled, preventing incorrect network configurations.


59-64: Excellent improvements to network data extraction

The enhanced ip commands now:

  • Only retrieve primary global addresses (avoiding secondary IPs)
  • Exclude deprecated IPv6 addresses
  • Better filter routes by excluding IPv6-specific prefixes (fa-ff range)

These changes will provide more accurate network configuration data.


429-429: Good simplification of visibility logic

Removing the hide_wlan and hide_eth functions in favor of inline conditions makes the code more direct and easier to understand.

Also applies to: 485-485

sbin/monitor_interface (4)

25-28: LGTM!

Clean implementation for reading carrier state with appropriate error handling.


42-51: Good simplification of the initialization logic.

The use of a single TASK array with proper line reading is cleaner than the previous approach.


86-87: Smart optimization for state checking.

Good use of LAST variable to avoid redundant state checks for the same interface.


21-24:

Details

❓ Verification inconclusive

Verify wireless interface naming assumption.

The function assumes the wireless interface is always named wlan0. Modern Linux systems may use predictable network interface names (e.g., wlp2s0).


🏁 Script executed:

#!/bin/bash
# Description: Check for wireless interfaces with different naming schemes

# List all network interfaces that might be wireless
fd -t d . /sys/class/net -x bash -c 'if [[ -d {}/wireless ]]; then echo "Wireless interface found: $(basename {})"; fi'

# Alternative: Use iw to list wireless interfaces if available
which iw >/dev/null 2>&1 && iw dev 2>/dev/null | grep -E "^\s+Interface" | awk '{print $2}'

Length of output: 234


Verify wireless interface naming assumption

  • File: sbin/monitor_interface
  • Function: wlan() (lines 21–24)

The function currently hard-codes wlan0. On modern Linux systems you’ll often see predictable names like wlp2s0, wlx…, etc. Please confirm that wlan0 exists on your target hosts or refactor wlan() to detect the wireless interface dynamically—for example, by scanning /sys/class/net/*/wireless or using iw dev.

Comment on lines +1248 to 1249
exec("ls --indicator-style=none /sys/class/net | grep -Po '^virbr[0-9]+'",$arrBridges);
exec("ls --indicator-style=none /sys/class/net | grep -Po '^(br|bond|eth|wlan)[0-9]+(\.[0-9]+)?'",$arrBridges);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove redundant exec command

The first exec command on line 1248 that searches for virbr interfaces is immediately overwritten by the second command on line 1249. This redundant operation serves no purpose and should be removed.

-exec("ls --indicator-style=none /sys/class/net | grep -Po '^virbr[0-9]+'",$arrBridges);
exec("ls --indicator-style=none /sys/class/net | grep -Po '^(br|bond|eth|wlan)[0-9]+(\.[0-9]+)?'",$arrBridges);
📝 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.

Suggested change
exec("ls --indicator-style=none /sys/class/net | grep -Po '^virbr[0-9]+'",$arrBridges);
exec("ls --indicator-style=none /sys/class/net | grep -Po '^(br|bond|eth|wlan)[0-9]+(\.[0-9]+)?'",$arrBridges);
exec("ls --indicator-style=none /sys/class/net | grep -Po '^(br|bond|eth|wlan)[0-9]+(\.[0-9]+)?'",$arrBridges);
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php at lines
1248-1249, remove the first exec command that searches for 'virbr' interfaces
because it is immediately overwritten by the second exec command. Keep only the
second exec command that searches for 'br', 'bond', 'eth', and 'wlan' interfaces
to avoid redundant operations.

Comment thread etc/rc.d/rc.inet1
if [[ -n $ADDR ]]; then
[[ $j -eq 0 ]] && MASK=${NETMASK[$i]} || MASK=${NETMASK[$i,$j]}
[[ -n $MASK ]] && run ip -4 addr add $(unzero $ADDR)/$MASK metric $INDEX dev $IFACE
[[ -n $MASK ]] && run ip -4 addr add $(unzero $ADDR)/$MASK dev $IFACE metric 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify the impact of reverting to static metric value

The metric assignment has been reverted from dynamic calculation to a fixed value of 1 for both IPv4 and IPv6 static addresses. While this simplifies the logic, it may affect multi-interface configurations where different metrics were previously used for route prioritization.

Please verify that this change doesn't negatively impact systems with multiple network interfaces that rely on different metric values for proper routing.

Also applies to: 412-412


🏁 Script executed:

#!/bin/bash
# Check if there are any existing configurations that might be affected by metric changes
fd "network.ini" --type f --exec grep -l "METRIC" {} \;

# Look for any documentation or comments about metric usage
rg -A 3 -B 3 "metric.*interface.*priority|route.*metric.*multiple" --type sh

Length of output: 161


🏁 Script executed:

#!/bin/bash
# Inspect METRIC usage in the network.ini generator
rg -n 'METRIC' -C 5 sbin/create_network_ini
rg -ni 'metric' -C 5 sbin/create_network_ini

Length of output: 9283


🏁 Script executed:

#!/bin/bash
# Locate rc.inet1 and display lines 380–430 for context
FILE=$(fd rc.inet1 --type f)
sed -n '380,430p' "$FILE"

Length of output: 218


🏁 Script executed:

#!/bin/bash
# Correctly locate and display lines 380–430 of rc.inet1
fd 'rc.inet1$' --type f | while read file; do
  echo "===== $file ====="
  sed -n '380,430p' "$file"
done

Length of output: 2248


Static metrics now override configured values
The static‐address branches in etc/rc.d/rc.inet1 have been changed to hard-code metric 1, ignoring any per-interface METRIC settings generated in create_network_ini. This will prevent users from assigning non-default priorities on multiple interfaces.

Please update the two ip addr add invocations to use the configured arrays instead of “1”:

• etc/rc.d/rc.inet1, around line 398:
Replace

  [[ -n $MASK ]] && run ip -4 addr add $(unzero $ADDR)/$MASK dev $IFACE metric 1

With

  [[ -n $MASK ]] && run ip -4 addr add $(unzero $ADDR)/$MASK dev $IFACE metric ${METRIC[$i]}

• etc/rc.d/rc.inet1, around line 404:
Replace

  [[ -n $MASK6 ]] && run ip -6 addr add $(unzero6 $ADDR6)/$MASK6 dev $IFACE metric 1

With

  [[ -n $MASK6 ]] && run ip -6 addr add $(unzero6 $ADDR6)/$MASK6 dev $IFACE metric ${METRIC6[$i]}
🤖 Prompt for AI Agents
In etc/rc.d/rc.inet1 around lines 398 and 404-405, the ip addr add commands for
IPv4 and IPv6 static addresses currently use a fixed metric value of 1, which
overrides any configured per-interface METRIC settings. To fix this, replace the
hardcoded metric 1 with the appropriate configured metric arrays: use
${METRIC[$i]} for the IPv4 command near line 398 and ${METRIC6[$i]} for the IPv6
command near line 404-405, ensuring the metric values reflect the intended
interface priorities.

Comment on lines 26 to 30
function strposX($s, $c, $n=1) {
$p = 0;
while ($n && $p=strpos($s,$c,$p)!==false) {$n--; $p+=strlen($c);}
while ($n && $p = strpos($s,$c,$p) !== false) {$n--; $p += strlen($c);}
return $p;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical bug fix in strposX function

Good catch! The assignment within the while loop condition was broken due to operator precedence. The parentheses ensure the assignment happens before the comparison.

🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.docker.manager/DockerSettings.page around lines 26 to
30, the strposX function has a critical bug due to operator precedence in the
while loop condition. Fix this by adding parentheses around the assignment
expression inside the while condition to ensure the assignment happens before
the comparison, like wrapping the strpos call with parentheses before comparing
to false.

Comment thread sbin/monitor_interface
;;
*)
# IP address not present? create it
[[ "$(ip -br addr show dev $PORT)" =~ "${ADDR[1]}" ]] || ip addr add dev ${TASK[$i]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use word boundaries to prevent partial IP address matching.

The current regex could match partial IP addresses (e.g., "192.168.1.1" would match in "192.168.1.10").

Use word boundary anchors or more precise matching:

-          [[ "$(ip -br addr show dev $PORT)" =~ "${ADDR[1]}" ]] || ip addr add dev ${TASK[$i]}
+          [[ "$(ip -br addr show dev $PORT)" =~ [[:space:]]${ADDR[1]}(/|[[:space:]]) ]] || ip addr add dev ${TASK[$i]}
-        [[ "$(ip -br addr show dev $PORT)" =~ "${ADDR[1]}" ]] && ip addr del dev ${TASK[$i]}
+        [[ "$(ip -br addr show dev $PORT)" =~ [[:space:]]${ADDR[1]}(/|[[:space:]]) ]] && ip addr del dev ${TASK[$i]}

Also applies to: 83-83

🤖 Prompt for AI Agents
In sbin/monitor_interface at lines 77 and 83, the regex used to check IP
addresses can match partial IPs, causing incorrect matches. Modify the regex to
include word boundary anchors or use a more precise pattern that matches the
entire IP address exactly, preventing partial matches like "192.168.1.1"
matching "192.168.1.10".

Comment thread sbin/monitor_interface
Comment on lines +29 to 40
state(){
local n NEW OLD HOLD
OLD=$(carrier $1)
HOLD=4
# new state should hold for at least 4 seconds
for n in {1..4}; do
sleep 1
NEW=$(carrier $1)
[[ $NEW != $OLD ]] && ((HOLD--))
done
[[ $status -eq 0 ]]
[[ $HOLD -eq 0 ]] && echo $NEW || echo $OLD
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inverted stability detection logic.

The current logic decrements HOLD when the state changes, which is opposite to the intended behavior described in the comment. This will report unstable states rather than waiting for stability.

Apply this fix to properly detect stable state:

 state(){
   local n NEW OLD HOLD
   OLD=$(carrier $1)
-  HOLD=4
+  HOLD=0
   # new state should hold for at least 4 seconds
   for n in {1..4}; do
     sleep 1
     NEW=$(carrier $1)
-    [[ $NEW != $OLD ]] && ((HOLD--))
+    [[ $NEW == $OLD ]] && ((HOLD++)) || { OLD=$NEW; HOLD=0; }
   done
-  [[ $HOLD -eq 0 ]] && echo $NEW || echo $OLD
+  echo $OLD
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In sbin/monitor_interface around lines 29 to 40, the logic for detecting stable
state is inverted because HOLD is decremented when the state changes, which
contradicts the comment stating the new state should hold for at least 4
seconds. To fix this, modify the loop so that HOLD is decremented only when the
state remains the same across iterations, ensuring the function waits for a
stable state before echoing the new state; otherwise, it should echo the old
state.

Comment thread sbin/monitor_interface
Comment on lines +64 to +74
case ${ADDR[1]} in
GW4)
# no existing default and new default is defined?
ROUTE=$(ip -4 route show to default dev $PORT)
[[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -4 route add dev ${TASK[$i]/GW4/}
;;
GW6)
# no existing default and new default is defined?
ROUTE=$(ip -6 route show to default dev $PORT)
[[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -6 route add dev ${TASK[$i]/GW6/}
;;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect route add syntax.

The current implementation appears to pass the entire task line with string substitution to ip route add, which would result in invalid syntax.

The route add commands need proper parsing. For example, if the task line is "eth0 GW4 default via 192.168.1.1", the current code would execute:

ip -4 route add dev eth0  default via 192.168.1.1

This is invalid syntax. It should be:

ip -4 route add default via 192.168.1.1 dev eth0

Consider parsing the gateway address properly:

         GW4)
           # no existing default and new default is defined?
           ROUTE=$(ip -4 route show to default dev $PORT)
-          [[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -4 route add dev ${TASK[$i]/GW4/}
+          if [[ -z $ROUTE && "${TASK[$i]}" =~ "default via ([0-9.]+)" ]]; then
+            ip -4 route add default via ${BASH_REMATCH[1]} dev $PORT
+          fi
           ;;
         GW6)
           # no existing default and new default is defined?
           ROUTE=$(ip -6 route show to default dev $PORT)
-          [[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -6 route add dev ${TASK[$i]/GW6/}
+          if [[ -z $ROUTE && "${TASK[$i]}" =~ "default via ([0-9a-fA-F:]+)" ]]; then
+            ip -6 route add default via ${BASH_REMATCH[1]} dev $PORT
+          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.

Suggested change
case ${ADDR[1]} in
GW4)
# no existing default and new default is defined?
ROUTE=$(ip -4 route show to default dev $PORT)
[[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -4 route add dev ${TASK[$i]/GW4/}
;;
GW6)
# no existing default and new default is defined?
ROUTE=$(ip -6 route show to default dev $PORT)
[[ -z $ROUTE && "${TASK[$i]}" =~ "default" ]] && ip -6 route add dev ${TASK[$i]/GW6/}
;;
case ${ADDR[1]} in
GW4)
# no existing default and new default is defined?
ROUTE=$(ip -4 route show to default dev $PORT)
if [[ -z $ROUTE && "${TASK[$i]}" =~ default\ via\ ([0-9.]+) ]]; then
ip -4 route add default via ${BASH_REMATCH[1]} dev $PORT
fi
;;
GW6)
# no existing default and new default is defined?
ROUTE=$(ip -6 route show to default dev $PORT)
if [[ -z $ROUTE && "${TASK[$i]}" =~ default\ via\ ([0-9a-fA-F:]+) ]]; then
ip -6 route add default via ${BASH_REMATCH[1]} dev $PORT
fi
;;
🤖 Prompt for AI Agents
In sbin/monitor_interface around lines 64 to 74, the ip route add commands use
incorrect syntax by appending the entire task string with substitution, causing
invalid commands. To fix this, parse the TASK array element to extract the
interface, destination (default), gateway IP, and other parameters separately,
then construct the ip route add command with the correct order: "ip -4 route add
default via <gateway> dev <interface>" for IPv4 and similarly for IPv6. Replace
the current substitution with explicit parsing and reordering of arguments
before calling ip route add.

@limetech limetech merged commit 32ff144 into unraid:master Jun 8, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants