Feat: 7.3 SR-IOV Support Network Cards #2443
Conversation
WalkthroughAdds SR‑IOV startup wrapper and CLI scripts, PHP helpers and endpoints, frontend controls and CSS, diagnostics log capture, and an rc.local startup entry to configure PF VF counts and per‑VF MAC/VFIO from /boot/config/sriov.cfg and /boot/config/sriovvfs.cfg at boot and via the web UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Boot as System Boot
participant RC as /etc/rc.d/rc.local
participant Wrapper as /usr/local/sbin/sriov
participant CFG1 as /boot/config/sriov.cfg
participant CFG2 as /boot/config/sriovvfs.cfg
participant SetVFs as sbin/sriov-setvfs.sh
participant VFSet as sbin/sriov-vfsettings.sh
participant Sysfs as /sys/bus/pci
Boot->>RC: start
RC->>Wrapper: execute
Wrapper->>Wrapper: check enable/disable file
Wrapper->>Sysfs: validate IOMMU presence
Wrapper->>CFG1: read VFS entries
loop per VFS entry
Wrapper->>SetVFs: invoke (BDF, Vendor:Device, count)
SetVFs->>Sysfs: validate device, write sriov_numvfs
SetVFs-->>Wrapper: return status/log
end
Wrapper->>CFG2: read VF settings
loop per VF setting
Wrapper->>VFSet: invoke (BDF, Vendor:Device, vfio, mac)
VFSet->>Sysfs: resolve PF/VF, unbind, set MAC, rebind/bind VFIO
VFSet-->>Wrapper: return status/log
end
Wrapper->>Wrapper: log completion
sequenceDiagram
participant UI as Browser UI
participant JS as SysDevs.page
participant Backend as apply.sriov-cfg.php
participant Helpers as SriovHelpers.php
participant Shell as sbin/sriov-vfsettings.sh
participant Sysfs as /sys/bus/pci
UI->>JS: user triggers apply
JS->>Backend: POST type=sriov / sriovsettings / inuse
Backend->>Helpers: query topology / VM usage / parse settings
Backend->>Sysfs: write sriov_numvfs (if requested)
Backend->>Shell: call VF settings scripts as needed
Shell->>Sysfs: perform unbind/set MAC/rebind or bind VFIO
Backend-->>JS: return JSON result
JS->>UI: display result and refresh table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
sbin/sriov (4)
29-29: Quote variable in cat command.The variable should be quoted to handle paths with spaces correctly.
Apply this diff:
-cat $CFG +cat "$CFG"
32-35: IOMMU check is correct but could be more robust.The check correctly verifies IOMMU availability. However, consider adding error suppression for the ls command to avoid potential error messages.
Optional improvement:
-if [[ ! "$(ls -A /sys/kernel/iommu_groups/)" ]]; then +if [[ ! "$(ls -A /sys/kernel/iommu_groups/ 2>/dev/null)" ]]; then
54-54: Replace deprecated egrep with grep -E.The
egrepcommand is deprecated and should be replaced withgrep -E.Apply this diff:
-ls -l /sys/bus/pci/devices/*/virtfn*| egrep [[:xdigit:]]{4}: +ls -l /sys/bus/pci/devices/*/virtfn* | grep -E '[[:xdigit:]]{4}:'Note: Also added quotes around the regex pattern and spacing around the pipe.
66-69: IOMMU check is duplicated.This IOMMU verification is identical to lines 32-35. Since both VFS and VFMAC processing require IOMMU, and VFS processing always runs first (the script exits if VFS config is missing), this second check is redundant.
Consider removing this duplicate check:
-if [[ ! "$(ls -A /sys/kernel/iommu_groups/ 2>/dev/null)" ]]; then - echo "Error: IOMMU not available" - exit 1 -fi -However, if you want to support scenarios where only VFMAC config exists without VFS config, keep this check but update the earlier exit condition on line 60.
sbin/sriov-setvfs.sh (1)
129-133: Success message could handle optional VD gracefully.The success message includes
${VD}which will be empty if Vendor:Device wasn't provided, resulting in "Device at BDF...".Consider:
-echo "Device ${VD} at ${BDF} set numvfs to ${NUMVFS}" +if [[ -n "$VD" ]]; then + echo "Device ${VD} at ${BDF} set numvfs to ${NUMVFS}" +else + echo "Device at ${BDF} set numvfs to ${NUMVFS}" +fisbin/sriov-setmac.sh (2)
146-155: PF interface determination works but could be simplified.The script tries two methods to find the PF interface name. Line 149 uses
ls | grepwhich is flagged by shellcheck and is generally discouraged.Consider simplifying to:
# Determine PF interface name -PF_IFACE=$(basename "$(readlink -f "$PF_PATH/net"/* 2>/dev/null)") -if [ -z "$PF_IFACE" ]; then - PF_IFACE=$(basename "$(ls -d /sys/class/net/*/device 2>/dev/null | grep "$PF_PATH" | head -n1 | cut -d/ -f5)") -fi +if [ -d "$PF_PATH/net" ]; then + PF_IFACE=$(basename "$PF_PATH/net"/*) +else + for netdev in /sys/class/net/*/device; do + if [ "$(readlink -f "$netdev")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$netdev")") + break + fi + done +fiThis avoids the problematic
ls | greppattern and is more readable.
193-199: Consider error handling for driver rebind.The driver rebind operation could fail, but there's no error checking. While less critical than MAC setting, a failed rebind could leave the VF in an inconsistent state.
Consider:
# Rebind VF to driver if it was bound before if [ -n "$VF_DRIVER" ]; then echo "Rebinding VF to driver $VF_DRIVER..." - echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" + if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" 2>/dev/null; then + echo "Warning: Failed to rebind VF to driver $VF_DRIVER" >&2 + fi fi echo "Done."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sbin/sriov(1 hunks)sbin/sriov-setmac.sh(1 hunks)sbin/sriov-setvfs.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
sbin/sriov-setmac.sh
[error] 56-56: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 149-149: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
sbin/sriov-setvfs.sh
[error] 56-56: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
🔇 Additional comments (15)
sbin/sriov (4)
18-22: LGTM!The SRIOV_ENABLED check correctly gates the feature and exits gracefully when disabled.
46-51: VFS processing loop looks correct.The loop correctly splits parameters by pipe delimiter and invokes the helper script with proper quoting.
60-79: MAC configuration processing looks good.The VFMAC parsing and processing follows the same pattern as VFS, with proper parameter splitting and script invocation. The approach here is cleaner than the VFS section (no redundant parsing).
81-82: LGTM!Clear completion message.
sbin/sriov-setvfs.sh (6)
1-46: LGTM! Well-documented script with proper licensing.The MIT license header and documentation are clear and appropriate.
51-54: LGTM! Root privilege check is correct.Proper validation that the script runs with root privileges.
61-67: Parameter count validation is correct.The check ensures exactly 3 parameters are provided with clear error messaging.
69-88: LGTM! Argument parsing logic is well-structured.The regex-based parsing correctly handles BDF with/without domain, Vendor:Device, and the VF count parameter. The warning for missing PCI domain is helpful.
97-100: LGTM! IOMMU validation is appropriate.Proper check for IOMMU availability with helpful error message.
102-112: Vendor:Device validation looks good.The optional validation using lspci is well-implemented with clear messages for both success and missing vendor/device cases.
sbin/sriov-setmac.sh (5)
69-88: LGTM! Argument parsing adapted correctly for MAC addresses.The parsing logic properly handles the MAC address as the third parameter instead of NUMVFS.
97-112: Device validation is correct.IOMMU and optional Vendor:Device checks are properly implemented.
137-144: LGTM! PF location logic is correct.Properly uses readlink to find the Physical Function for the given Virtual Function.
158-172: VF index determination logic is correct.The loop properly finds the VF index by comparing PCI addresses. Good defensive check for glob expansion on line 161.
180-187: LGTM! Driver unbind logic is correct.Properly checks if VF has a driver and unbinds it before MAC configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
sbin/sriov (2)
2-2: Fix typo in comment.The comment contains "modified fir SRIOV" which should be "modified for SRIOV".
Apply this diff:
-# limetech - wrapper for Andre Ritchter's vfio-bind script modified fir SRIOV +# limetech - wrapper for Andre Ritchter's vfio-bind script modified for SRIOV
37-44: Simplify config parsing - redundant logic detected.The script parses the VFS configuration twice. Lines 37-40 parse with fromdos/declare but the result is never used. Line 44 re-parses with grep/cut/tr, and this is what's actually used in the loop. The first parsing approach has issues: using
<<<with a while loop only processes the last line, and the parsed$VFSvariable is checked but never used.Remove the redundant parsing:
-while IFS='=' read -r name value -do - [[ ! -z "$value" ]] && declare "${name}"="${value//\"}" -done <<< $(/usr/bin/fromdos < $CFG) -[[ -z "$VFS" ]] && exit - -# Read the line properly (don't let bash squash the spaces) VFS_LINE=$(grep "^VFS=" "$CFG" | cut -d= -f2- | tr -d '"') +[[ -z "$VFS_LINE" ]] && exitsbin/sriov-setvfs.sh (3)
56-59: Fix incorrect empty arguments check.The condition
[[ -z "$@" ]]doesn't work correctly with arrays. It will always succeed even when arguments are provided, causing the script to exit incorrectly.Apply this diff:
-if [[ -z "$@" ]]; then +if [[ $# -eq 0 ]]; then echo "Error: Please provide Domain:Bus:Device.Function (dddd:bb:dd.f) and/or Vendor:Device (vvvv:dddd)" 1>&2 exit 1 fi
69-95: Add validation that BDF was successfully parsed.If no valid BDF pattern matches in the argument parsing loop (lines 70-88), the
$BDFvariable will be unset when reaching line 90, causing an invalid sysfs path.Add validation after the parsing loop:
fi done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
116-127: Add validation for NUMVFS parameter.The script doesn't validate that
$NUMVFSwas set during argument parsing or that it's a valid non-negative integer. If the third parameter is missing or invalid,$NUMVFSwill be unset or invalid, causing an incorrect write to sysfs.Add validation before line 116:
+if [[ -z "$NUMVFS" ]]; then + echo "Error: No VF count provided" 1>&2 + exit 1 +fi + +if ! [[ "$NUMVFS" =~ ^[0-9]+$ ]]; then + echo "Error: VF count must be a non-negative integer" 1>&2 + exit 1 +fi + printf "\nSetting...\n"sbin/sriov-setmac.sh (7)
35-44: Fix documentation to reflect MAC address parameter.The documentation incorrectly describes the third parameter. Line 38 says "Number of VFs" when it should be "MAC Address", and line 44's comment is incomplete, saying "Set nu to value..." instead of describing MAC address setting.
Apply this diff:
# This script takes three parameters: # <Domain:Bus:Device.Function> i.e. dddd:bb:dd.f # <Vendor:Device> i.e. vvvv:dddd -# <Number of VFs +# <MAC Address> i.e. HH:HH:HH:HH:HH:HH # and then: # # (1) If both <Vendor:Device> and <Domain:Bus:Device.Function> were provided, # validate that the requested <Vendor:Device> exists at <Domain:Bus:Device.Function> # -# (2) Set nu to value supplied as 3rd parameter: +# (2) Set MAC address to value supplied as 3rd parameter: #
56-59: Fix incorrect empty arguments check.Same issue as in sriov-setvfs.sh:
[[ -z "$@" ]]doesn't work correctly with arrays.Apply this diff:
-if [[ -z "$@" ]]; then +if [[ $# -eq 0 ]]; then echo "Error: Please provide Domain:Bus:Device.Function (dddd:bb:dd.f) and/or Vendor:Device (vvvv:dddd)" 1>&2 exit 1 fi
61-67: Fix incorrect usage example.The usage example shows "numvfs" but this script expects a MAC address as the third parameter.
Apply this diff:
echo "Usage: $0 <param1> <param2> <param3>" 1>&2 - echo "Example: $0 0000:01:00.0 10de:1fb8 numvfs" 1>&2 + echo "Example: $0 0000:01:00.0 10de:1fb8 00:11:22:33:44:55" 1>&2 exit 1 fi
69-95: Add validation that BDF was successfully parsed.Same issue as in sriov-setvfs.sh:
$BDFmight be unset if no valid BDF pattern matched during argument parsing.Add validation after the parsing loop:
fi done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
118-133: Remove dead code block.This entire commented-out section is for setting
sriov_numvfs, which is not the purpose of this script. This code appears to be copied from sriov-setvfs.sh and serves no purpose here. Removing it will improve clarity.Apply this diff:
printf "\nSetting...\n" -: ' -# Capture stderr output from echo into a variable -error_msg=$( (echo "$NUMVFS" > "$TARGET_DEV_SYSFS_PATH/sriov_numvfs") 2>&1 ) - -if [[ $? -ne 0 ]]; then - echo "Error: Failed to set sriov_numvfs at $TARGET_DEV_SYSFS_PATH" >&2 - clean_msg=$(echo "$error_msg" | sed -n 's/.*error: \(.*\)/\1/p') - echo "System message: $clean_msg" >&2 - exit 1 -fi - -printf "\n" - - -echo "Device ${VD} at ${BDF} set numvfs to ${NUMVFS}" -' - # Locate PF device
174-178: Add MAC address validation before use.The script doesn't validate that
$MACwas set during argument parsing, nor does it validate the MAC address format before attempting to apply it.Add validation before line 174:
+if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +# Validate MAC address format (colon or hyphen separated) +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then + echo "Error: Invalid MAC address format. Expected format: XX:XX:XX:XX:XX:XX or XX-XX-XX-XX-XX-XX" 1>&2 + exit 1 +fi + echo "Setting MAC for VF:"
189-191: Add error handling for MAC address setting.The
ip link setcommand can fail, but there's no error checking. This could result in silent failures when setting the MAC address.Apply this diff:
# Set MAC #echo "$PF_IFACE" vf "$VF_INDEX" mac "$MAC" -ip link set "$PF_IFACE" vf "$VF_INDEX" mac "$MAC" +if ! ip link set "$PF_IFACE" vf "$VF_INDEX" mac "$MAC"; then + echo "Error: Failed to set MAC address $MAC on VF $VF_INDEX" >&2 + exit 1 +fi
🧹 Nitpick comments (7)
sbin/sriov (4)
66-69: Remove duplicate IOMMU check.The IOMMU availability check is already performed on lines 32-35. Since IOMMU state cannot change during script execution, this second check is redundant.
Apply this diff:
[[ ! -f "$CFG_VFS" ]] && exit grep -q "^VFMAC=" "$CFG_VFS" || exit echo "Loading MAC config from $CFG_VFS" cat "$CFG_VFS" echo "---" -if [[ ! "$(ls -A /sys/kernel/iommu_groups/ 2>/dev/null)" ]]; then - echo "Error: IOMMU not available" - exit 1 -fi - # Fix the VFMAC parsing
46-51: Add validation for helper script existence and error handling.The script invokes
/usr/local/sbin/sriov-setvfs.shwithout checking if it exists or handling potential failures. This could lead to silent failures during VF configuration.Apply this diff:
+# Validate helper script exists +if [[ ! -x /usr/local/sbin/sriov-setvfs.sh ]]; then + echo "Error: /usr/local/sbin/sriov-setvfs.sh not found or not executable" 1>&2 + exit 1 +fi + for PARAM in $VFS_LINE; do IFS='|' read -r arg1 arg2 arg3 <<< "$PARAM" echo "Processing $arg1 $arg2 set VFs to $arg3" - /usr/local/sbin/sriov-setvfs.sh "$arg1" "$arg2" "$arg3" + if ! /usr/local/sbin/sriov-setvfs.sh "$arg1" "$arg2" "$arg3"; then + echo "Error: Failed to set VFs for $arg1" 1>&2 + exit 1 + fi echo "---" done
54-54: Replace deprecated egrep with grep -E.The
egrepcommand is deprecated. Usegrep -Einstead for better portability and to avoid deprecation warnings.Apply this diff:
echo 'Devices VFs defined:' -ls -l /sys/bus/pci/devices/*/virtfn*| egrep [[:xdigit:]]{4}: +ls -l /sys/bus/pci/devices/*/virtfn* | grep -E '[[:xdigit:]]{4}:'
74-79: Add validation for MAC helper script existence and error handling.Similar to the VFS processing loop, this section invokes
/usr/local/sbin/sriov-setmac.shwithout checking if it exists or handling failures.Apply this diff:
# Fix the VFMAC parsing VFMAC_LINE=$(grep "^VFMAC=" "$CFG_VFS" | cut -d= -f2- | tr -d '"') +# Validate helper script exists +if [[ ! -x /usr/local/sbin/sriov-setmac.sh ]]; then + echo "Error: /usr/local/sbin/sriov-setmac.sh not found or not executable" 1>&2 + exit 1 +fi + for PARAM_VFS in $VFMAC_LINE; do IFS='|' read -r arg1 arg2 arg3 <<< "$PARAM_VFS" echo "Processing $arg1 $arg2 set Mac to $arg3" - /usr/local/sbin/sriov-setmac.sh "$arg1" "$arg2" "$arg3" + if ! /usr/local/sbin/sriov-setmac.sh "$arg1" "$arg2" "$arg3"; then + echo "Error: Failed to set MAC for $arg1" 1>&2 + exit 1 + fi echo "---" donesbin/sriov-setvfs.sh (1)
116-127: Consider validating sriov_numvfs capability before writing.The script writes to
sriov_numvfswithout checking if the file exists. While the error will be caught by the subsequent check on line 122, explicitly validating SR-IOV capability beforehand provides clearer error messages.Add this check before line 116:
+# Validate SR-IOV capability +if [[ ! -f "$TARGET_DEV_SYSFS_PATH/sriov_numvfs" ]]; then + echo "Error: Device at ${BDF} does not support SR-IOV (sriov_numvfs not found)" 1>&2 + exit 1 +fi + printf "\nSetting...\n"sbin/sriov-setmac.sh (2)
184-186: Consider adding error handling for driver unbind.The driver unbind operation has no error checking. While this may succeed in most cases, adding error handling would make failures more visible.
Apply this diff:
# Unbind VF from driver if loaded if [ -n "$VF_DRIVER" ]; then echo "Unbinding VF from driver $VF_DRIVER..." - echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" + if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" 2>/dev/null; then + echo "Warning: Failed to unbind VF from driver $VF_DRIVER" 1>&2 + fi fi
194-196: Consider adding error handling for driver rebind.Similar to the unbind operation, the driver rebind has no error checking. While typically non-critical, error handling would improve observability.
Apply this diff:
# Rebind VF to driver if it was bound before if [ -n "$VF_DRIVER" ]; then echo "Rebinding VF to driver $VF_DRIVER..." - echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" + if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" 2>/dev/null; then + echo "Warning: Failed to rebind VF to driver $VF_DRIVER" 1>&2 + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sbin/sriov(1 hunks)sbin/sriov-setmac.sh(1 hunks)sbin/sriov-setvfs.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
sbin/sriov-setvfs.sh
[error] 56-56: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
sbin/sriov-setmac.sh
[error] 56-56: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 149-149: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
sbin/sriov-setvfs.sh (1)
119-129: Add validation for NUMVFS parameter.The script doesn't validate that
$NUMVFSwas set during argument parsing or that it's a valid non-negative integer. If the third parameter is missing or doesn't match expected patterns,$NUMVFSwill be unset, causing an empty write to sysfs.This matches the previous review comment requesting validation before line 116.
sbin/sriov-setmac.sh (2)
71-92: Add validation that BDF was successfully parsed.Between the argument parsing loop (lines 71-90) and the usage of
$BDFat line 92, there's no validation that$BDFwas actually set. If no valid BDF pattern matches in the arguments,$BDFwill be unset, causing an invalid sysfs path.This matches the previous review comment. Add validation after line 90:
fi done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
71-90: Add MAC address validation before use.The script doesn't validate that
$MACwas set during argument parsing (lines 71-90). If the third parameter doesn't match the PCI ID patterns, it gets assigned to$MAC, but there's no validation that it's a valid MAC address format before using it at line 173.This matches the previous review comment. Add validation after line 90:
fi done +if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +# Validate MAC address format +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then + echo "Error: Invalid MAC address format. Expected format: XX:XX:XX:XX:XX:XX or XX-XX-XX-XX-XX-XX" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
38-38: Remove unused variable with typo.Line 38 defines
$newelement_change = $newelement_check.$numfs;with a typo ($numfsshould be$numvfs). However, this variable is never used anywhere in the code, making it dead code.Apply this diff to remove the unused line:
$new = $old; $numvfs= _var($_POST,'numvfs'); - $newelement_change = $newelement_check.$numfs; $found = false;sbin/sriov-setmac.sh (1)
130-133: Avoidls | greppattern.Line 132 uses
ls -d ... | grepwhich Shellcheck warns against. This pattern can break with non-alphanumeric filenames and can be replaced with glob patterns or a cleaner approach.Apply this diff to use a more robust approach:
# Determine PF interface name PF_IFACE=$(basename "$(readlink -f "$PF_PATH/net"/* 2>/dev/null)") if [ -z "$PF_IFACE" ]; then - PF_IFACE=$(basename "$(ls -d /sys/class/net/*/device 2>/dev/null | grep "$PF_PATH" | head -n1 | cut -d/ -f5)") + for netdev in /sys/class/net/*/device; do + if [ "$(readlink -f "$netdev")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$netdev")") + break + fi + done fiemhttp/plugins/dynamix/include/SysDevs.php (1)
192-212: Consider restructuring VF list data format.The function returns a flat array alternating between "IOMMU group X" strings and PCI addresses (lines 206-207). This structure is confusing and makes it difficult to understand the relationship between groups and devices. It's used with
in_array()checks at lines 352 and 368 to skip VF entries, but the mixed string/PCI format is error-prone.Consider restructuring to return an associative array keyed by IOMMU group or a simple flat array of just VF PCI addresses:
function getVfListByIommuGroup(): array { $vf_list = []; foreach (glob('/sys/bus/pci/devices/*/physfn') as $vf_physfn) { $vf_dir = dirname($vf_physfn); $vf_pci = basename($vf_dir); $vf_list[] = $vf_pci; } sort($vf_list, SORT_NATURAL); return $vf_list; }Then update lines 352 and 368 to check against this cleaner structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/plugins/dynamix/SysDevs.page(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(6 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)sbin/sriov(1 hunks)sbin/sriov-setmac.sh(1 hunks)sbin/sriov-setvfs.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sbin/sriov
🧰 Additional context used
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
🪛 Shellcheck (0.11.0)
sbin/sriov-setmac.sh
[warning] 132-132: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
sbin/sriov-setvfs.sh
[error] 56-56: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
🔇 Additional comments (4)
emhttp/plugins/dynamix/include/SysDevs.php (2)
407-509: XSS protection looks good.The SR-IOV UI rendering code properly escapes user-controlled variables with
htmlentities()andhtmlspecialchars()before outputting to HTML (e.g., lines 436, 437, 503, 504, 505). System-generated data from lspci and sysfs is also handled appropriately.
257-274: Configuration parsing implemented correctly.The SR-IOV configuration file parsing (lines 257-274) properly validates entries against a regex pattern before accepting them, filters out malformed entries, and deduplicates the results. This prevents injection of invalid configuration data.
emhttp/plugins/dynamix/SysDevs.page (2)
50-65: Function logic looks correct.The
saveMACConfigfunction properly retrieves the MAC value, POSTs it to the backend, and handles success/failure cases with appropriate user feedback and reboot notices. The implementation follows established patterns in the codebase.
83-140: MAC generation logic is sound.The
generateMACfunction implements proper validation and MAC address generation:
- PCI ID format validation with regex (line 90)
- User input validation for hex host ID (line 107-109)
- MAC format follows a logical structure:
62:domain_lo:bus:dev:func:host- Uses
CSS.escape()to safely construct the jQuery selector (line 133), preventing CSS injection- Appropriate error handling with user-friendly messages
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
etc/rc.d/rc.S.cont(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
etc/rc.d/rc.S.cont
🔇 Additional comments (1)
etc/rc.d/rc.S.cont (1)
16-18: Code changes approved; SR-IOV script verified.The
/usr/local/sbin/sriovscript exists in the repository atsbin/sriov, is properly executable, and includes appropriate error handling (gracefully exits if config files are missing). The path reference inrc.S.contis correct—scripts stored insbin/are installed to/usr/local/sbin/at runtime, following the same pattern asvfio-pci. The initialization sequence is correct: SR-IOV setup runs before VFIO binding and before udev initialization. Neither the sriov script nor vfio-pci were modified in this PR—only the rc.S.cont call was added.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
etc/rc.d/rc.S.cont (1)
11-11: Fix comment formatting.The comment has a formatting issue: "November 25(SRIOV)" should be "November 2025 (SR-IOV)" for proper year format, spacing, and standard hyphenation.
Apply this diff:
-# Simon Fairweather - modified for Unraid OS, November 25(SRIOV) +# Simon Fairweather - modified for Unraid OS, November 2025 (SR-IOV)
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/SysDevs.php (2)
61-130: Consider checking readlink() return values.The function logic is well-structured and handles SR-IOV enumeration appropriately. However,
readlink()at lines 76 and 78 can returnfalseon failure, which would then be passed tobasename().Consider adding explicit checks:
$driver_link = "$devdir/driver"; if (is_link($driver_link)) { - $driver = basename(readlink($driver_link)); + $driver_path = readlink($driver_link); + $driver = $driver_path !== false ? basename($driver_path) : null; $module_link = "$driver_link/module"; - $module = is_link($module_link) ? basename(readlink($module_link)) : $driver; + if (is_link($module_link)) { + $module_path = readlink($module_link); + $module = $module_path !== false ? basename($module_path) : $driver; + } else { + $module = $driver; + } $vf_param = detectVfParam($driver); }
349-349: Remove commented debug code.The commented
var_dump()should be removed before merging to production.- #echo "<tr><td>";var_dump($sriov, $sriovvfs);echo"</td></tr>";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/include/SysDevs.php(6 hunks)etc/rc.d/rc.S.cont(1 hunks)etc/rc.d/rc.local(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
etc/rc.d/rc.S.cont
📚 Learning: 2025-02-27T21:53:37.589Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: etc/rc.d/rc.rsyslogd:48-50
Timestamp: 2025-02-27T21:53:37.589Z
Learning: For the unraid/webgui project, simple error handling is preferred over extensive error checking in shell scripts. The copy operation for configuration files like in rc.rsyslogd doesn't require additional error logging, permission checks, or return statements.
Applied to files:
etc/rc.d/rc.S.cont
🔇 Additional comments (8)
etc/rc.d/rc.local (1)
17-18: The review comment is based on an incorrect assumption about vfio-pci's capabilities.The code evidence shows that
vfio-pci-bind.sh(line 177) has a comment acknowledging "/dev/vfio not available when called via rc.S." This meansvfio-pcicannot properly function when invoked early inrc.S.cont, regardless of whether SR-IOV VFs exist. Reordering the sriov call to run before vfio-pci will not resolve the underlying issue that/dev/vfiois unavailable during early boot, which is a prerequisite for device binding.The boot sequence concern is actually inverted:
vfio-pcishould likely run later (alongside or aftersriovinrc.local), not earlier. Both scripts have misleading comments claiming "early in startup" execution, but this doesn't align with their actual placement or the code's documented constraints.Likely an incorrect or invalid review comment.
emhttp/plugins/dynamix/include/SysDevs.php (7)
22-22: LGTM: Reasonable restriction on SR-IOV device classes.Limiting SR-IOV handling to network (0x02) and display (0x03) controllers is appropriate since these are the primary use cases.
132-150: LGTM: Well-implemented parameter detection with proper security.The function correctly uses
escapeshellarg()for shell command safety and has reasonable fallback logic for detecting VF parameters.
152-183: LGTM: Robust PCI class detection with proper fallbacks.The function correctly extracts PCI class information with appropriate fallbacks and secure command execution.
186-186: LGTM: SR-IOV data initialization.Appropriately initializes the SR-IOV data structure for use in UI rendering.
214-214: Note: Depends on getVfListByIommuGroup() data structure.This initialization works with the current implementation but will need adjustment if
getVfListByIommuGroup()is refactored to return a properly structured associative array (see previous comment).
253-293: LGTM: Config file parsing with proper validation.The implementation correctly parses SR-IOV configuration files with regex validation and deduplication.
407-442: SR-IOV VF configuration UI looks functional.The parent device SR-IOV controls (VF count selector, save/apply buttons) are generated correctly and use proper escaping for HTML attributes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
373-390: Do not skip SR‑IOV group headers.Line 373 filters out any string that appears in
$sriovvfs, but that array currently contains the same"IOMMU group X"labels generated bygetVfListByIommuGroup(). As a result the header row is skipped,$iommuis never set for the group, and the first PF row reuses a stale or undefined$iommu, breaking the VF UI. Please rethink this filter (e.g. compare only trimmed BDFs after refactoringgetVfListByIommuGroup()) so group headers still execute.
♻️ Duplicate comments (4)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (2)
52-53: Skip inserting “0” VF entries when none exist.Line 52 still appends a new entry even when
numvfsis"0", recreating a...|0record immediately after the delete path removes it. Please carry over the guard that was requested previously so deletion remains consistent.- if (!$found) $newexplode[] = $newelement_check.$numvfs; + if (!$found && $numvfs != "0") $newexplode[] = $newelement_check.$numvfs;
74-75: Skip inserting entries when the MAC is empty.Line 74 still pushes a new record even when
macis empty, so a blank MAC entry reappears right after the delete path removes it. Add the empty-MAC guard that was requested earlier to keep the behavior consistent.- if (!$found) $newexplode[] = $newelement_check.$mac; + if (!$found && $mac != "") $newexplode[] = $newelement_check.$mac;emhttp/plugins/dynamix/include/SysDevs.php (2)
200-220: Return VF groups as described in the docblock.Line 214 still builds a flat numeric array with alternating
"IOMMU group X"labels and PCI IDs. That’s the same structure that was flagged before, and it still doesn’t match the docblock or the consuming code. Worse, those duplicate group strings now cause the laterin_array()filter to skip the group headers entirely, so$iommuis never initialised for SR‑IOV groups. Please convert this to an associative array keyed by group name (as previously suggested) and update the callers accordingly.function getVfListByIommuGroup(): array { - $groups = []; - - foreach (glob('/sys/bus/pci/devices/*/physfn') as $vf_physfn) { - $vf_dir = dirname($vf_physfn); - $vf_pci = basename($vf_dir); - - $iommu_link = "$vf_dir/iommu_group"; - if (is_link($iommu_link)) { - $iommu_group = basename(readlink($iommu_link)); - } else { - $iommu_group = "unknown"; - } - - $groups[] = "IOMMU group " . $iommu_group; - $groups[] = $vf_pci; - } - - ksort($groups, SORT_NATURAL); - return $groups; + $groups = []; + + foreach (glob('/sys/bus/pci/devices/*/physfn') as $vf_physfn) { + $vf_dir = dirname($vf_physfn); + $vf_pci = basename($vf_dir); + + $iommu_link = "$vf_dir/iommu_group"; + $iommu_group = is_link($iommu_link) ? basename(readlink($iommu_link)) : 'unknown'; + $key = "IOMMU group " . $iommu_group; + + if (!isset($groups[$key])) { + $groups[$key] = []; + } + $groups[$key][] = $vf_pci; + } + + ksort($groups, SORT_NATURAL); + return $groups; }
460-523: Stop overwriting the parent PCI address inside the VF loop.Starting at Line 461 the code reassigns
$pciaddressto the VF’s DBDF, so the remainder of the outer loop (USB/SATA rendering, ack tracking, etc.) runs against the last VF instead of the parent device. This is the same shadowing problem noted earlier and it still causes incorrect output. Hold the PF address in its own variable and use a separate$vf_pciaddressinside the loop.- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + $pf_pciaddress = $pciaddress; + foreach ($sriov[$pf_pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; } echo "<tr><td></td><td>"; $outputvfio = $vrfline =[]; - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); $vd = trim(explode(" ", $vrfline[0])[0], "[]"); - exec("lspci -v -s $pciaddress", $outputvfio); + exec("lspci -v -s $vf_pciaddress", $outputvfio); if (preg_grep("/vfio-pci/i", $outputvfio)) { echo "<i class=\"fa fa-circle orb green-orb middle\" title=\"",_('Kernel driver in use: vfio-pci'),"\"></i>"; $isbound = "true"; } echo "</td><td>"; if ((strpos($line, 'Host bridge') === false) && (strpos($line, 'PCI bridge') === false)) { - if (file_exists('/sys/bus/pci/devices/'.$pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; + if (file_exists('/sys/bus/pci/devices/'.$vf_pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; echo "</td><td>"; if (!$removed) { - echo '<input type="checkbox" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; + echo '<input type="checkbox" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; // check config file for two formats: <Domain:Bus:Device.Function>|<Vendor:Device> or just <Domain:Bus:Device.Function> - echo (in_array($pciaddress."|".$vd, $vfio_cfg_devices) || in_array($pciaddress, $vfio_cfg_devices)) ? " checked>" : ">"; + echo (in_array($vf_pciaddress."|".$vd, $vfio_cfg_devices) || in_array($vf_pciaddress, $vfio_cfg_devices)) ? " checked>" : ">"; } } else { echo "</td><td>"; } echo '</td><td title="'; foreach ($outputvfio as $line2) echo htmlentities($line2,ENT_QUOTES)." "; echo '">IOMMU Group '.$vrf['iommu_group'].": ",$vrfline[0],'</td></tr>'; - if (array_key_exists($pciaddress,$pci_device_diffs)) { + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { echo "<tr><td></td><td><td></td><td></td><td>"; - echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($pciaddress)."','".htmlentities($pci_device_diffs[$pciaddress]['status'])."')\"></i>"; + echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($vf_pciaddress)."','".htmlentities($pci_device_diffs[$vf_pciaddress]['status'])."')\"></i>"; echo _("PCI Device change"); - echo " "._("Action").":".ucfirst(_($pci_device_diffs[$pciaddress]['status']))." "; - $ackparm .= $pciaddress.",".$pci_device_diffs[$pciaddress]['status'].";"; - if ($pci_device_diffs[$pciaddress]['status']!="removed") echo $pci_device_diffs[$pciaddress]['device']['description']; + echo " "._("Action").":".ucfirst(_($pci_device_diffs[$vf_pciaddress]['status']))." "; + $ackparm .= $vf_pciaddress.",".$pci_device_diffs[$vf_pciaddress]['status'].";"; + if ($pci_device_diffs[$vf_pciaddress]['status']!="removed") echo $pci_device_diffs[$vf_pciaddress]['device']['description']; echo "</td></tr>"; - if ($pci_device_diffs[$pciaddress]['status']=="changed") { + if ($pci_device_diffs[$vf_pciaddress]['status']=="changed") { echo "<tr><td></td><td><td></td><td></td><td>"; echo _("Differences"); - foreach($pci_device_diffs[$pciaddress]['differences'] as $key => $changes){ + foreach($pci_device_diffs[$vf_pciaddress]['differences'] as $key => $changes){ echo " $key "._("before").":{$changes['old']} "._("after").":{$changes['new']} "; } echo "</td></tr>"; } } - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; + if (isset($sriov_devices_settings[$vf_pciaddress])) { + $mac = $sriov_devices_settings[$vf_pciaddress]['mac']; } else { $mac = null; } $placeholder = empty($mac) ? 'Undefined dynamic allocation' : ''; $value_attr = empty($mac) ? '' : htmlspecialchars($mac, ENT_QUOTES); echo "<tr><td></td><td></td><td></td><td></td><td>"; echo '<label for="mac_address">MAC Address:</label>'; - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; - echo '<a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; - echo '<a class="info" href="#" title="'._("Save MAC config").'" onclick="saveMACConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; + echo '<a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($vf_pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; + echo '<a class="info" href="#" title="'._("Save MAC config").'" onclick="saveMACConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; if ($vrf['driver'] == "vfio-pci") echo _("Current").": "; echo $vrf['driver'] == "vfio-pci" ? _("Bound to VFIO") : strtoupper($vrf['mac']); echo "</td></tr>"; } + $pciaddress = $pf_pciaddress;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/SysDevs.php(6 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)sbin/sriov(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/SysDevs.page
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
sbin/sriov-vfsettings.sh (2)
71-97: Ensure BDF parsing failures abort earlyIf none of the arguments match the PCI regex,
$BDFstays empty. The subsequentTARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"resolves to/sys/bus/pci/devices/, which exists, so the script keeps going and attempts to unbind/rebind a non-existent device. Bail out as soon as parsing fails so we never touch sysfs with an empty identifier. citeturn0done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Domain:Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
157-176: Validate the MAC argument before applying it
$MACis accepted blindly; if parsing never set it, or if it’s malformed,ip link set … mac "$MAC"runs with an empty/invalid string. That silently fails and leaves the VF unconfigured. Add a presence + format check before logging and applying the change. citeturn0-if [[ -z "$MAC" ]]; then - echo "Error: No MAC address provided" 1>&2 - exit 1 -fi - -# Validate MAC address format (various common formats) -if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then - echo "Error: Invalid MAC address format. Expected format: XX:XX:XX:XX:XX:XX or XX-XX-XX-XX-XX-XX" 1>&2 - exit 1 -fi +if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then + echo "Error: Invalid MAC address format. Expected XX:XX:XX:XX:XX:XX" 1>&2 + exit 1 +fiemhttp/plugins/dynamix/include/SriovHelpers.php (1)
176-195: Return VF IOMMU groups as documentedThe docblock promises an associative structure keyed by “IOMMU group …”, but the current implementation pushes alternating labels and PCI IDs into a flat numeric array.
ksort()is then a no-op, and consumers (e.g. SysDevs) can’t reliably correlate VFs by group. Please group the PCI IDs under their IOMMU key as originally intended and then iterate/update the call sites accordingly. citeturn0function getVfListByIommuGroup(): array { - $groups = []; + $groups = []; foreach (glob('/sys/bus/pci/devices/*/physfn') as $vf_physfn) { $vf_dir = dirname($vf_physfn); $vf_pci = basename($vf_dir); $iommu_link = "$vf_dir/iommu_group"; if (is_link($iommu_link)) { $iommu_group = basename(readlink($iommu_link)); } else { $iommu_group = "unknown"; } - $groups[] = "IOMMU group " . $iommu_group; - $groups[] = $vf_pci; + $key = "IOMMU group " . $iommu_group; + if (!isset($groups[$key])) { + $groups[$key] = []; + } + $groups[$key][] = $vf_pci; } ksort($groups, SORT_NATURAL); return $groups; }Update the consuming code (e.g. iterate
$sriovvfsby$group => $vfList) so it reflects the new associative structure. citeturn0emhttp/plugins/dynamix/include/SysDevs.php (1)
229-292: Fix VF loop to avoid clobbering PF state and crashing on missing MACInside the VF loop we overwrite
$pciaddresswith each VF’s BDF. That corrupts the outer PF context (everything after the loop now references the last VF), and it also feeds the VF BDF into later USB/SATA blocks. Additionally, the display line runsstrtoupper($vrf['mac'])unconditionally; VFs without a net interface don’t exposemac, so PHP 8 throws a TypeError and the page dies. Please keep the PF address intact via a new$vf_pciaddressvariable and guard the MAC display so it falls back gracefully when the attribute is absent. citeturn0- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + foreach ($sriov[$pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; } echo "<tr><td></td><td>"; $outputvfio = $vrfline =[]; - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); $vd = trim(explode(" ", $vrfline[0])[0], "[]"); - exec("lspci -v -s $pciaddress", $outputvfio); + exec("lspci -v -s $vf_pciaddress", $outputvfio); if (preg_grep("/vfio-pci/i", $outputvfio)) { echo "<i class=\"fa fa-circle orb green-orb middle\" title=\"",_('Kernel driver in use: vfio-pci'),"\"></i>"; $isbound = "true"; } echo "</td><td>"; if ((strpos($line, 'Host bridge') === false) && (strpos($line, 'PCI bridge') === false)) { - if (file_exists('/sys/bus/pci/devices/'.$pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; + if (file_exists('/sys/bus/pci/devices/'.$vf_pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; echo "</td><td>"; if (!$removed) { - echo '<input type="checkbox" id="vfvfio'.$pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; + echo '<input type="checkbox" id="vfvfio'.$vf_pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; // check config file for two formats: <Domain:Bus:Device.Function>|<Vendor:Device> or just <Domain:Bus:Device.Function> - echo (array_key_exists($pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? " checked>" : ">"; + echo (array_key_exists($vf_pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? " checked>" : ">"; } } else { echo "</td><td>"; } echo '</td><td title="'; foreach ($outputvfio as $line2) echo htmlentities($line2,ENT_QUOTES)." "; - echo '">IOMMU Group '.$vrf['iommu_group'].": ",$vrfline[0],'</td></tr>'; - if (array_key_exists($pciaddress,$pci_device_diffs)) { + echo '">IOMMU Group '.$vrf['iommu_group'].": ",$vrfline[0],'</td></tr>'; + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { echo "<tr><td></td><td><td></td><td></td><td>"; - echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($pciaddress)."','".htmlentities($pci_device_diffs[$pciaddress]['status'])."')\"></i>"; + echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($vf_pciaddress)."','".htmlentities($pci_device_diffs[$vf_pciaddress]['status'])."')\"></i>"; echo _("PCI Device change"); - echo " "._("Action").":".ucfirst(_($pci_device_diffs[$pciaddress]['status']))." "; - $ackparm .= $pciaddress.",".$pci_device_diffs[$pciaddress]['status'].";"; - if ($pci_device_diffs[$pciaddress]['status']!="removed") echo $pci_device_diffs[$pciaddress]['device']['description']; + echo " "._("Action").":".ucfirst(_($pci_device_diffs[$vf_pciaddress]['status']))." "; + $ackparm .= $vf_pciaddress.",".$pci_device_diffs[$vf_pciaddress]['status'].";"; + if ($pci_device_diffs[$vf_pciaddress]['status']!="removed") echo $pci_device_diffs[$vf_pciaddress]['device']['description']; echo "</td></tr>"; - if ($pci_device_diffs[$pciaddress]['status']=="changed") { + if ($pci_device_diffs[$vf_pciaddress]['status']=="changed") { echo "<tr><td></td><td><td></td><td></td><td>"; echo _("Differences"); - foreach($pci_device_diffs[$pciaddress]['differences'] as $key => $changes){ + foreach($pci_device_diffs[$vf_pciaddress]['differences'] as $key => $changes){ echo " $key "._("before").":{$changes['old']} "._("after").":{$changes['new']} "; } echo "</td></tr>"; } } - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; - } else { - $mac = null; - } + $mac = $sriov_devices_settings[$vf_pciaddress]['mac'] ?? null; $placeholder = empty($mac) ? 'Undefined dynamic allocation' : ''; $value_attr = empty($mac) ? '' : htmlspecialchars($mac, ENT_QUOTES); echo "<tr><td></td><td></td><td></td><td></td><td>"; echo '<label for="mac_address">MAC Address:</label>'; - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; - echo '<a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; - echo '<a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; - if ($vrf['driver'] == "vfio-pci") - echo _("Current").": "; - echo $vrf['driver'] == "vfio-pci" ? _("Bound to VFIO") : strtoupper($vrf['mac']); + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; + echo '<a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($vf_pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; + echo '<a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; + echo _("Current").": "; + if ($vrf['driver'] === "vfio-pci") { + echo _("Bound to VFIO"); + } else { + echo $mac ? strtoupper($mac) : _("No MAC reported"); + } echo "</td></tr>"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)sbin/sriov(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sbin/sriov
- emhttp/plugins/dynamix/include/update.sriov-cfg.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.php
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/SysDevs.php (2)
emhttp/plugins/dynamix/include/Helpers.php (1)
comparePCIData(607-653)emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(40-117)getVfListByIommuGroup(176-196)parseVFvalues(201-224)parseVFSettings(229-253)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 132-132: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
sbin/sriov-vfsettings.sh (2)
72-91: BDF validation still missing (duplicate from past review).The script assumes
$BDFis set after the parsing loop but it can be empty if no valid BDF matched. This was flagged in a previous review but remains unaddressed.Add validation after line 91:
fi done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"Based on past review comments.
158-162: MAC validation still missing (duplicate from past review).The script uses
$MACwithout validating it was set or has the correct format. This was flagged in a previous review but remains unaddressed.Add validation before line 158:
+if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +# Validate MAC address format (various common formats) +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then + echo "Error: Invalid MAC address format. Expected format: XX:XX:XX:XX:XX:XX or XX-XX-XX-XX-XX-XX" 1>&2 + exit 1 +fi + echo "Setting MAC for VF:"Based on past review comments.
emhttp/plugins/dynamix/include/SriovHelpers.php (2)
208-231: Return empty array when config file is absent (duplicate from past review).
parseVFvalues()currently returnsnullimplicitly when/boot/config/sriov.cfgdoesn't exist (line 229 returns only inside the if block). Downstream code expects an array and will raise PHP warnings. This was flagged in a previous review but remains unaddressed.Apply this diff to always return an array:
function parseVFvalues() { $sriov_devices = []; $DBDF_SRIOV_REGEX = '/^[[:xdigit:]]{4}:[[:xdigit:]]{2}:[[:xdigit:]]{2}\.[[:xdigit:]]\|[[:xdigit:]]{4}:[[:xdigit:]]{4}\|[[:digit:]]+$/'; - if (is_file("/boot/config/sriov.cfg")) { - $file = trim(file_get_contents("/boot/config/sriov.cfg")); - $file = preg_replace('/^VFS=/', '', $file); // Remove prefix - $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); - - foreach ($entries as $entry) { - if (preg_match($DBDF_SRIOV_REGEX, $entry)) { - // Format: <DBDF>|<Vendor:Device>|<VF_count> - [$dbdf, $ven_dev, $vf_count] = explode('|', $entry); - $sriov_devices[$dbdf] = [ - 'dbdf' => $dbdf, - 'vendor' => $ven_dev, - 'vf_count' => (int)$vf_count, - ]; - } - } - - # $sriov_devices = array_values(array_unique($sriov_devices, SORT_REGULAR)); - return $sriov_devices; + if (!is_file("/boot/config/sriov.cfg")) { + return $sriov_devices; } + + $file = trim(file_get_contents("/boot/config/sriov.cfg")); + $file = preg_replace('/^VFS=/', '', $file); // Remove prefix + $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); + + foreach ($entries as $entry) { + if (preg_match($DBDF_SRIOV_REGEX, $entry)) { + // Format: <DBDF>|<Vendor:Device>|<VF_count> + [$dbdf, $ven_dev, $vf_count] = explode('|', $entry); + $sriov_devices[$dbdf] = [ + 'dbdf' => $dbdf, + 'vendor' => $ven_dev, + 'vf_count' => (int)$vf_count, + ]; + } + } + + return $sriov_devices; }Based on past review comments.
236-260: Return empty array when config file is absent (duplicate from past review).
parseVFSettings()currently returnsnullimplicitly when/boot/config/sriovvfs.cfgdoesn't exist (line 258 returns only inside the if block). Downstream code expects an array and will raise PHP warnings. This was flagged in a previous review but remains unaddressed.Apply this diff to always return an array:
function parseVFSettings() { $sriov_devices_settings = []; $DBDF_SRIOV_SETTINGS_REGEX = '/^[[:xdigit:]]{4}:[[:xdigit:]]{2}:[[:xdigit:]]{2}\.[[:xdigit:]]\|[[:xdigit:]]{4}:[[:xdigit:]]{4}\|[01]\|([[:xdigit:]]{2}:){5}[[:xdigit:]]{2}$/'; - if (is_file("/boot/config/sriovvfs.cfg")) { - $file = trim(file_get_contents("/boot/config/sriovvfs.cfg")); - $file = preg_replace('/^VFSETTINGS=/', '', $file); // Remove prefix - $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); - - foreach ($entries as $entry) { - if (preg_match($DBDF_SRIOV_SETTINGS_REGEX, $entry)) { - // Format: <DBDF>|<Vendor:Device>|<VFIO_flag>|<MAC> - [$dbdf, $ven_dev, $vfio_flag, $mac] = explode('|', $entry); - $sriov_devices_settings[$dbdf] = [ - 'dbdf' => $dbdf, - 'vendor' => $ven_dev, - 'vfio' => (int)$vfio_flag, - 'mac' => strtoupper($mac), - ]; - } - } - - # $sriov_devices_settings = array_values(array_unique($sriov_devices_settings, SORT_REGULAR)); - return $sriov_devices_settings; + if (!is_file("/boot/config/sriovvfs.cfg")) { + return $sriov_devices_settings; } + + $file = trim(file_get_contents("/boot/config/sriovvfs.cfg")); + $file = preg_replace('/^VFSETTINGS=/', '', $file); // Remove prefix + $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); + + foreach ($entries as $entry) { + if (preg_match($DBDF_SRIOV_SETTINGS_REGEX, $entry)) { + // Format: <DBDF>|<Vendor:Device>|<VFIO_flag>|<MAC> + [$dbdf, $ven_dev, $vfio_flag, $mac] = explode('|', $entry); + $sriov_devices_settings[$dbdf] = [ + 'dbdf' => $dbdf, + 'vendor' => $ven_dev, + 'vfio' => (int)$vfio_flag, + 'mac' => strtoupper($mac), + ]; + } + } + + return $sriov_devices_settings; }Based on past review comments.
🧹 Nitpick comments (1)
sbin/sriov-vfsettings.sh (1)
131-134: Avoidls | greppattern per shellcheck SC2010.Line 133 uses
ls -d ... | grepwhich can fail with non-alphanumeric filenames and is flagged by shellcheck.Replace with a safer approach:
# Determine PF interface name PF_IFACE=$(basename "$(readlink -f "$PF_PATH/net"/* 2>/dev/null)") if [ -z "$PF_IFACE" ]; then - PF_IFACE=$(basename "$(ls -d /sys/class/net/*/device 2>/dev/null | grep "$PF_PATH" | head -n1 | cut -d/ -f5)") + for netdev in /sys/class/net/*/device; do + if [ "$(readlink -f "$netdev")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$netdev")") + break + fi + done fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- emhttp/plugins/dynamix/include/apply.sriov-cfg.php
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 133-133: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (11)
sbin/sriov-vfsettings.sh (2)
93-115: LGTM!The device existence and IOMMU checks are appropriate, and the optional vendor:device validation provides good defensive checking.
187-192: LGTM!The optional VFIO binding logic is appropriate and properly logs output and errors.
emhttp/plugins/dynamix/include/SriovHelpers.php (3)
40-124: LGTM!The
getSriovInfoJsonfunction comprehensively enumerates SR-IOV devices with appropriate defensive checks and optional VF detail enrichment.
126-177: LGTM!Both
detectVfParamandgetPciClassNameAndIdhave robust fallback mechanisms and appropriate error handling.
179-203: LGTM!The
getVfListByIommuGroupfunction correctly enumerates VFs and groups them by IOMMU group with appropriate fallback for missing links.emhttp/plugins/dynamix/SysDevs.page (6)
25-48: LGTM!The refinement to check
className.substring(0, 5) === "iommu"appropriately narrows the BIND string inclusion to IOMMU-related checkboxes.
50-66: LGTM!The
saveVFSettingsConfigfunction correctly posts settings, updates the UI with reboot notices, and uses the proper swal() signature.
67-84: LGTM!The
saveVFsConfigfunction correctly posts VF count, updates the UI with reboot notices, and reloads the table when interactive.
86-143: LGTM!The
generateMACfunction properly validates PCI ID format, prompts for host ID with input validation, derives the MAC address from PCI components, and updates the corresponding field.
145-163: LGTM!The
applyVFSettingsfunction correctly saves settings silently, applies them via POST, and updates the UI with appropriate reboot notices.
223-272: LGTM!The
doApplyfunction provides good UX with immediate loading feedback, proper success/error handling, and automatic table reload after completion.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
sbin/sriov-vfsettings.sh (3)
68-68: Fix typo in usage message.Line 68 still contains
echiinstead ofecho. This typo was flagged in a prior review but the fix was not applied.Apply this diff:
- echi "parm3 is binding to VFIO, parm 4 is mac address." + echo "parm3 is binding to VFIO, parm 4 is mac address."
72-93: Add validation that BDF was successfully parsed.The script assumes
$BDFis set after the argument parsing loop, but if no valid BDF pattern matched,$BDFwill be unset. Line 93 then tries to use an unset variable, which could lead to unexpected behavior (e.g., accessing/sys/bus/pci/devices/).Add validation immediately after the parsing loop:
done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
158-177: Add MAC address validation before use.The script echoes and applies
$MACwithout checking whether it was actually set during argument parsing or validating its format. If the MAC argument is missing or malformed, theip link setcommand at line 174 will fail without a clear diagnostic message.Add validation before line 158:
+if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +# Validate MAC address format (colon or hyphen-separated hex pairs) +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$ ]]; then + echo "Error: Invalid MAC address format. Expected format: XX:XX:XX:XX:XX:XX or XX-XX-XX-XX-XX-XX" 1>&2 + exit 1 +fi + echo "Setting MAC for VF:"
🧹 Nitpick comments (1)
sbin/sriov-vfsettings.sh (1)
130-134: Replace ls | grep pattern with shell globbing.Line 133 uses
ls -d ... | grepwhich can fail with non-alphanumeric filenames and is flagged by shellcheck (SC2010).Refactor to use a
forloop to search for the matching device symlink:# Determine PF interface name PF_IFACE=$(basename "$(readlink -f "$PF_PATH/net"/* 2>/dev/null)") if [ -z "$PF_IFACE" ]; then - PF_IFACE=$(basename "$(ls -d /sys/class/net/*/device 2>/dev/null | grep "$PF_PATH" | head -n1 | cut -d/ -f5)") + for net_dir in /sys/class/net/*/device; do + if [ -d "$net_dir" ] && [ "$(readlink -f "$net_dir")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$net_dir")") + break + fi + done fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sbin/sriov-vfsettings.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 133-133: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
144-160: Fix VF filtering so PF rows are not dropped
Line 144 compares the entire$lineagainst$sriovvfs. BecausegetVfListByIommuGroup()currently yields both"IOMMU group N"headers and VF BDFs, the first VF causes the header branch to be skipped, so$iommunever initialises and the PF rows that follow vanish or get attached to the previous group. Build a normalised set of VF PCI IDs once and only skip inside the device branch. For example:-$sriovvfs = getVfListByIommuGroup(); +$sriovvfs = getVfListByIommuGroup(); +$sriovvfSet = []; +foreach ($sriovvfs as $key => $entry) { + $items = is_array($entry) ? $entry : [$entry]; + foreach ($items as $vf_bdf) { + if (preg_match($DBDF_REGEX, $vf_bdf)) { + $sriovvfSet[$vf_bdf] = true; + } elseif (preg_match($BDF_REGEX, $vf_bdf)) { + $sriovvfSet["0000:".$vf_bdf] = true; + } + } +} … - if (in_array($line,$sriovvfs)) continue; if ($line[0]=='I') { … - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $pciaddress)) { $pciaddress = "0000:".$pciaddress; } + if (isset($sriovvfSet[$pciaddress])) continue;This keeps the group header path intact while still suppressing VF rows from the legacy
lspcioutput.
♻️ Duplicate comments (9)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (3)
37-39: Fix the$numfstypo.
$numfsis undefined, which throws PHP notices and produces the wrong pending value. Use the posted$numvfs(or drop the unused assignment entirely) to keep the handler clean.- $newelement_change = $newelement_check.$numfs; + $newelement_change = $newelement_check.$numvfs;
52-53: Don’t reinsert entries when numvfs is 0.After we delete an entry for
numvfs == "0", the fallback append immediately adds it back as"|0", undoing the deletion. Guard the append so zero-count requests actually remove the PF entry.- if (!$found) $newexplode[] = $newelement_check.$numvfs; + if (!$found && $numvfs != "0") $newexplode[] = $newelement_check.$numvfs;
76-77: Skip appending empty MAC entries.When the MAC is blank and VFIO is 0 we delete the record, but the unconditional append adds a new
"|0|00:00:00:00:00:00"entry right back. Only append when we actually have settings to persist.- if (!$found) $newexplode[] = $newelement_check.$vfio."|".$mac; + if (!$found && !($mac == "00:00:00:00:00:00" && $vfio == 0)) { + $newexplode[] = $newelement_check.$vfio."|".$mac; + }sbin/sriov-vfsettings.sh (3)
64-69: Fix the usage handler typo.When argument validation fails,
echiraises “command not found” instead of printing guidance. Useechoso the help text renders correctly.- echo "Usage: $0 <param1> <param2> <param3> <parm4>" 1>&2 - echo "Example: $0 0000:01:00.0 10de:1fb8 1 62:00:01:00:00:99" 1>&2 - echi "parm3 is binding to VFIO, parm 4 is mac address." + echo "Usage: $0 <param1> <param2> <param3> <param4>" 1>&2 + echo "Example: $0 0000:01:00.0 10de:1fb8 1 62:00:01:00:00:99" 1>&2 + echo "param3 controls VFIO binding, param4 is the MAC address." 1>&2
72-99: Validate that BDF parsing succeeded.If no argument matches the PCI regex,
$BDFstays empty and the script starts manipulating/sys/bus/pci/devices/directly. Bail out early with a clear error instead.for arg in "$@"; do ... done +if [[ -z "$BDF" ]]; then + echo "Error: No valid Domain:Bus:Device.Function provided" 1>&2 + exit 1 +fi + TARGET_DEV_SYSFS_PATH="/sys/bus/pci/devices/$BDF"
158-176: Reject missing or malformed MAC input.We reach the
ip link set … mac "$MAC"call even when$MACis empty or garbage, yielding a cryptic failure. Validate presence and format before invoking ip(8).echo " PCI: $VF_PCI" echo " MAC: $MAC" +# Ensure a MAC address was supplied and well-formed +if [[ -z "$MAC" ]]; then + echo "Error: No MAC address provided" 1>&2 + exit 1 +fi + +if ! [[ "$MAC" =~ ^([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}$ ]]; then + echo "Error: Invalid MAC address format ($MAC)" 1>&2 + exit 1 +fi + # Determine current driver for this VF VF_DRIVER=$(basename "$(readlink -f "$VF_PATH/driver" 2>/dev/null)")emhttp/plugins/dynamix/include/SriovHelpers.php (2)
257-276: Return VF lists keyed by IOMMU group, as documented
Line 257 still appends"IOMMU group N"and PCI IDs into a flat numeric array, so the docblock (“associative array with keys like ‘IOMMU group 29’”) is incorrect and downstream checks (e.g. SysDevs.php Line 144) end up skipping the group headers entirely. Please emit a group-indexed structure:function getVfListByIommuGroup(): array { - $groups = []; + $groups = []; foreach (glob('/sys/bus/pci/devices/*/physfn') as $vf_physfn) { … - $groups[] = "IOMMU group " . $iommu_group; - $groups[] = $vf_pci; + $key = "IOMMU group " . $iommu_group; + $groups[$key][] = $vf_pci; } ksort($groups, SORT_NATURAL); return $groups; }Then adjust callers to iterate the per-group arrays (or flatten into a set) before filtering. This mirrors the previous feedback on the same function.
282-334: Always return arrays from parseVFvalues/parseVFSettings
Both functions fall off the end when the config files are absent, so$sriov_devices = parseVFvalues();becomesnull. The first$sriov_devices[$pciaddress](see SysDevs.php Line 204) then triggers “Trying to access array offset on value of type null” under PHP 8. The initial SR‑IOV setup path hits this on every server without pre-existing cfg files. Please default to an empty array:function parseVFvalues() { $sriov_devices = []; $DBDF_SRIOV_REGEX = '...'; - if (is_file("/boot/config/sriov.cfg")) { - … - return $sriov_devices; - } + if (!is_file("/boot/config/sriov.cfg")) { + return $sriov_devices; + } + $file = trim(file_get_contents("/boot/config/sriov.cfg")); + … + return $sriov_devices; } function parseVFSettings() { $sriov_devices_settings = []; $DBDF_SRIOV_SETTINGS_REGEX = '...'; - if (is_file("/boot/config/sriovvfs.cfg")) { - … - return $sriov_devices_settings; - } + if (!is_file("/boot/config/sriovvfs.cfg")) { + return $sriov_devices_settings; + } + $file = trim(file_get_contents("/boot/config/sriovvfs.cfg")); + … + return $sriov_devices_settings; }This prevents fatal warnings on first-run systems.
emhttp/plugins/dynamix/include/SysDevs.php (1)
231-293: Keep the PF PCI ID intact when rendering VFs
Line 231 assigns$pciaddress = $vrf['pci'];and never restores it. After the loop finishes, later logic (USB/SATA discovery, change badges) runs against the last VF instead of the PF. The same block also assumes$sriov_devices_settings[$pciaddress]exists, so fresh VFs trigger “Undefined index” notices. Please introduce a dedicated$vf_pciaddressand fetch settings via a safe default, e.g.:- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + foreach ($sriov[$pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { - $pciaddress = "0000:".$pciaddress; + if (preg_match($BDF_REGEX, $vf_pciaddress)) { + $vf_pciaddress = "0000:".$vf_pciaddress; } echo "<tr><td></td><td>"; $outputvfio = $vrfline =[]; - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); … - echo '<input type="checkbox" id="vfvfio'.$pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; - echo (array_key_exists($pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? " checked>" : ">"; + echo '<input type="checkbox" id="vfvfio'.$vf_pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; + $vf_settings = $sriov_devices_settings[$vf_pciaddress] ?? ['vfio' => 0, 'mac' => ""]; + echo ($vf_settings['vfio'] == 1) ? " checked>" : ">"; … - if (array_key_exists($pciaddress,$pci_device_diffs)) { + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { … - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; - } else { - $mac = null; - } + $mac = $vf_settings['mac'] ?: null; … - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$value_attr\""; + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$value_attr\""; … - echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\');'; - echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($sriov_devices_settings[$pciaddress]['vfio'] == 1 ? 'true':'false').'\',\''.$value_attr.'\');'; - echo " <span id=vfstatus$pciaddress></span>"; + echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\');'; + $vfio_flag = $vf_settings['vfio'] == 1 ? 'true' : 'false'; + echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\','.$vfio_flag.',\''.$value_attr.'\');'; + echo " <span id=vfstatus$vf_pciaddress></span>";This keeps the outer
$pciaddressintact and prevents undefined-index warnings for newly created VFs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)emhttp/plugins/dynamix/include/SysDriversInit.php(1 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)sbin/sriov(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/SysDriversInit.phpemhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/include/SysDriversInit.phpemhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
🧬 Code graph analysis (4)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (2)
emhttp/plugins/dynamix/include/SriovHelpers.php (5)
getSriovInfoJson(60-146)parseVFSettings(310-335)setVfMacAddress(350-446)rebindVfDriver(148-197)is_pci_inuse(484-524)emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
emhttp/plugins/dynamix/include/SysDevs.php (2)
emhttp/plugins/dynamix/include/Helpers.php (1)
comparePCIData(607-653)emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(60-146)getVfListByIommuGroup(257-277)parseVFvalues(282-305)parseVFSettings(310-335)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 133-133: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
sbin/sriov-vfsettings.sh (1)
68-68: Fix the usage typo.Line 68 still calls
echi, so the script prints “echi: command not found” instead of the guidance when the user supplies the wrong argument count. Replace it withechoso the help text actually appears.- echi "parm3 is binding to VFIO, parm 4 is mac address." + echo "parm3 is binding to VFIO, parm 4 is mac address."emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
38-38: Use the defined VF count variable.Line 38 concatenates
$numfs, which is undefined, so PHP emits notices and the replacement string ends up empty. Use$numvfs(the value you just pulled from POST) instead.- $newelement_change = $newelement_check.$numfs; + $newelement_change = $newelement_check.$numvfs;emhttp/plugins/dynamix/api/SysDevs.page (1)
472-483: Only persist config after a successful apply.
saveVFsConfig(pciId, vd, 0);runs regardless of whether the backend reported success, so failed applies still overwrite/boot/config/sriov.cfg. Guard it with theokflag you just computed so you only persist when the kernel change actually succeeded.- const ok = data.success === true || data.success === 1; - - saveVFsConfig(pciId, vd, 0); + const ok = data.success === true || data.success === 1; + if (ok) { + saveVFsConfig(pciId, vd, 0); + }emhttp/plugins/dynamix/SysDevs.page (1)
480-490: Only persist config after a successful apply.
saveVFsConfig(pciId, vd, 0);always runs, even whendata.successis false, so failed applies still overwrite/boot/config/sriov.cfg. Wrap the call in theokguard so you only write the file after the kernel update succeeds.- const ok = data.success === true || data.success === 1; - - saveVFsConfig(pciId, vd, 0); + const ok = data.success === true || data.success === 1; + if (ok) { + saveVFsConfig(pciId, vd, 0); + }emhttp/plugins/dynamix/include/SriovHelpers.php (1)
275-324: Default to empty arrays when config files are absent.Both
parseVFvalues()andparseVFSettings()still implicitly returnnullwhen their respective config files don't exist. This causes PHP warnings (and errors in PHP 8) when callers attempt to use the return value as an array. Always return an empty array to ensure type safety.Apply this diff to fix both functions:
function parseVFvalues() { - $sriov_devices = []; - $DBDF_SRIOV_REGEX = '/^[[:xdigit:]]{4}:[[:xdigit:]]{2}:[[:xdigit:]]{2}\.[[:xdigit:]]\|[[:xdigit:]]{4}:[[:xdigit:]]{4}\|[[:digit:]]+$/'; - if (is_file("/boot/config/sriov.cfg")) { - $file = trim(file_get_contents("/boot/config/sriov.cfg")); - $file = preg_replace('/^VFS=/', '', $file); // Remove prefix - $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); - - foreach ($entries as $entry) { - if (preg_match($DBDF_SRIOV_REGEX, $entry)) { - // Format: <DBDF>|<Vendor:Device>|<VF_count> - [$dbdf, $ven_dev, $vf_count] = explode('|', $entry); - $sriov_devices[$dbdf] = [ - 'dbdf' => $dbdf, - 'vendor' => $ven_dev, - 'vf_count' => (int)$vf_count, - ]; - } - } - } - return $sriov_devices; + $sriov_devices = []; + $DBDF_SRIOV_REGEX = '/^[[:xdigit:]]{4}:[[:xdigit:]]{2}:[[:xdigit:]]{2}\.[[:xdigit:]]\|[[:xdigit:]]{4}:[[:xdigit:]]{4}\|[[:digit:]]+$/'; + if (!is_file("/boot/config/sriov.cfg")) { + return $sriov_devices; + } + + $file = trim(file_get_contents("/boot/config/sriov.cfg")); + $file = preg_replace('/^VFS=/', '', $file); // Remove prefix + $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); + + foreach ($entries as $entry) { + if (preg_match($DBDF_SRIOV_REGEX, $entry)) { + // Format: <DBDF>|<Vendor:Device>|<VF_count> + [$dbdf, $ven_dev, $vf_count] = explode('|', $entry); + $sriov_devices[$dbdf] = [ + 'dbdf' => $dbdf, + 'vendor' => $ven_dev, + 'vf_count' => (int)$vf_count, + ]; + } + } + + return $sriov_devices; } function parseVFSettings() { $sriov_devices_settings = []; $DBDF_SRIOV_SETTINGS_REGEX = '/^[[:xdigit:]]{4}:[[:xdigit:]]{2}:[[:xdigit:]]{2}\.[[:xdigit:]]\|[[:xdigit:]]{4}:[[:xdigit:]]{4}\|[01]\|([[:xdigit:]]{2}:){5}[[:xdigit:]]{2}$/'; - if (is_file("/boot/config/sriovvfs.cfg")) { - $file = trim(file_get_contents("/boot/config/sriovvfs.cfg")); - $file = preg_replace('/^VFSETTINGS=/', '', $file); // Remove prefix - $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); - - foreach ($entries as $entry) { - if (preg_match($DBDF_SRIOV_SETTINGS_REGEX, $entry)) { - // Format: <DBDF>|<Vendor:Device>|<VFIO_flag>|<MAC> - [$dbdf, $ven_dev, $vfio_flag, $mac] = explode('|', $entry); - if ($mac == "00:00:00:00:00:00") $mac = ""; - $sriov_devices_settings[$dbdf] = [ - 'dbdf' => $dbdf, - 'vendor' => $ven_dev, - 'vfio' => (int)$vfio_flag, - 'mac' => strtoupper($mac), - ]; - } - } - } - return $sriov_devices_settings; + if (!is_file("/boot/config/sriovvfs.cfg")) { + return $sriov_devices_settings; + } + + $file = trim(file_get_contents("/boot/config/sriovvfs.cfg")); + $file = preg_replace('/^VFSETTINGS=/', '', $file); // Remove prefix + $entries = preg_split('/\s+/', $file, -1, PREG_SPLIT_NO_EMPTY); + + foreach ($entries as $entry) { + if (preg_match($DBDF_SRIOV_SETTINGS_REGEX, $entry)) { + // Format: <DBDF>|<Vendor:Device>|<VFIO_flag>|<MAC> + [$dbdf, $ven_dev, $vfio_flag, $mac] = explode('|', $entry); + if ($mac == "00:00:00:00:00:00") $mac = ""; + $sriov_devices_settings[$dbdf] = [ + 'dbdf' => $dbdf, + 'vendor' => $ven_dev, + 'vfio' => (int)$vfio_flag, + 'mac' => strtoupper($mac), + ]; + } + } + + return $sriov_devices_settings; }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/SriovHelpers.php (2)
54-140: Consider checking JSON encoding success.The function returns the result of
json_encodewithout verifying success. If encoding fails (e.g., due to malformed UTF-8 in device names), it will returnfalse.Consider adding error handling:
- ksort($results, SORT_NATURAL); - return json_encode($results, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + ksort($results, SORT_NATURAL); + $json = json_encode($results, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + if ($json === false) { + error_log("SR-IOV JSON encoding failed: " . json_last_error_msg()); + return '{}'; + } + return $json;
404-404: Use strict comparison operator.Line 404 uses
==for string comparison. Use===for type-safe comparison to avoid unexpected type coercion.Apply this diff:
- if ($mac=="") $mac="00:00:00:00:00:00"; + if ($mac === "") $mac = "00:00:00:00:00:00";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/api/SysDevs.page(1 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)sbin/sriov(1 hunks)sbin/sriov-setvfs.sh(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sbin/sriov-setvfs.sh
- sbin/sriov
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/SysDevs.pageemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.php
🧬 Code graph analysis (3)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (2)
emhttp/plugins/dynamix/include/SriovHelpers.php (5)
getSriovInfoJson(54-140)parseVFSettings(301-324)setVfMacAddress(339-435)rebindVfDriver(142-190)is_pci_inuse(473-513)emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 138-138: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (3)
emhttp/plugins/dynamix/include/SriovHelpers.php (3)
24-31: LGTM!The function properly escapes shell arguments and handles missing modalias files gracefully by returning an empty array.
142-190: LGTM!The driver rebinding logic is well-structured with proper error handling and informative result arrays. The use of
@suppression is acceptable for sysfs operations where files may not be writable.
193-211: LGTM!The function safely detects VF parameters with proper shell escaping and graceful handling of missing
shell_execfunctionality.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
138-157: Remove line-based$sriovvfsfilter; it breaks IOMMU group handlingAt Line [140] you do:
if (in_array($line,$sriovvfs)) continue;Given
getVfListByIommuGroup()currently returns an array that includes"IOMMU group X"strings as well as VF PCI IDs, this causes:
- IOMMU group headers to be skipped entirely whenever the group contains any VF.
$iommunot being refreshed for such groups, so subsequent devices can be mis-associated with the wrong group.- The actual VF filtering is already correctly done later via
in_array($pciaddress,$sriovvfs)(Line [156]) after the BDF has been normalised.The line-based check is therefore both redundant and harmful.
I recommend simply removing it and relying on the BDF-based filter:
- if (in_array($line,$sriovvfs)) continue;This keeps group headings and
$iommucorrect while still hiding VFs from the generic PCI list.
♻️ Duplicate comments (3)
emhttp/plugins/dynamix/include/SysDevs.php (1)
225-287: Fix VF loop: avoid$pciaddressshadowing and invalid PHP in VFIO flag handlingIn the VF rendering block (Lines [225]-[287]) there are two closely related problems:
Variable shadowing/confusion
- Line [226] reassigns
$pciaddressto the VF PCI ID, shadowing the PF’s address from the outer scope and making the block harder to reason about. This was called out in a previous review as well.- Subsequent uses of
$pciaddressinside the VF loop really refer to the VF, and should use a dedicated$vf_pciaddressvariable.Invalid PHP and unsafe access when computing the current VFIO flag
- Line [284] contains
isset(\$sriov_devices_settings[\$pciaddress])and$sriov_devices_settings[$pciaddress]['vfio']directly inside theechostring. The\$sequences here are not in a string; they produce invalid PHP syntax and will fail at parse time.- The intent is to safely derive a
'true'/'false'string based on existing settings, but this needs to be done in PHP before concatenation, with properisset()checks.A consolidated fix that (a) introduces
$vf_pciaddress, (b) safely determines$current_vfio, and (c) correctly wires the JS call could look like:- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + foreach ($sriov[$pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; } echo "<tr><td></td><td>"; $outputvfio = $vrfline =[]; - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); @@ - $vd = trim(explode(" ", $vrfline[0])[0], "[]"); - exec("lspci -v -s $pciaddress", $outputvfio); + $vd = trim(explode(" ", $vrfline[0])[0], "[]"); + exec("lspci -v -s $vf_pciaddress", $outputvfio); @@ - if (file_exists('/sys/bus/pci/devices/'.$pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; + if (file_exists('/sys/bus/pci/devices/'.$vf_pciaddress.'/reset')) echo "<i class=\"fa fa-retweet grey-orb middle\" title=\"",_('Function Level Reset (FLR) supported'),".\"></i>"; echo "</td><td>"; if (!$removed) { - echo '<input type="checkbox" id="vfvfio'.$pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; + echo '<input type="checkbox" id="vfvfio'.$vf_pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; // check config file for two formats: <Domain:Bus:Device.Function>|<Vendor:Device> or just <Domain:Bus:Device.Function> - echo (array_key_exists($pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? " checked>" : ">"; + echo (array_key_exists($vf_pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? " checked>" : ">"; } @@ - if (array_key_exists($pciaddress,$pci_device_diffs)) { + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { @@ - echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($pciaddress)."','".htmlentities($pci_device_diffs[$pciaddress]['status'])."')\"></i>"; + echo "<i class=\"fa fa-warning fa-fw orange-text\" title=\""._('PCI Change')."\n"._('Click to acknowledge').".\" onclick=\"ackPCI('".htmlentities($vf_pciaddress)."','".htmlentities($pci_device_diffs[$vf_pciaddress]['status'])."')\"></i>"; @@ - $ackparm .= $pciaddress.",".$pci_device_diffs[$pciaddress]['status'].";"; - if ($pci_device_diffs[$pciaddress]['status']!="removed") echo $pci_device_diffs[$pciaddress]['device']['description']; + $ackparm .= $vf_pciaddress.",".$pci_device_diffs[$vf_pciaddress]['status'].";"; + if ($pci_device_diffs[$vf_pciaddress]['status']!="removed") echo $pci_device_diffs[$vf_pciaddress]['device']['description']; @@ - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; + if (isset($sriov_devices_settings[$vf_pciaddress])) { + $mac = $sriov_devices_settings[$vf_pciaddress]['mac']; } else { $mac = null; } @@ - echo "<tr><td></td><td></td><td></td><td></td><td>"; - echo '<label for="mac_address">'._("MAC Address").':</label>'; - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; - echo ' <a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; - echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; - echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities(isset(\$sriov_devices_settings[\$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1 ? 'true':'false').'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; + echo "<tr><td></td><td></td><td></td><td></td><td>"; + echo '<label for="mac_address">'._("MAC Address").':</label>'; + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; + echo ' <a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($vf_pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; + echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; + $current_vfio = (isset($sriov_devices_settings[$vf_pciaddress]) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? 'true' : 'false'; + echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($current_vfio).'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; @@ - echo " <span id=vfstatus$pciaddress></span>"; + echo " <span id=vfstatus$vf_pciaddress></span>";This resolves the parse error, keeps notices away via
isset, and makes the PF/VF distinction clear.emhttp/plugins/dynamix/include/apply.sriov-cfg.php (2)
25-33: Makeacknowledge_PCI()throw structured errors instead of echoing “ERROR”
acknowledge_PCI()currently:
- Echoes
"ERROR"and returns whenloadSavedData()fails (Line [29]), which will corrupt JSON output whenaction_settings()calls it.- Does not check the result of
file_put_contents(), so write failures are silent.Given this helper is only used server‑side (inside
action_settings()), it should signal problems via exceptions that the caller’stry/catchcan convert into a JSON error.Suggested change:
function acknowledge_PCI($pciaddr) { $savedfile = "/boot/config/savedpcidata.json"; $saved = loadSavedData($savedfile); - if (!$saved) {echo "ERROR"; return;}; + if (!$saved) { + throw new RuntimeException(_("No saved PCI data found").": $savedfile"); + } $current = loadCurrentPCIData(); - $saved[$pciaddr] = $current[$pciaddr]; - file_put_contents($savedfile,json_encode($saved,JSON_PRETTY_PRINT)); + if (!isset($current[$pciaddr])) { + throw new RuntimeException(_("PCI address not found in current data").": $pciaddr"); + } + $saved[$pciaddr] = $current[$pciaddr]; + $bytes = file_put_contents($savedfile, json_encode($saved, JSON_PRETTY_PRINT)); + if ($bytes === false) { + throw new RuntimeException(_("Failed to write PCI data").": $savedfile"); + } }
action_settings()is already called under atry { ... } catch (Throwable $e)from the"sriov"case, so these exceptions will surface as structured JSON errors instead of stray text.
159-166: Fix request-parameter guard:_var()makesisset()always trueThe guard:
$pciid = _var($_POST, 'pciid'); $vd = _var($_POST, 'vd'); if (!isset($pciid) || !isset($vd)) { json_response(false, _("Missing PCI ID or virtual device")); exit; }never triggers, because
_var()always defines$pciidand$vd(defaulting to''), soisset()is always true. Also,json_response()already exits, so the explicitexitis redundant.To correctly catch missing/empty parameters and avoid odd errors later (e.g. paths like
/sys/bus/pci/devices//sriov_numvfs), use an emptiness check and drop the extra exit:$type = _var($_POST, 'type'); $pciid = _var($_POST, 'pciid'); $vd = _var($_POST, 'vd'); -if (!isset($pciid) || !isset($vd)) { - json_response(false, _("Missing PCI ID or virtual device")); - exit; -} +if ($pciid === '' || $vd === '') { + json_response(false, _("Missing PCI ID or virtual device")); +}This keeps input validation aligned with
_var()semantics and the JSON helper’s behaviour.
🧹 Nitpick comments (1)
sbin/sriov-setvfs.sh (1)
129-142: sysfs write and error handling look solid.Stderr is properly captured in the subshell (line 132), the exit status is checked (line 134), and system messages are extracted with sed (line 136). Error reporting is helpful.
One minor note: if the sed pattern on line 136 doesn't match the kernel error message format,
clean_msgwill be empty, printing"System message: "with no detail. This is unlikely to break anything, but could be more defensive.If you'd like extra robustness, fall back to the full error message if the pattern doesn't extract cleanly:
error_msg=$( (echo "$NUMVFS" > "$TARGET_DEV_SYSFS_PATH/sriov_numvfs") 2>&1 ) if [[ $? -ne 0 ]]; then echo "Error: Failed to set sriov_numvfs at $TARGET_DEV_SYSFS_PATH" >&2 clean_msg=$(echo "$error_msg" | sed -n 's/.*error: \(.*\)/\1/p') + if [[ -z "$clean_msg" ]]; then + clean_msg="$error_msg" + fi echo "System message: $clean_msg" >&2 exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)sbin/sriov-setvfs.sh(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (3)
emhttp/plugins/dynamix/include/Helpers.php (3)
loadSavedData(575-580)loadCurrentPCIData(583-604)comparePCIData(607-653)emhttp/plugins/dynamix/include/SriovHelpers.php (5)
getSriovInfoJson(54-140)parseVFSettings(303-326)setVfMacAddress(341-430)rebindVfDriver(142-192)is_pci_inuse(469-509)emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/SysDevs.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(54-140)getVfListByIommuGroup(252-272)parseVFvalues(277-298)parseVFSettings(303-326)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 138-138: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
[error] 163-163: Argument to -z is always false due to literal strings.
(SC2157)
[warning] 164-164: This expression is constant. Did you forget the $ on a variable?
(SC2050)
🔇 Additional comments (4)
sbin/sriov-setvfs.sh (3)
56-67: Excellent—all critical parameter validation issues are fixed.The empty-arguments check now correctly uses
[[ $# -eq 0 ]](line 56), exact parameter count is enforced (line 62), and comprehensive error messages guide users through proper usage.
69-88: Argument parsing is robust and handles all variants well.The loop correctly distinguishes full DBDF, short BDF (inferring domain 0000), Vendor:Device, and NUMVFS via regex matching. Unrecognized arguments are flagged with clear errors.
90-127: Validation chain is complete and properly sequenced.BDF (lines 90–93), device existence (lines 97–100), IOMMU presence (lines 102–105), Vendor:Device confirmation (lines 108–117), and NUMVFS range checks (lines 119–127) are all in place with appropriate error messages and exit codes.
emhttp/plugins/dynamix/include/SysDevs.php (1)
42-45: SR-IOV state initialisation looks correctInitialising
$sriov,$sriovvfs,$sriov_devices, and$sriov_devices_settingsvia the helper functions is consistent with SriovHelpers.php and keeps SysDevs focused on rendering. No issues here.
[based_on_learnings]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
emhttp/plugins/dynamix/include/SysDevs.php (3)
195-224: Fix markup issues in SR-IOV PF controls.Two markup issues remain from previous reviews:
- Line 222: Invalid closing tag
<?span>should be</span>.- Line 204: The label's
forattribute correctly targetsvf{$pciaddress}, which matches the select id at line 205, so this is actually correct now.Apply this diff to fix the span closing tag:
- if ($file_numvfs != $num_vfs) echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot"))."<?span>"; + if ($file_numvfs != $num_vfs) { + echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot"))."</span>"; + }
226-231: Fix variable shadowing in VF loop.Line 226 reassigns
$pciaddresswith the VF's PCI address, shadowing the parent PF's address from the outer scope (line 149). This makes the code confusing and error-prone, as subsequent code cannot distinguish between the PF and VF addresses.Apply this diff to prevent shadowing:
foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; }Then update all subsequent references in lines 232-288 to use
$vf_pciaddress.
284-284: CRITICAL: Fix escaped variable names causing JavaScript generation bug.Line 284 contains escaped dollar signs (
\$sriov_devices_settings[\$pciaddress]) inside thehtmlentities()call, which will output literal text strings instead of evaluating the PHP variables. This breaks the intent to safely check for the existence of the array key before accessing it, and will generate invalid JavaScript in theonclickattribute.The escaped backslashes prevent PHP from evaluating the
isset()check and array access. This must be fixed by computing the value outside the echo statement:+ $current_vfio = (isset($sriov_devices_settings[$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? 'true' : 'false'; - echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities(isset(\$sriov_devices_settings[\$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1 ? 'true':'false').'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; + echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($current_vfio).'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> ';This also addresses the unguarded array access issue flagged in previous reviews.
🧹 Nitpick comments (2)
sbin/sriov-vfsettings.sh (2)
35-48: Update documentation to reflect actual parameter count and order.The script header states it takes three parameters but actually requires four (line 64). The documentation omits the VFIO flag (parameter 3) and lists MAC as the third parameter when it's actually the fourth.
Update the header to accurately reflect the parameter order and purpose:
# This script takes three parameters: -# <Domain:Bus:Device.Function> i.e. dddd:bb:dd.f -# <Vendor:Device> i.e. vvvv:dddd -# <MAC Address> -# and then: +# This script takes four parameters: +# <Domain:Bus:Device.Function> i.e. dddd:bb:dd.f +# <Vendor:Device> i.e. vvvv:dddd +# <VFIO binding flag> (0 or 1) +# <MAC Address> i.e. XX:XX:XX:XX:XX:XX +# and then:
136-139: Refactor to avoidls | greppattern (SC2010).Line 138 uses
lspiped togrep, which is unreliable for special filenames. Use a loop or glob-based approach instead.Replace the problematic
ls | greppattern:-PF_IFACE=$(basename "$(ls -d /sys/class/net/*/device 2>/dev/null | grep "$PF_PATH" | head -n1 | cut -d/ -f5)") +# Find PF interface by matching device symlink +for net_dev in /sys/class/net/*/device; do + [ -L "$net_dev" ] || continue + if [ "$(readlink -f "$net_dev")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$net_dev")") + break + fi +done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(54-140)getVfListByIommuGroup(252-272)parseVFvalues(277-298)parseVFSettings(303-326)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 138-138: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (5)
sbin/sriov-vfsettings.sh (2)
72-91: Good argument parsing with flexible type detection.The pattern-matching approach cleanly identifies each parameter type regardless of order. Validation of parsed values (BDF, MAC) is present later in the script.
163-171: MAC validation is correctly implemented.The fix from the previous critical issue review is confirmed: variables are properly expanded without misquoting, and the regex validation correctly matches standard MAC address formats.
Can you confirm that the vfio-pci-bind.sh script exists at
/usr/local/sbin/vfio-pci-bind.shand that its exit code correctly indicates success/failure? This path is hardcoded and called at line 204.emhttp/plugins/dynamix/include/SysDevs.php (3)
17-18: LGTM! Includes are properly structured.The SR-IOV helper and libvirt helper includes are correctly added to support the new SR-IOV functionality.
42-45: SR-IOV data initialization looks correct, but note dependency on helper refactor.The initialization properly calls the helper functions from
SriovHelpers.php. However, as flagged in previous reviews,getVfListByIommuGroup()(line 43) currently returns a flat array with alternating group labels and PCI addresses, which may not match the expected associative structure documented in its function comment. Once that helper is refactored per the outstanding issue, verify the usage at lines 140 and 156 still works correctly with the new structure.
140-140: VF filtering logic will need adjustment after helper refactor.These
in_arraychecks currently work with the flat array returned bygetVfListByIommuGroup(), but once that function is refactored to return an associative array (as documented in outstanding issues), these checks will need to be updated to iterate through the grouped VF arrays. Consider adding a helper function likeisVirtualFunction($pciaddress)to encapsulate this check.Also applies to: 156-156
| # Unbind VF from driver if loaded | ||
| if [ -n "$VF_DRIVER" ]; then | ||
| echo "Unbinding VF from driver $VF_DRIVER..." | ||
| echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" | ||
| fi | ||
|
|
||
| # Set MAC | ||
| if ! ip link set "$PF_IFACE" vf "$VF_INDEX" mac "$MAC"; then | ||
| echo "Error: Failed to set MAC address $MAC on VF $VF_INDEX" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Rebind VF to driver if it was bound before | ||
| if [ -n "$VF_DRIVER" ]; then | ||
| echo "Rebinding VF to driver $VF_DRIVER..." | ||
| echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" | ||
| fi | ||
|
|
||
| echo "MAC Address set" | ||
|
|
||
| if [[ "$VFIO" == "1" ]]; then | ||
| echo "Binding VF to vfio" | ||
| /usr/local/sbin/vfio-pci-bind.sh "$BDF" "$VD" \ | ||
| 1>>/var/log/vfio-pci \ | ||
| 2>>/var/log/vfio-pci-errors | ||
| fi |
There was a problem hiding this comment.
Add error checking for driver unbind, rebind, and VFIO binding operations.
The script performs critical operations without checking for success: unbind (line 185), rebind (line 197), and VFIO binding (line 204). If any of these fail silently, the device could be left in an inconsistent state, yet the script reports success. This is a major reliability issue.
Add error checking to each critical operation:
# Unbind VF from driver if loaded
if [ -n "$VF_DRIVER" ]; then
echo "Unbinding VF from driver $VF_DRIVER..."
- echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind"
+ if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" 2>/dev/null; then
+ echo "Error: Failed to unbind VF from driver $VF_DRIVER" >&2
+ exit 1
+ fi
fi
# Set MAC
if ! ip link set "$PF_IFACE" vf "$VF_INDEX" mac "$MAC"; then
echo "Error: Failed to set MAC address $MAC on VF $VF_INDEX" >&2
exit 1
fi
# Rebind VF to driver if it was bound before
if [ -n "$VF_DRIVER" ]; then
echo "Rebinding VF to driver $VF_DRIVER..."
- echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind"
+ if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" 2>/dev/null; then
+ echo "Error: Failed to rebind VF to driver $VF_DRIVER" >&2
+ exit 1
+ fi
fi
echo "MAC Address set"
if [[ "$VFIO" == "1" ]]; then
echo "Binding VF to vfio"
- /usr/local/sbin/vfio-pci-bind.sh "$BDF" "$VD" \
+ if ! /usr/local/sbin/vfio-pci-bind.sh "$BDF" "$VD" \
1>>/var/log/vfio-pci \
- 2>>/var/log/vfio-pci-errors
+ 2>>/var/log/vfio-pci-errors; then
+ echo "Error: Failed to bind VF to vfio" >&2
+ exit 1
+ fi
fi🤖 Prompt for AI Agents
In sbin/sriov-vfsettings.sh around lines 182 to 207 the script currently writes
to the kernel driver bind/unbind sysfs files and calls the vfio binding helper
without checking results; add explicit error checks after each critical
operation: verify the write to /sys/bus/pci/drivers/$VF_DRIVER/unbind succeeded
and if not print a descriptive error to stderr and exit non‑zero; likewise check
the write to /sys/bus/pci/drivers/$VF_DRIVER/bind and handle failure the same
way; when invoking /usr/local/sbin/vfio-pci-bind.sh check its exit status and on
failure log stderr (and optionally the helper's stdout/stderr) and exit
non‑zero; ensure these checks run only when VF_DRIVER or VFIO are set as before
so the script cannot report success when these operations fail.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/SysDevs.page (2)
33-39: Consider validating MAC address before persisting.Lines 35-38 read the MAC address from the input field and concatenate it into
string2without validating that it's non-empty or properly formatted. If a user submits with an empty or invalid MAC, the config file will contain malformed entries.Add validation before building the config string:
if (element.type === "checkbox" && element.className.substring(0, 7) === "vfiommu") { var pciId = element.id.replace(/^vfvfio/, ""); var mac = document.getElementById("vfmac" + pciId).value; + // Skip entries with empty MAC + if (!mac || mac.trim() === "") continue; var elvfio = document.getElementById("vfvfio" + pciId).checked; if (elvfio == true) var vfio = "1"; else vfio = "0"; string2 = string2 + element.value + "|" + vfio + "|" + mac + " "; }
483-504: Avoid function name shadowing.The nested
formatDetailsfunction defined at line 483 shadows the outerformatDetailshelper defined at line 443. While both format details, they have different signatures (the nested one has anindentparameter) and behaviors (the nested one includes VF-specific logic with checkmarks). This shadowing reduces code clarity and makes debugging harder.Rename the nested function to reflect its specialized purpose:
// Helper: recursively format details to plain text - function formatDetails(obj, indent = 0) { + function formatVFDetails(obj, indent = 0) { const pad = " ".repeat(indent); if (obj == null) return ""; if (typeof obj === "string") return pad + obj; if (typeof obj === "number" || typeof obj === "boolean") return pad + obj; - if (Array.isArray(obj)) return obj.map(x => formatDetails(x, indent)).join("\n"); + if (Array.isArray(obj)) return obj.map(x => formatVFDetails(x, indent)).join("\n"); if (typeof obj === "object") { return Object.entries(obj).map(([key, val]) => { if (typeof val === "object" && val !== null) { // Special case for VF result objects if ("success" in val || "error" in val || "details" in val) { const symbol = val.success ? "✔" : "✖"; const msg = val.error || val.details || ""; return `${pad}${key}\n${pad} ${symbol} ${msg}`; } - return `${pad}${key}:\n${formatDetails(val, indent + 1)}`; + return `${pad}${key}:\n${formatVFDetails(val, indent + 1)}`; } return `${pad}${key}: ${val}`; }).join("\n"); } return pad + String(obj); } // Build message let msg = ""; if (data.error) { msg = _("Error") + ": " + data.error; } else if (data.details) { - msg = formatDetails(data.details); + msg = formatVFDetails(data.details); } else { msg = "Configuration successfully applied."; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/SysDevs.page(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
|
@coderabbitai, please close outstnding issues and recheck. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1782 🏁 Script executed: Length of output: 381 🏁 Script executed: Length of output: 60 @SimonFair, I need to clarify a couple of important points about review comments: 📌 Review Comment ResolutionGitHub review comments cannot be programmatically resolved by bots - only you (the PR author) or repository maintainers can resolve conversation threads through the GitHub UI. These comments remain as a historical record of the review process. Currently, there are 22 unaddressed review comments from my previous reviews still open on this PR. 🔄 Options Moving ForwardI can help you in the following ways: Option 1: Trigger a Full Fresh Review ✅ RecommendedI can perform a complete fresh review of all changed files to verify which issues have been addressed and identify any remaining concerns. This will give you a clean assessment of the current state. Option 2: Manual ResolutionYou can resolve the review comment threads manually through the GitHub UI as you address each issue. The conversations will remain visible but marked as resolved. Option 3: Targeted File ScanIf you've addressed specific issues, I can scan particular files to verify the fixes before doing a full review. Would you like me to trigger a full fresh review now? If so, I'll analyze the current state of all 10 changed files and provide an updated assessment. Let me know your preference, or simply confirm and I'll proceed with the full review! 🧠 Learnings used |
Option 1 please |
|
@coderabbitapi Option 1 full review. |
|
@SimonFair, I'll trigger a complete fresh review of all the changes in this PR right away. This will give you a clean assessment of the current state and identify any remaining issues. The review will cover all 10 changed files:
I'll analyze the current state and provide you with an updated assessment shortly! 🔍 ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
75-75: Allow MAC persistence for host drivers.The condition
&& $vfio != 0prevents storing MAC addresses when the VF uses its host driver. Users should be able to persist custom MACs regardless of the driver binding.- if (!$found && $vfio != 0) $newexplode[] = $newelement_check.$vfio."|".$mac; + if (!$found && $mac != "00:00:00:00:00:00") { + $newexplode[] = $newelement_check.$vfio."|".$mac; + }emhttp/plugins/dynamix/include/SysDevs.php (4)
204-204: Fix label-to-input association.The label's
forattribute uses"vf_select"but the select element has id"vf{$pciaddress}". They need to match for proper accessibility.- echo '<label for="vf_select"> '._('Select number of VFs').': </label>'; + echo '<label for="vf'.$pciaddress.'"> '._('Select number of VFs').': </label>';
222-222: Fix malformed closing span tag.Line 222 has
<?span>which is invalid HTML. It should be</span>.- if ($file_numvfs != $num_vfs) echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot"))."<?span>"; + if ($file_numvfs != $num_vfs) { + echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot"))."</span>"; + }
226-290: Fix variable shadowing in VF rendering loop.Line 226 reassigns
$pciaddressto the VF's address, shadowing the parent PF's address from the outer scope. This makes the code confusing and error-prone.Rename the inner variable:
foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; }Then update all subsequent references in the loop (lines 226-290) to use
$vf_pciaddress.
284-284: Guard array access with isset check.Line 284 accesses
$sriov_devices_settings[$pciaddress]['vfio']without verifying the key exists, which will generate PHP notices when the VF has no saved settings.- echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities(isset(\$sriov_devices_settings[\$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1 ? 'true':'false').'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; + $current_vfio = (isset($sriov_devices_settings[$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? 'true' : 'false'; + echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($current_vfio).'\',\''.$value_attr.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> ';emhttp/plugins/dynamix/include/apply.sriov-cfg.php (3)
163-166: Remove redundant echo and unreachable exit.Since
json_response()already echoes JSON and exits (line 57), theechoat line 164 is redundant and theexitat line 165 is unreachable.if (!isset($pciid) || !isset($vd)) { - json_response(false, _("Missing PCI ID or virtual device")); - exit; + json_response(false, _("Missing PCI ID or virtual device")); }
281-281: Remove redundant echo before json_response.Same issue as line 164—
json_response()handles the output and exit internally.- json_response(false, _("Unknown request type").": $type"); + json_response(false, _("Unknown request type").": $type");Note: The current line already appears correct. If there's an
echobefore this call in the actual code, remove it.
25-33: Improve error handling in acknowledge_PCI.The function uses
echo "ERROR"and returns early (line 29), which is inconsistent with thejson_response()pattern used throughout the file. Additionally, thefile_put_contents()call (line 32) lacks error handling.function acknowledge_PCI($pciaddr) { $savedfile = "/boot/config/savedpcidata.json"; $saved = loadSavedData($savedfile); - if (!$saved) {echo "ERROR"; return;}; + if (!$saved) { + throw new RuntimeException(_("No saved PCI data found")); + } $current = loadCurrentPCIData(); $saved[$pciaddr] = $current[$pciaddr]; - file_put_contents($savedfile,json_encode($saved,JSON_PRETTY_PRINT)); + $bytes = file_put_contents($savedfile, json_encode($saved, JSON_PRETTY_PRINT)); + if ($bytes === false) { + throw new RuntimeException(_("Failed to write PCI data")); + } }sbin/sriov-vfsettings.sh (1)
185-207: Add error checking for critical driver operations.The unbind (line 185), rebind (line 197), and VFIO binding (lines 204-206) operations lack error checks. If these fail silently, the device could be left in an inconsistent state.
Apply error checks to each critical operation:
# Unbind VF from driver if loaded if [ -n "$VF_DRIVER" ]; then echo "Unbinding VF from driver $VF_DRIVER..." - echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" + if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/unbind" 2>/dev/null; then + echo "Error: Failed to unbind VF from driver $VF_DRIVER" >&2 + exit 1 + fi fi # Set MAC if ! ip link set "$PF_IFACE" vf "$VF_INDEX" mac "$MAC"; then echo "Error: Failed to set MAC address $MAC on VF $VF_INDEX" >&2 exit 1 fi # Rebind VF to driver if it was bound before if [ -n "$VF_DRIVER" ]; then echo "Rebinding VF to driver $VF_DRIVER..." - echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" + if ! echo "$VF_PCI" > "/sys/bus/pci/drivers/$VF_DRIVER/bind" 2>/dev/null; then + echo "Error: Failed to rebind VF to driver $VF_DRIVER" >&2 + exit 1 + fi fi echo "MAC Address set" if [[ "$VFIO" == "1" ]]; then echo "Binding VF to vfio" - /usr/local/sbin/vfio-pci-bind.sh "$BDF" "$VD" \ + if ! /usr/local/sbin/vfio-pci-bind.sh "$BDF" "$VD" \ 1>>/var/log/vfio-pci \ - 2>>/var/log/vfio-pci-errors + 2>>/var/log/vfio-pci-errors; then + echo "Error: Failed to bind VF to vfio" >&2 + exit 1 + fi fiemhttp/plugins/dynamix/SysDevs.page (2)
210-216: Missing spaces in string concatenation.The concatenated
messageTextat lines 210-216 lacks spaces around thepluralvariable, causing words to run together. The result would be: "The followingVM iscurrently using this device" instead of "The following VM is currently using this device".Apply this diff to add the required spaces:
const plural = vmCount === 1 ? _("VM is") : _("VMs are"); const messageText = - _("The following") + plural + _("currently using this device") + ":\n\n" + + _("The following ") + plural + _(" currently using this device") + ":\n\n" + vmListText + "\n\n" + _("Please stop") + " " + (vmCount === 1 ? "it" : "them") + _(" before applying changes.");
318-324: Missing spaces in string concatenation.Same issue as lines 210-216: the
messageTextconcatenation lacks spaces around thepluralvariable.Apply this diff:
const plural = vmCount === 1 ? _("VM is") : _("VMs are"); const messageText = - _("The following") + plural + _("currently using this device") + ":\n\n" + + _("The following ") + plural + _(" currently using this device") + ":\n\n" + vmListText + "\n\n" + _("Please stop") + " " + (vmCount === 1 ? "it" : "them") + _(" before applying changes.");
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/scripts/diagnostics (1)
786-786: Fix copy-paste error in comment.The comment on line 786 says "copy sriov log" but this block copies the sriov-errors log. Update it to match the actual operation.
-// copy sriov log +// copy sriov-errors log $srioverrors = "/var/log/sriov-errors";emhttp/plugins/dynamix/include/SriovHelpers.php (1)
252-272: Remove ineffectiveksort()call.The function builds a numerically-indexed array (lines 266-267 use
$groups[]), so theksort()call on line 270 has no effect. Sorting by numeric keys doesn't change the order of an array built with sequential push operations.Apply this diff:
$groups[] = "IOMMU group " . $iommu_group; $groups[] = $vf_pci; } - ksort($groups, SORT_NATURAL); return $groups;If you want to sort the output, consider grouping by IOMMU group first (as suggested in a previous review), or sort the VF PCI addresses within each group.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
emhttp/plugins/dynamix/SysDevs.page(4 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)emhttp/plugins/dynamix/include/apply.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/scripts/diagnostics(1 hunks)etc/rc.d/rc.local(1 hunks)sbin/sriov(1 hunks)sbin/sriov-setvfs.sh(1 hunks)sbin/sriov-vfsettings.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
etc/rc.d/rc.localemhttp/plugins/dynamix/include/update.sriov-cfg.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/update.sriov-cfg.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/SriovHelpers.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 138-138: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (13)
etc/rc.d/rc.local (1)
18-19: LGTM!The SR-IOV invocation is correctly placed in the startup sequence with proper logging redirection to
/var/log/sriovand/var/log/sriov-errors.sbin/sriov (1)
1-75: LGTM!The wrapper script correctly orchestrates SR-IOV VF configuration by reading boot config files and invoking the helper scripts. The previous issues with variable naming and parsing have been resolved.
sbin/sriov-setvfs.sh (1)
1-142: LGTM!The script now includes comprehensive input validation for BDF, Vendor:Device, and VF count parameters. All previously identified issues have been resolved.
emhttp/plugins/dynamix/SysDevs.page (3)
64-102: LGTM: Config save functions are well-structured.Both
saveVFSettingsConfigandsaveVFsConfigproperly handle configuration persistence with optional UI feedback controlled by theinteractiveparameter. The SweetAlert calls use correct object syntax, and the table reload at line 99 ensures the UI stays in sync after VF count changes.
104-182: LGTM: MAC generation logic is sound.The
generateMACfunction properly validates PCI ID format, prompts for a host ID with hex-only validation, derives a deterministic MAC address from the PCI address components, and updates the form field. The custom SweetAlert input handling (lines 127-140) withmaxLength,pattern, and keypress filtering provides a good UX.
348-540: LGTM: Apply functions follow safe patterns.Both
doVFSettingApplyanddoVFApplyproperly:
- Show "Updating..." modal during operation
- Parse JSON responses safely with try-catch (lines 373-383)
- Save config only after confirming success (lines 388, 519)
- Handle nested
detailsstructures via theformatDetailshelper- Reload the UI table after completion
The error handling is thorough and the control flow ensures the boot config stays in sync with actual hardware state.
emhttp/plugins/dynamix/include/SriovHelpers.php (7)
24-31: LGTM: Module detection is properly implemented.The
getModulesFromModaliasfunction correctly queries available kernel modules usingmodprobe -Rwith the device's modalias. The use ofescapeshellargprevents command injection, and graceful handling of missing/unreadable modalias files is appropriate.
54-140: LGTM: Comprehensive SR-IOV device enumeration.The
getSriovInfoJsonfunction thoroughly enumerates SR-IOV capabilities:
- Discovers PF devices via
sriov_totalvfspresence- Detects driver, module, and VF parameter name
- Resolves device class and name via
getPciClassNameAndId- Optionally includes per-VF details (vendor:device ID, interface name, MAC, IOMMU group, driver, available modules)
- Returns well-structured, sorted JSON
The optional
$includeVfDetailsparameter allows callers to control the level of detail, which is good for performance.
142-192: LGTM: VF driver rebinding is properly guarded.The
rebindVfDriverfunction correctly:
- Validates VF is part of a PF (line 147)
- Unbinds current driver with writable check (line 163)
- Loads target driver if needed (line 169)
- Uses
driver_overridemechanism (lines 172-178)- Includes writable check for
drivers_probe(line 175) as recommended in past reviews- Verifies the binding succeeded (lines 182-190)
The multi-step rebind process with verification at each stage is robust.
195-246: LGTM: Detection functions have proper fallback logic.Both helper functions are well-designed:
detectVfParam(195-213): Parsesmodinfooutput and searches for known VF parameter names with a fallback regex patterngetPciClassNameAndId(218-246): Attempts machine-readablelspci -mmfirst, then falls back to regularlspcioutput, and extracts numeric class ID from sysfsThe defensive approach ensures these functions work across different system configurations.
277-326: LGTM: Config parsing functions always return arrays.Both
parseVFvaluesandparseVFSettingshave been corrected (per past reviews) to always return an array, even when config files are absent. The return statements at lines 297 and 325 are outside theis_file()conditionals, ensuring callers can safely iterate without null checks.The conversion of all-zero MAC addresses to empty strings (line 315) is a sensible normalization.
341-430: LGTM: MAC address setting with proper error propagation.The
setVfMacAddressfunction implements a robust multi-step process:
- Validates VF path and finds parent PF (lines 357-369)
- Detects PF interface and VF index (lines 371-386)
- Unbinds current driver with writable check (lines 393-403)
- Sets MAC via
ip linkcommand (lines 406-420)- Rebinds to target driver and propagates errors correctly (lines 423-427)
The error propagation fix from past reviews is confirmed at line 425:
$result['error'] = $result2['error'](not$result['error']).
435-509: LGTM: VM usage detection properly guards libvirt access.The helper functions are well-designed:
getVfIndex(435-446): Cleanly iteratesvirtfn*symlinks to find the VF indexbuild_pci_active_vm_map(448-467): Now includes the libvirt guard ($libvirt_running !== "yes") recommended in past reviews, preventing fatal errors when libvirtd is downis_pci_inuse(469-509): Checks both VF and PF types, handling transitive usage (PF is in use if any of its VFs are assigned to active VMs), and returns consistent JSONThe guard at lines 449-450 ensures
$lvis only accessed when safe.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
137-157: Don’t skip IOMMU headers when filtering SR‑IOV VFs
$sriovvfscurrently contains both"IOMMU group N"labels and VF PCI IDs. The check at Line 140:if (in_array($line,$sriovvfs)) continue;causes header lines for any group that has VFs to be skipped. That leaves
$iommuholding the previous group’s value, so FLR detection, in‑use checks, and group checkbox classes for those devices use the wrong IOMMU group.You only need to suppress the VF device rows, which is already handled by the
$pciaddresscheck at Line 156.Consider removing the header-level filter and tightening the VF filter:
- if (in_array($line,$sriovvfs)) continue; @@ - if ( in_array($pciaddress,$sriovvfs)) continue; + if (in_array($pciaddress, $sriovvfs, true)) continue;This keeps the IOMMU group headers and
$iommuvalue correct while still hiding SR‑IOV VFs from the generic table.
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/include/SysDevs.php (2)
195-224: Fix SR‑IOV PF “Pending action” span markupThe PF-level SR‑IOV UI is almost there, but Line 222 still generates invalid HTML:
if ($file_numvfs != $num_vfs) ... " ... Pending action or reboot"))."<?span>";The tag is
<?spaninstead of</span>, and the conditional has no braces, making the markup brittle.Suggested fix:
- if ($file_numvfs != $num_vfs) echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot"))."<?span>"; + if ($file_numvfs != $num_vfs) { + echo " <span id='vfnotice".$pciaddress."'><i class=\"fa fa-warning fa-fw orange-text\"></i> "._("Pending action or reboot")."</span>"; + }This produces well‑formed HTML and makes the conditional clearer.
225-289: Avoid shadowing PF PCI with VF PCI and align VF IDs/labelsInside the VF loop you overwrite the PF’s
$pciaddresswith the VF PCI:foreach($sriov[$pciaddress]['vfs'] as $vrf) { $pciaddress = $vrf['pci']; ... }This was previously flagged and is still confusing and error‑prone. It also drives all VF HTML IDs/values and config lookups off a variable whose meaning changes mid‑block, which makes future maintenance risky.
Refactor to use a dedicated VF variable everywhere in this block and wire the MAC label to the correct input id. For example:
- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + foreach ($sriov[$pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; @@ - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { @@ - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; @@ - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); @@ - exec("lspci -v -s $pciaddress", $outputvfio); + exec("lspci -v -s $vf_pciaddress", $outputvfio); @@ - if (file_exists('/sys/bus/pci/devices/'.$pciaddress.'/reset')) + if (file_exists('/sys/bus/pci/devices/'.$vf_pciaddress.'/reset')) @@ - echo '<input type="checkbox" id="vfvfio'.$pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; - echo (array_key_exists($pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? " checked>" : ">"; + echo '<input type="checkbox" id="vfvfio'.$vf_pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; + echo (array_key_exists($vf_pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? " checked>" : ">"; @@ - if (array_key_exists($pciaddress,$pci_device_diffs)) { + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { @@ - echo "<i ... onclick=\"ackPCI('".htmlentities($pciaddress)."','".htmlentities($pci_device_diffs[$pciaddress]['status'])."')\"></i>"; + echo "<i ... onclick=\"ackPCI('".htmlentities($vf_pciaddress)."','".htmlentities($pci_device_diffs[$vf_pciaddress]['status'])."')\"></i>"; @@ - $ackparm .= $pciaddress.",".$pci_device_diffs[$pciaddress]['status'].";"; - if ($pci_device_diffs[$pciaddress]['status']!="removed") echo $pci_device_diffs[$pciaddress]['device']['description']; + $ackparm .= $vf_pciaddress.",".$pci_device_diffs[$vf_pciaddress]['status'].";"; + if ($pci_device_diffs[$vf_pciaddress]['status']!="removed") echo $pci_device_diffs[$vf_pciaddress]['device']['description']; @@ - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; + if (isset($sriov_devices_settings[$vf_pciaddress])) { + $mac = $sriov_devices_settings[$vf_pciaddress]['mac']; @@ - echo '<label for="mac_address">'._("MAC Address").':</label>'; - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; - echo ' <a ... onclick="generateMAC(\''.htmlentities($pciaddress).'\'); ...>'; - echo ' <a ... onclick="saveVFSettingsConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\'); ...>'; - echo ' <a ... onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities(isset($sriov_devices_settings[$pciaddress]) && $sriov_devices_settings[$pciaddress]['vfio'] == 1 ? 'true':'false').'\',\''.$value_attr.'\'); ...>'; + echo '<label for="vfmac'.$vf_pciaddress.'">'._("MAC Address").':</label>'; + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$value_attr\" placeholder=\"$placeholder\">"; + echo ' <a ... onclick="generateMAC(\''.htmlentities($vf_pciaddress).'\'); ...>'; + echo ' <a ... onclick="saveVFSettingsConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\'); ...>'; + $current_vfio = (isset($sriov_devices_settings[$vf_pciaddress]) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? "true" : "false"; + echo ' <a ... onclick="applyVFSettings(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($current_vfio).'\',\''.$value_attr.'\'); ...>'; @@ - echo " <span id=vfstatus$pciaddress></span>"; + echo " <span id=vfstatus$vf_pciaddress></span>";This keeps the PF PCI address stable, clarifies intent, ensures VF‑specific config/acknowledge operations use the correct key, and fixes the MAC label’s
forattribute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/SriovHelpers.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/SysDevs.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(54-140)getVfListByIommuGroup(252-272)parseVFvalues(277-298)parseVFSettings(303-326)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
341-435: Initializedetailsarray to prevent fatal error.The initial
$resultstructure (lines 343-355) lacks a'details'key. When MAC setting fails ($ret !== 0at line 420) butrebindVfDriver()succeeds and returns details, line 431 attemptsarray_merge($result['details'], $result2['details'])on an undefined array, causing a TypeError in PHP 8.Apply this diff:
$result = [ 'vf_pci' => $vf_pci, 'mac' => $mac, 'pf_pci' => null, 'pf_iface' => null, 'vf_index' => null, 'driver_before' => null, 'driver_after' => null, 'unbind' => false, 'mac_set' => false, 'rebind' => false, - 'error' => null + 'error' => null, + 'details' => [] ];
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/update.vfio-pci-cfg.php (1)
32-42: Use consistent type comparisons in isValidVF.Lines 38-39 mix string comparison (
$fn === '1') with numeric comparison ($fn > 1). Since$fncomes fromexplode()and is a string, use consistent string comparisons or cast to int at the start for clarity:function isValidVF($fields) { list($fn, $mac) = $fields; + $fn = (int)$fn; $mac = strtolower(trim($mac)); $isZeroMac = ($mac === '00:00:00:00:00:00'); $hasMac = ($mac !== '' && !$isZeroMac); - if ($fn === '1') return true; + if ($fn === 1) return true; if ($fn > 1) return true; - if ($fn === '0') return $hasMac; + if ($fn === 0) return $hasMac; return $hasMac; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)emhttp/plugins/dynamix/include/update.vfio-pci-cfg.php(1 hunks)emhttp/plugins/dynamix/sheets/SysDevs.css(1 hunks)sbin/sriov(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sbin/sriov
- emhttp/plugins/dynamix/include/update.sriov-cfg.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/sheets/SysDevs.cssemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/update.vfio-pci-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
🔇 Additional comments (2)
emhttp/plugins/dynamix/sheets/SysDevs.css (1)
16-24: LGTM!The CSS rule appropriately styles the host ID input in SweetAlert modals with fixed width and centering, supporting the MAC generation flow introduced in this PR.
emhttp/plugins/dynamix/SysDevs.page (1)
18-641: Excellent progress on addressing past issues.All critical and major issues flagged in previous reviews have been resolved in subsequent commits:
- VF settings payload now correctly posted to backend
- SweetAlert parameter formats corrected
- Translation functions properly applied
- String concatenation spacing fixed
- Configuration persistence correctly guarded by apply success
- Error handling improved with proper variable initialization
The SR-IOV VF configuration flow is now robust and well-structured.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/SysDevs.page (2)
308-308: Remove commented code.Line 308 contains commented-out code that should be removed for code cleanliness.
Apply this diff:
const mac = document.getElementById("vfmac" + pciId).value; const vfio = document.getElementById("vfvfio" + pciId).checked; - //vfs = parseInt(vfs, 10);
453-465: Remove unused helper function.The global
formatDetailsfunction defined here is never called. Line 495 defines a localformatDetailsinsidedoVFApplythat shadows this one, anddoVFSettingApply(lines 408-412) manually handles formatting without using either version.Option 1: Remove the unused global function:
- // Helper: safely stringify nested structures - function formatDetails(details) { - if (details == null) return ""; - if (typeof details === "string") return details; - if (Array.isArray(details)) - return details.map(formatDetails).join("\n"); - if (typeof details === "object") - return Object.entries(details) - .map(([k, v]) => `${k}: ${formatDetails(v)}`) - .join("\n\n"); - return String(details); - } -Option 2: Use this global helper consistently in both
doVFSettingApplyanddoVFApplyinstead of redefining it locally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/SysDevs.page(2 hunks)emhttp/plugins/dynamix/include/update.sriov-cfg.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/include/update.sriov-cfg.php
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/SysDevs.page
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
138-146: Don’t skip IOMMU group header lines using$sriovvfsfilterforeach ($groups as $line) { if (!$line) continue; if (in_array($line,$sriovvfs)) continue; if ($line[0]=='I') { …
$sriovvfscontains both"IOMMU group X"strings and VF PCI IDs. Thisin_arraytherefore discards any IOMMU group header whose name appears in$sriovvfs—i.e. every group that has SR‑IOV VFs. PFs in those groups are then rendered under the previous group’s header, and theiommuindex stays stale, so checkboxes and FLR indicators are associated with the wrong group.You only need to skip the VF device lines, which is already handled later via the
$pciaddresscheck.Recommended change: remove the header‑line filter and rely on the PCI‑address filter:
- if (!$line) continue; - if (in_array($line,$sriovvfs)) continue; + if (!$line) continue; if ($line[0]=='I') { …
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
225-297: Avoid$pciaddressshadowing in VF loop and align VF status/JS parametersThis VF rendering block reassigns
$pciaddressto the VF’s PCI ID and then uses it for checkboxes, diff tracking, MAC fields, and theapplyVFSettingscall:foreach($sriov[$pciaddress]['vfs'] as $vrf) { $pciaddress = $vrf['pci']; … if (preg_match($BDF_REGEX, $pciaddress)) { $pciaddress = "0000:".$pciaddress; } … echo '<input … id="vfvfio'.$pciaddress.'" … value="'.$pciaddress."|".$vd.'" '; … if (array_key_exists($pciaddress,$pci_device_diffs)) { … } … echo "<input … name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" …>"; … if (isset($sriov_devices_settings[$pciaddress])) { … } $vfiocheck = $vrf['driver'] == "vfio-pci" ? true:false; echo ' … onclick="applyVFSettings(\''.htmlentities($pciaddress).'\', …, \''.htmlentities($vfiocheck).'\', …)" …'; … if ($filevfio != $vfiocheck || (strtoupper($vrf['mac']) != $mac && $mac != null && $vrf['mac']!=null )) …Issues:
- Reassigning
$pciaddressinside the loop shadows the PF’s PCI address and makes it easy to use the wrong value outside this block or in shared variables like$ackparm. This was already flagged earlier and still applies.applyVFSettingsis passed a raw boolean$vfiocheck, which becomes'1'/''in theonclickattribute, while the PF‑level path constructs explicit'true'/'false'strings for the same parameter. This is inconsistent and likely to break JS that expects'true'/'false'.- The pending‑status check uses
$vrf['mac']without anisset/emptyguard, which can raise notices whenmacis absent.- The MAC label’s
for="mac_address"does not match the VF input id (vfmac…).Suggested consolidated fix (outline):
- foreach($sriov[$pciaddress]['vfs'] as $vrf) { - $pciaddress = $vrf['pci']; + foreach ($sriov[$pciaddress]['vfs'] as $vrf) { + $vf_pciaddress = $vrf['pci']; if ($removed) $line=preg_replace('/R/', '', $line, 1); - if (preg_match($BDF_REGEX, $pciaddress)) { + if (preg_match($BDF_REGEX, $vf_pciaddress)) { // By default lspci does not output the <Domain> when the only domain in the system is 0000. Add it back. - $pciaddress = "0000:".$pciaddress; + $vf_pciaddress = "0000:".$vf_pciaddress; } echo "<tr><td></td><td>"; $outputvfio = $vrfline =[]; - exec('lspci -ns "'.$pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$pciaddress.'"',$vrfline); + exec('lspci -ns "'.$vf_pciaddress.'"|awk \'BEGIN{ORS=" "}{print "["$3"]"}\';lspci -s "'.$vf_pciaddress.'"',$vrfline); @@ - exec("lspci -v -s $pciaddress", $outputvfio); + exec("lspci -v -s $vf_pciaddress", $outputvfio); @@ - if (file_exists('/sys/bus/pci/devices/'.$pciaddress.'/reset')) + if (file_exists('/sys/bus/pci/devices/'.$vf_pciaddress.'/reset')) @@ - echo '<input type="checkbox" id="vfvfio'.$pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$pciaddress."|".$vd.'" '; - echo (array_key_exists($pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$pciaddress]['vfio'] == 1) ? " checked>" : ">"; + echo '<input type="checkbox" id="vfvfio'.$vf_pciaddress.'" class="vfiommu'.$vrf['iommu_group'].'" value="'.$vf_pciaddress."|".$vd.'" '; + echo (array_key_exists($vf_pciaddress,$sriov_devices_settings) && $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1) ? " checked>" : ">"; @@ - if (array_key_exists($pciaddress,$pci_device_diffs)) { + if (array_key_exists($vf_pciaddress,$pci_device_diffs)) { @@ - echo "<i … onclick=\"ackPCI('".htmlentities($pciaddress)."','".htmlentities($pci_device_diffs[$pciaddress]['status'])."')\"></i>"; + echo "<i … onclick=\"ackPCI('".htmlentities($vf_pciaddress)."','".htmlentities($pci_device_diffs[$vf_pciaddress]['status'])."')\"></i>"; @@ - $ackparm .= $pciaddress.",".$pci_device_diffs[$pciaddress]['status'].";"; + $ackparm .= $vf_pciaddress.",".$pci_device_diffs[$vf_pciaddress]['status'].";"; @@ - if ($pci_device_diffs[$pciaddress]['status']!="removed") echo $pci_device_diffs[$pciaddress]['device']['description']; + if ($pci_device_diffs[$vf_pciaddress]['status']!="removed") echo $pci_device_diffs[$vf_pciaddress]['device']['description']; @@ - if ($pci_device_diffs[$pciaddress]['status']=="changed") { + if ($pci_device_diffs[$vf_pciaddress]['status']=="changed") { @@ - foreach($pci_device_diffs[$pciaddress]['differences'] as $key => $changes){ + foreach($pci_device_diffs[$vf_pciaddress]['differences'] as $key => $changes){ @@ - if (isset($sriov_devices_settings[$pciaddress])) { - $mac = $sriov_devices_settings[$pciaddress]['mac']; + if (isset($sriov_devices_settings[$vf_pciaddress])) { + $mac = $sriov_devices_settings[$vf_pciaddress]['mac']; @@ - echo '<label for="mac_address">'._("MAC Address").':</label>'; - echo "<input class='narrow' type=\"text\" name=\"vfmac$pciaddress\" id=\"vfmac$pciaddress\" value=\"$saved_mac\" placeholder=\"$placeholder\">"; - echo ' <a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; - echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; + echo '<label for="vfmac'.$vf_pciaddress.'">'._("MAC Address").':</label>'; + echo "<input class='narrow' type=\"text\" name=\"vfmac$vf_pciaddress\" id=\"vfmac$vf_pciaddress\" value=\"$saved_mac\" placeholder=\"$placeholder\">"; + echo ' <a class="info" href="#" title="'._("Generate MAC").'" onclick="generateMAC(\''.htmlentities($vf_pciaddress).'\'); return false;"><i class="fa fa-refresh mac_generate"> </i></a>'; + echo ' <a class="info" href="#" title="'._("Save MAC config").'" onclick="saveVFSettingsConfig(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\'); return false;"><i class="fa fa-save"> </i></a>'; @@ - if (isset($sriov_devices_settings[$pciaddress])) { - $filevfio = $sriov_devices_settings[$pciaddress]['vfio'] == 1 ? true : false; + if (isset($sriov_devices_settings[$vf_pciaddress])) { + $filevfio = $sriov_devices_settings[$vf_pciaddress]['vfio'] == 1 ? true : false; } else $filevfio = false; - $vfiocheck = $vrf['driver'] == "vfio-pci" ? true:false; - echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($vfiocheck).'\',\''.$current_mac.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; + $vfiocheck = $vrf['driver'] == "vfio-pci" ? 'true' : 'false'; + echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($vf_pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($vfiocheck).'\',\''.$current_mac.'\'); return false;"><i title="Apply now VFIO and MAC Address" class="fa fa-play"></i></a> '; @@ - $vfstatus =""; - if ($filevfio != $vfiocheck || (strtoupper($vrf['mac']) != $mac && $mac != null && $vrf['mac']!=null )) $vfstatus = "<i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot")); - echo " <span id=vfstatus$pciaddress>$vfstatus</span>"; + $vfstatus = ""; + if ($filevfio != ($vfiocheck === 'true') || + (!empty($mac) && !empty($vrf['mac']) && strtoupper($vrf['mac']) != $mac)) { + $vfstatus = "<i class=\"fa fa-warning fa-fw orange-text\"></i> ".sprintf(_("Pending action or reboot")); + } + echo " <span id=\"vfstatus$vf_pciaddress\">$vfstatus</span>";This removes the PF/VF shadowing, keeps all per‑VF state keyed by the VF PCI ID, aligns
applyVFSettings’s third argument with the PF‑side'true'/'false'convention, avoids undefined‑index notices formac, and fixes the MAC label’sfortarget.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/SriovHelpers.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/SysDevs.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (4)
getSriovInfoJson(54-140)getVfListByIommuGroup(252-272)parseVFvalues(277-298)parseVFSettings(303-326)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/SriovHelpers.php (3)
142-203: Fix misleading indentation around driver probe path.Line 183's indentation suggests it's part of the
if (is_writable($drv_override))block on line 181, but because there are no braces, it actually executes unconditionally. While the logic is correct (the variable must be defined before the nextifstatement), the misleading indentation hurts readability.Apply this diff to clarify the control flow:
// Step 3: Override driver binding if (is_writable($drv_override)) @file_put_contents($drv_override, "$new_drv"); - $probe_path = "/sys/bus/pci/drivers_probe"; + $probe_path = "/sys/bus/pci/drivers_probe"; if (is_writable($probe_path)) @file_put_contents($probe_path, $vf);
259-283: Remove no-op ksort on numerically-indexed array.Line 281 calls
ksort($groups, SORT_NATURAL)on a numerically-indexed array (keys 0, 1, 2, ...), which has no effect since numeric keys are already in ascending order. Either remove the ksort call entirely, or if you intend to return a grouped structure in the future, update the implementation to build an associative array keyed by group name.Apply this diff to remove the no-op:
} - ksort($groups, SORT_NATURAL); return $groups; }
311-337: LGTM: Config parsing is safe, with minor type-safety suggestion.The function correctly returns an empty array when the config file is missing. Line 326's special handling of the null MAC address (
00:00:00:00:00:00) is intentional and coordinates withsetVfMacAddress().For stricter type safety, consider using
===instead of==on line 326:- if ($mac == "00:00:00:00:00:00") $mac = ""; + if ($mac === "00:00:00:00:00:00") $mac = "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SriovHelpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (4)
domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
🔇 Additional comments (9)
emhttp/plugins/dynamix/include/SriovHelpers.php (9)
1-20: LGTM: File header and dependencies are well-structured.The includes for libvirt components support the VM state detection functions later in the file. Based on learnings,
$allowedPCIClassis intentionally global for use in SysDevs.php.
21-31: LGTM: Clean helper with appropriate error handling.The function safely queries available modules via modalias and returns an empty array on any error path, making it safe for callers to iterate the result.
33-140: LGTM: Comprehensive SR-IOV device enumeration.This function robustly collects PF and VF metadata with appropriate fallbacks for missing sysfs entries. The optional
$includeVfDetailsparameter provides flexibility for callers that need minimal overhead.
205-224: LGTM: Robust VF parameter detection.The function uses a sensible priority order for known VF parameter names and gracefully falls back to pattern matching or null when no match is found.
226-257: LGTM: Multi-layered fallback for PCI device info.The function tries sysfs first for the class ID, then machine-readable lspci, then human-readable lspci, ensuring maximum compatibility across different system states.
285-309: LGTM: Safely handles missing config file.The function correctly returns an empty array when
/boot/config/sriov.cfgdoesn't exist, preventing null dereference issues in callers. As per past review, this fix has been properly applied.
451-465: LGTM: Simple and effective VF index resolution.The function correctly maps VF PCI addresses back to their index by inspecting the PF's virtfn symlinks. The null return allows callers to detect when a VF/PF relationship cannot be established.
467-486: LGTM: Proper guard for libvirt dependency.The function now correctly checks
$libvirt_runningbefore using the$lvglobal, preventing fatal errors when libvirtd is down. As per past review, this fix has been properly applied.
488-528: LGTM: Clear API endpoint for PCI in-use detection.The function correctly determines VM usage for both VFs (direct check) and PFs (by inspecting child VFs). The JSON response structure is consistent and the explicit exit is appropriate for an endpoint handler.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SriovHelpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)
domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_state_translate(1910-1923)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
287-336: LGTM - Config parsing now safely returns empty arrays.Both
parseVFvalues()andparseVFSettings()correctly return empty arrays when config files are absent, preventing null dereference errors in callers.Minor observation:
parseVFSettings()uses 2-space indentation for the function body (lines 314-315) while the inner block uses 4-space indentation. Consider aligning with the 4-space style used inparseVFvalues()for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/SriovHelpers.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T15:18:55.567Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 2443
File: emhttp/plugins/dynamix/include/SriovHelpers.php:16-16
Timestamp: 2025-11-14T15:18:55.567Z
Learning: In the file emhttp/plugins/dynamix/include/SriovHelpers.php, the global variable $allowedPCIClass is defined and is used in emhttp/plugins/dynamix/include/SysDevs.php which includes SriovHelpers.php. This variable should not be flagged as unused.
Applied to files:
emhttp/plugins/dynamix/include/SriovHelpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (5)
get_domains(1483-1486)domain_get_vm_pciids(2338-2348)get_domain_by_name(1493-1496)domain_get_info(1568-1579)domain_state_translate(1910-1923)
🔇 Additional comments (6)
emhttp/plugins/dynamix/include/SriovHelpers.php (6)
14-19: LGTM on includes and global setup.The
$allowedPCIClassglobal is correctly defined here for use inSysDevs.php(filters to network cards class0x02). Based on learnings, this cross-file usage is intentional.
24-140: Well-structured SR-IOV enumeration functions.Both functions demonstrate good practices:
getModulesFromModalias()properly escapes shell arguments and returns an empty array on failuregetSriovInfoJson()provides comprehensive device enumeration with optional VF detail enrichment, proper fallbacks for missing data, and consistent JSON output
142-203: LGTM - Driver rebinding logic is solid.All previously identified issues have been addressed:
- Error messages are now properly assigned (not lost via
$res + [...])escapeshellarg()protects themodprobecall- Writable checks guard all sysfs writes including
drivers_probe- Clear step-by-step flow with verification
206-257: LGTM on utility functions.Both functions apply defensive patterns:
detectVfParam()guards against missingshell_exec()and escapes the driver argumentgetPciClassNameAndId()properly escapes PCI addresses and provides fallback parsing whenlspci -mmoutput doesn't match
351-448: LGTM - MAC address setting logic is complete.All previously identified issues have been addressed:
'details'is initialized as an empty array (line 365)- MAC regex is properly anchored:
/^([a-fA-F0-9]{2}[:\-]){5}[a-fA-F0-9]{2}$/- Error propagation correctly uses
$result2['error'](line 441)- Success flag check uses
$result['error'] === null(line 444)The conversion of empty MAC to
"00:00:00:00:00:00"for theip link setcommand (line 422) correctly handles dynamic MAC allocation.
487-527: LGTM - API endpoint for PCI usage check.The function correctly:
- Handles both VF (single device) and PF (checks all child VFs) cases
- Deduplicates VM names when multiple VFs from the same PF are in use
- Returns a consistent JSON structure with proper
Content-TypeheaderThe
exitpattern is appropriate for this API endpoint context.
| $vms = $lv->get_domains(); | ||
| foreach ($vms as $vm) { | ||
| $vmpciids = $lv->domain_get_vm_pciids($vm); | ||
| $res = $lv->get_domain_by_name($vm); | ||
| $dom = $lv->domain_get_info($res); | ||
| $state = $lv->domain_state_translate($dom['state']); | ||
| if ($state === 'shutoff') continue; |
There was a problem hiding this comment.
Add null check before accessing domain info.
There's a potential issue if a VM is removed or becomes inaccessible between get_domains() and get_domain_by_name(). If get_domain_by_name() returns false, then domain_get_info() also returns false, causing $dom['state'] access to fail.
Apply this diff for defensive handling:
$vms = $lv->get_domains();
+ if (!$vms) return $pcitovm;
foreach ($vms as $vm) {
$vmpciids = $lv->domain_get_vm_pciids($vm);
$res = $lv->get_domain_by_name($vm);
+ if (!$res) continue;
$dom = $lv->domain_get_info($res);
+ if (!$dom) continue;
$state = $lv->domain_state_translate($dom['state']);
if ($state === 'shutoff') continue;🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/include/SriovHelpers.php around lines 471-477,
get_domain_by_name() may return false if the VM disappears; calling
domain_get_info() and then accessing $dom['state'] can fail. Add a defensive
check: after calling get_domain_by_name($vm) verify the result is truthy (not
false/null) before calling domain_get_info(); if get_domain_by_name() returns
false, skip this VM (continue) or log and continue; also verify
domain_get_info() returned a truthy array before reading ['state'] and skip if
not.
3 participants
Summary by CodeRabbit
New Features
UX Improvements
Diagnostics
✏️ Tip: You can customize this high-level summary in your review settings.