Feature: SRIOV GPU Changes #2512
Conversation
WalkthroughPropagated a new per‑VF Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Page as SysDevs.page (JS)
participant Helper as SriovHelpers
participant Server as apply.sriov-cfg.php
participant Script as sbin/sriov-vfsettings.sh
UI->>Page: user triggers "Apply VF settings" (includes class_id)
Page->>Helper: request VF details (reads per‑VF class_id)
Page->>Server: doVFSettingApply(pciId,vd,vfio,mac,currentvfio,currentmac,class_id,message)
Server->>Script: invoke sriov-vfsettings.sh pciId vd class_id vfio [mac]
Script->>Script: if class_id == 0x02 -> validate MAC, unbind driver, set MAC, rebind
Script->>Script: if vfio requested -> bind to vfio
Script-->>Server: result (success/failure)
Server-->>Page: response
Page->>Helper: saveVFSettingsConfig(..., class_id)
Helper-->>Page: persisted
Page-->>UI: display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-11-14T15:18:55.567ZApplied to files:
📚 Learning: 2025-10-22T17:36:50.995ZApplied to files:
📚 Learning: 2025-06-03T21:27:15.912ZApplied to files:
📚 Learning: 2025-06-21T00:10:40.789ZApplied to files:
📚 Learning: 2025-12-28T15:54:58.673ZApplied to files:
🧬 Code graph analysis (1)emhttp/plugins/dynamix/include/DashboardApps.php (1)
🔇 Additional comments (6)
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 |
🔧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix/include/SriovHelpers.php:
- Around line 84-95: The PF's class variables ($class, $class_id, $name) are
being overwritten by VF loop assignments from getPciClassNameAndId($vf_pci);
rename the VF-specific variables (e.g., $vf_class, $vf_class_id, $vf_name)
inside the VF loop and use those new names when populating $vf_entry['class']
and $vf_entry['class_id'] so the original PF $class/$class_id/$name values (set
earlier) remain unchanged for $results[$pci]; update all references in the VF
loop accordingly (including where you set $vf_entry['class'] and
$vf_entry['class_id']) to prevent shadowing.
In @emhttp/plugins/dynamix/include/SysDevs.php:
- Around line 424-448: The switch on $class_id can leave $mac undefined for
unhandled classes, causing a warning where $mac is later used (e.g., around the
usage near line 460); initialize $mac before the switch (e.g., $mac = null) or
ensure the switch's default case sets $mac = null (and any related variables
like $action/$saveaction) so $mac is always defined regardless of $class_id,
referencing the existing variables $class_id, $vrf, $pciaddress and the switch
block in SysDevs.php.
- Around line 454-458: The echo calling applyVFSettings uses $current_mac even
for non-network classes, causing an undefined variable; ensure $current_mac is
initialized before it's used (e.g., set a default empty string or null) prior to
the switch that sets it for class_id "0x02", or ensure you pass a safe value
when calling applyVFSettings; update the code around the applyVFSettings echo
and the switch that sets $current_mac so $current_mac is always defined when
used with applyVFSettings (references: applyVFSettings(), $current_mac, $vrf,
$class_id).
In @emhttp/plugins/dynamix/SysDevs.page:
- Around line 431-435: The function applyVFSettings assigns to mac without a
declaration, creating an implicit global; update the code in applyVFSettings to
declare mac locally (e.g., use let mac = ... or const mac = (class_id == '0x02'
? document.getElementById("vfmac" + pciId).value : null)); mirror the same fix
you applied to saveVFSettingsConfig so mac is not a global variable.
- Around line 203-208: In saveVFSettingsConfig, mac is assigned without a
declaration making it an implicit global; fix this by declaring mac with let (or
const if not reassigned) before use (e.g., initialize let mac = null), then set
mac = document.getElementById("vfmac" + pciId).value only when class_id ==
'0x02'; ensure vfio remains read from document.getElementById("vfvfio" +
pciId).checked and update the $.post call to use the locally scoped mac
variable.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (1)
225-225: Consider validatingclass_idinput.The
class_idis retrieved from POST without validation. While it's used in string comparisons, validating it against expected values ('0x02','0x03') would add defense-in-depth.♻️ Optional: Add validation for class_id
$mac = _var($_POST, 'mac'); $class_id = _var($_POST, 'class_id'); + // Validate class_id if provided + if ($class_id !== '' && !in_array($class_id, ['0x02', '0x03'], true)) { + json_response(false, _("Invalid device class")); + } $vfio = _var($_POST, 'vfio');
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/SysDevs.pageemhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.php
🧰 Additional context used
🧠 Learnings (2)
📚 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/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/SysDevs.page
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/SriovHelpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
🔇 Additional comments (5)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
16-16: LGTM!The extension of
$allowedPCIClassto include'0x03'(Display class) alongside'0x02'(Network class) correctly enables SR-IOV support for GPUs. Based on learnings, this global variable is used inSysDevs.phpwhich includes this file.emhttp/plugins/dynamix/include/apply.sriov-cfg.php (1)
241-262: LGTM - Class-aware MAC and VFIO handling.The logic correctly separates concerns:
- MAC changes only apply to network class (
0x02) devices- VFIO rebinding for non-network devices (e.g., GPU
0x03) proceeds without MAC considerationsThe control flow ensures that when both MAC and VFIO change for network devices, the MAC path handles the driver rebind, preventing duplicate operations.
emhttp/plugins/dynamix/include/SysDevs.php (1)
449-449: LGTM - Proper class_id propagation to save action.The
saveVFSettingsConfigcall correctly passesclass_idto enable class-aware configuration saving.emhttp/plugins/dynamix/SysDevs.page (2)
135-137: LGTM - Correctly enables display filter when SR-IOV filter is active.Since SR-IOV now supports both network (
0x02) and display (0x03) device classes, enablingshowdisplayalongsideshownetworkwhenshowsriovis activated ensures all SR-IOV capable devices are visible.
487-528: LGTM - Proper class_id propagation through the apply workflow.The
doVFSettingApplyfunction correctly:
- Accepts
class_idas a parameter- Includes it in the POST payload to
apply.sriov-cfg.php- Passes it to
saveVFSettingsConfigon successful applicationThis ensures consistent class-aware handling throughout the configuration flow.
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)
sbin/sriov (1)
56-56: Typo:/nshould be\nfor newline.The escape sequence for newline is
\n, not/n.Proposed fix
-echo -e "Loading settings config from $CFG_VFS/n" +echo -e "Loading settings config from $CFG_VFS\n"
🤖 Fix all issues with AI agents
In @sbin/sriov-vfsettings.sh:
- Around line 162-165: The error message in the VF_INDEX check wrongly uses $BDF
(the VF address) as the PF; update the string in the if-block that checks
VF_INDEX (the lines around the VF_INDEX, VF_PCI check) to reference the actual
PF variable (e.g. $PF or $PF_BDF) instead of $BDF, and ensure that the chosen PF
variable is defined earlier in the script so the message reads like "Error: VF
index not found for $VF_PCI under PF $PF" (or use the exact PF variable name
used elsewhere, e.g. $PF_BDF).
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
84-95: Variable shadowing is intentional but consider renaming for clarity.The
$class,$class_id, and$namevariables from line 76 are shadowed here. While this is intentional to get the VF's class info, the$nameis fetched but not added to the VF entry. Consider either:
- Using distinct variable names (e.g.,
$vf_class,$vf_class_id) for clarity- Adding
$nameto the VF entry if useful, or omit fetching itemhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
60-60: Consider validating class_id format.The
class_idis read from POST but not validated. While the regex inparseVFSettings()enforces0xprefix on read, malformed input could be written to the config file.Proposed validation
$class_id = _var($_POST,'class_id'); +if (!preg_match('/^0x[0-9a-fA-F]{2}$/', $class_id)) { + echo 0; + exit; +} $vfio= _var($_POST,'vfio');sbin/sriov-vfsettings.sh (1)
140-143: Consider replacingls | grepwith a glob-based approach.Shellcheck flags the
ls | greppattern as potentially problematic with non-alphanumeric filenames. While PCI paths are typically well-structured, a safer approach uses globs.Proposed alternative
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 + [ -e "$netdev" ] || continue + if [ "$(readlink -f "$netdev")" = "$PF_PATH" ]; then + PF_IFACE=$(basename "$(dirname "$netdev")") + break + fi + done fi
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/update.sriov-cfg.phpsbin/sriovsbin/sriov-vfsettings.sh
🧰 Additional context used
🧠 Learnings (3)
📚 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/SriovHelpers.phpemhttp/plugins/dynamix/include/update.sriov-cfg.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/apply.sriov-cfg.phpemhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/include/update.sriov-cfg.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/update.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/update.sriov-cfg.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(106-108)
🪛 Shellcheck (0.11.0)
sbin/sriov-vfsettings.sh
[warning] 142-142: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (13)
sbin/sriov (2)
14-15: LGTM!The updated comment correctly documents the new 5-field VFSETTINGS format including class_id.
68-70: LGTM!The 5-field parsing and forwarding to
sriov-vfsettings.shis correctly implemented and consistent with the new format.emhttp/plugins/dynamix/include/SriovHelpers.php (2)
16-16: LGTM!Extending the allowed PCI class list to include
0x03(display class) for GPU SR-IOV support is correct.
312-333: LGTM!The updated regex and parsing correctly handle the new 5-field VFSETTINGS format with class_id validation.
emhttp/plugins/dynamix/include/apply.sriov-cfg.php (3)
121-126: LGTM!Correctly gates MAC processing to network-class VFs only (class_id
0x02), which aligns with the design intent.
133-138: LGTM!The command construction correctly includes class_id in the proper position with proper escaping.
246-267: LGTM!The conditional logic correctly handles:
- MAC changes (network class only) with appropriate driver binding
- VFIO-only changes for non-network classes or when MAC is unchanged
The
setVfMacAddresscall on line 249 already handles the driver binding based on the target VFIO state, so combined MAC+VFIO changes are properly addressed.emhttp/plugins/dynamix/include/update.sriov-cfg.php (1)
71-76: LGTM!The entry construction correctly uses the 5-field format for both update and insert operations.
sbin/sriov-vfsettings.sh (5)
64-68: LGTM!The updated argument count check and usage message correctly document the new 5-parameter interface.
71-92: LGTM!The argument parsing correctly handles the positional parameters with appropriate pattern matching. The use of
-zchecks ensures proper ordering of CLASS_ID before VFIO parsing.
167-175: LGTM!MAC validation is appropriate for network-class VFs. The caller is expected to provide a MAC address (or the reset value
00:00:00:00:00:00).
183-207: LGTM!The driver unbind/rebind flow around MAC setting is correctly implemented, and the non-network class path appropriately skips MAC operations.
209-214: LGTM!VFIO binding is correctly applied regardless of device class, which is appropriate since both network and GPU VFs can be passed through to VMs.
Change SRIOV to be SR-IOV
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)
223-227: Data structure mismatch for "Removed" group entries.Line 226 stores removed entries as strings (
$lsiommu[$removedKey][$bdf] = $line), but line 250 expects an associative array with a'line'key ($pcidetail['line']). This will cause removed devices to fail rendering or display incorrect data.Proposed fix: use consistent data structure
- $lsiommu[$removedKey][$bdf] = $line; + $lsiommu[$removedKey][$bdf]['line'] = $line;
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix/include/SysDevs.php:
- Around line 249-252: The foreach over $group can hit the metadata key
'_has_sriov' whose value is a boolean, so accessing $pcidetail['line'] causes
PHP warnings; in the loop (the foreach ($group as $pciaddress => $pcidetail)
block) add an explicit guard to skip non-array entries (e.g., continue if
!is_array($pcidetail)) before referencing $pcidetail['line'], and keep the
existing in_array($pciaddress,$sriovvfs) && !empty($group['_has_sriov']) check
intact.
- Around line 465-489: The switch lacks safe defaults causing $mac, $action, and
$saveaction to be undefined if $class_id falls through; fix by initializing
those variables before the switch (e.g. $mac = null; $action = ''; $saveaction =
'') or by populating them in the default: branch; ensure the rest of the code
that uses $mac (saved/current), the generateMAC() call, and the final
saveVFSettingsConfig(...) invocation always have defined values
($saved_mac/$current_mac should be derived from $mac after initialization).
In @emhttp/plugins/dynamix/SysDevs.page:
- Line 205: The assignment to mac in the SysDevs.page snippet is missing a
declaration and creates an implicit global; declare the variable (e.g., use let
or const) in the local scope before or as part of the conditional (replace the
bare assignment to mac in the class_id == '0x02' branch with a declared variable
like let mac = ... or declare let mac; then set mac = ... in the branches) so
mac is block-scoped and not global — update the line using the unique
identifiers class_id and pciId (or the element id "vfmac"+pciId) to locate and
fix the assignment.
- Line 434: The assignment to mac in the conditional creates an implicit global;
declare the variable locally (e.g., use let or const) before or in the branch so
it’s not global. Replace the bare assignment in the block that checks class_id
== '0x02' (the line using document.getElementById("vfmac" + pciId).value) with a
local declaration such as let mac = ... (or declare let mac; above and assign
inside the branches), mirroring the fix used at the other occurrence around line
205 to avoid leaking mac to the global scope.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/SysDevs.pageemhttp/plugins/dynamix/include/SysDevs.php
🧰 Additional context used
🧠 Learnings (2)
📚 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.pageemhttp/plugins/dynamix/include/SysDevs.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.php
🔇 Additional comments (6)
emhttp/plugins/dynamix/include/SysDevs.php (3)
116-136: LGTM!The SR-IOV group flagging logic correctly identifies IOMMU groups that contain SR-IOV devices but where the first device is not SR-IOV. This enables appropriate warning display for ACS override requirements.
316-322: LGTM!The SR-IOV warning appropriately alerts users when IOMMU groups contain mixed SR-IOV devices, directing them to enable ACS Override for proper isolation.
236-242: LGTM!The filter adjustment correctly handles IOMMU groups where the first device is not SR-IOV by using the second device for class-based filtering. The
_has_sriovkey check prevents using metadata for filtering.emhttp/plugins/dynamix/SysDevs.page (3)
74-74: LGTM!The label update to "SR-IOV" follows standard terminology, and auto-enabling
showdisplaywhen SR-IOV is toggled on ensures GPU-related SR-IOV devices are visible alongside network VFs.Also applies to: 135-137
487-528: LGTM!The
doVFSettingApplyfunction correctly propagatesclass_idandcurrentmacthrough the POST payload and subsequent save operation, enabling proper per-class handling on the server side.
188-193: LGTM!Terminology updated to "SR-IOV" for consistency with industry standards.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/SysDevs.php (1)
317-323: Consider extracting the link from the translatable string.The HTML
<a>tag embedded in the translatable string may cause issues for translators and could break if the translation system escapes HTML. Consider separating the link:♻️ Suggested improvement
- echo '<span class="orange-text">'._('Warning').': '._('IOMMU group contains SR-IOV devices. Enable ACS Override in <a href="/Main/Flash#boot-parameters">Boot Parameters</a> for proper isolation').'.</span>'; + echo '<span class="orange-text">'._('Warning').': '._('IOMMU group contains SR-IOV devices. Enable ACS Override in').' <a href="/Main/Flash#boot-parameters">'._('Boot Parameters').'</a> '._('for proper isolation').'.</span>';
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/SysDevs.pageemhttp/plugins/dynamix/include/SriovHelpers.phpemhttp/plugins/dynamix/include/SysDevs.php
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/SysDevs.page
🧰 Additional context used
🧠 Learnings (2)
📚 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-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/SriovHelpers.php
🔇 Additional comments (7)
emhttp/plugins/dynamix/include/SriovHelpers.php (3)
16-16: LGTM - Extended allowed PCI class list.The addition of
'0x03'(display controllers) to$allowedPCIClassenables SR-IOV support for GPUs, which aligns with the PR objective. Based on learnings, this variable is consumed inSysDevs.php.
84-94: LGTM - VF class information properly exposed.The VF class and class_id are now correctly retrieved and added to each VF entry, enabling class-specific handling downstream. The
$vfnamevariable is unused but this is acceptable as only the class information is needed for VFs.
316-330: Regex pattern is correct for the actual data format.The regex at line 316 correctly expects
0x[[:xdigit:]]{2}because the full 4-digit class_id returned bygetPciClassNameAndId()(e.g.,0x0200) is intentionally truncated at SysDevs.php line 463 usingsubstr($vrf['class_id'],0,4), reducing it to the 2-digit format (e.g.,0x02). This truncated value is passed to the POST handler and stored in the config file, matching the regex pattern. The example at line 15 of update.sriov-cfg.php confirms the actual stored format uses 2-digit class_id values like0x02and0x02.emhttp/plugins/dynamix/include/SysDevs.php (4)
114-136: LGTM - SR-IOV group flagging logic.The logic correctly identifies IOMMU groups containing SR-IOV devices where the first device is not SR-IOV-capable. This enables appropriate warnings about ACS override requirements. The
is_string($pdev)check properly skips metadata keys.
236-253: LGTM - Adjusted group iteration for SR-IOV handling.The changes correctly handle the mixed IOMMU groups by:
- Using the second device for filter checks when the first isn't SR-IOV
- Properly iterating the new nested data structure
- Skipping VFs in flagged groups to avoid duplicate rendering
463-508: LGTM - Class-specific VF UI implementation.The class-based switch statement properly differentiates between network controllers (0x02) with MAC address handling and display controllers (0x03). The use of
htmlspecialchars()andhtmlentities()for user-facing values provides appropriate XSS protection.
391-391: No concerns with this line.
Stop VMs starting if Root devices defined and VFs exist.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js:
- Around line 194-196: The code redeclares errtext with var and overwrites
messages when both pcierror and srioverror are true; fix by declaring errtext
once (use let or const) before the conditionals and then set it via
mutually-exclusive logic or by combining messages: e.g., if both pcierror and
srioverror are true build a single string like "Start disabled due to PCI
Changes and SR-IOV GPU Changes", otherwise set the single message; update the
branch that pushes to opts (opts.push({...})) to use the single errtext
variable.
In @emhttp/plugins/dynamix/include/DashboardApps.php:
- Line 178: The addVMContext JavaScript call is missing the two error-flag
arguments ($pcierror and $srioverror) and currently only passes 11 parameters
instead of the required 13; update the other addVMContext invocation to include
the two error flags as the final arguments (matching the existing call that
passes ..., $pcierror, $srioverror) so the JS function receives all parameters
and PCI / SR-IOV error state is propagated to the VM usage menu.
🧹 Nitpick comments (5)
emhttp/plugins/dynamix.vm.manager/scripts/pcicheck.php (2)
45-46: Simplify redundant ternary operators.The ternary expressions are redundant since we're already inside the
ifblock where the condition is truthy. The"no"branches are unreachable dead code.♻️ Suggested simplification
-if ($pcierror) { echo $pcierror == true ? "yes" : "no"; exit; } -if ($srioverror) { echo $srioverror == true ? "sriov" : "no"; exit; } +if ($pcierror) { echo "yes"; exit; } +if ($srioverror) { echo "sriov"; exit; }
27-44: Consider consistent indentation.The code inside the
foreachloops has inconsistent indentation - some lines start at column 0, others are indented. This is a minor style nit.♻️ Suggested formatting
foreach ($argv as $arg) { -if (preg_match('/"host"\s*:\s*"([^"]+)"/', $arg, $matches)) { - $pci_addresses[] = $matches[1]; + if (preg_match('/"host"\s*:\s*"([^"]+)"/', $arg, $matches)) { + $pci_addresses[] = $matches[1]; } } foreach($pci_addresses as $pciid) { -if (isset($pci_device_changes[$pciid])) { - $pcierror = true; + if (isset($pci_device_changes[$pciid])) { + $pcierror = true; } // Check if device is an SR-IOV PF with VFs defined $check_id = $pciid; if (!preg_match('/^[0-9a-fA-F]{4}:/', $check_id)) { $check_id = "0000:" . $check_id; } if (isset($sriov[$check_id]) && !empty($sriov[$check_id]['vfs'])) { $srioverror = true; } }emhttp/plugins/dynamix/include/SysDevs.php (2)
116-136: Logic for filtering metadata key may be incorrect.Line 122 uses
!is_string($pdev)to skip non-device keys, but_has_sriovis a string key. While this currently works because_has_sriovis only set after the loop completes for each group, the check is misleading and fragile.Consider using a more explicit check:
♻️ Suggested improvement
foreach ($devices as $pdev => $pdetail) { - if (!is_string($pdev)) continue; + if (!is_string($pdev) || str_starts_with($pdev, '_')) continue;
317-323: Inline HTML in translation string may cause i18n issues.The
<a>tag is embedded within the translated string on line 321. This can cause problems for translators and may break if the translation system doesn't handle HTML properly.♻️ Suggested improvement
- echo '<span class="orange-text">'._('Warning').': '._('IOMMU group contains SR-IOV devices. Enable ACS Override in <a href="/Main/Flash#boot-parameters">Boot Parameters</a> for proper isolation').'.</span>'; + echo '<span class="orange-text">'._('Warning').': '._('IOMMU group contains SR-IOV devices. Enable ACS Override in').' <a href="/Main/Flash#boot-parameters">'._('Boot Parameters').'</a> '._('for proper isolation').'.</span>';emhttp/plugins/dynamix.vm.manager/include/VMMachines.php (1)
60-70: SR-IOV error detection logic is correct.The implementation properly normalizes PCI IDs and checks for VF presence. Note that the PCI ID normalization pattern (lines 64-67) is duplicated from
libvirt_helpers.php(lines 924-926).Consider extracting the PCI ID normalization to a shared helper function to reduce duplication:
function normalizePciId($pciId) { if (!preg_match('/^[0-9a-fA-F]{4}:/', $pciId)) { return "0000:" . $pciId; } return $pciId; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.phpemhttp/plugins/dynamix.vm.manager/javascript/vmmanager.jsemhttp/plugins/dynamix.vm.manager/scripts/pcicheck.phpemhttp/plugins/dynamix/include/DashboardApps.phpemhttp/plugins/dynamix/include/SysDevs.phpsbin/qemu
🧰 Additional context used
🧠 Learnings (3)
📚 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.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/scripts/pcicheck.phpemhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.phpemhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/DashboardApps.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.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/scripts/pcicheck.phpemhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.phpemhttp/plugins/dynamix/include/SysDevs.phpemhttp/plugins/dynamix/include/DashboardApps.php
🧬 Code graph analysis (4)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
getSriovInfoJson(54-142)
emhttp/plugins/dynamix.vm.manager/scripts/pcicheck.php (2)
emhttp/plugins/dynamix/include/Helpers.php (1)
comparePCIData(605-651)emhttp/plugins/dynamix/include/SriovHelpers.php (1)
getSriovInfoJson(54-142)
emhttp/plugins/dynamix/include/SysDevs.php (1)
emhttp/plugins/dynamix/include/SmartInfo.php (1)
append(44-46)
emhttp/plugins/dynamix/include/DashboardApps.php (1)
emhttp/plugins/dynamix/include/SriovHelpers.php (1)
getSriovInfoJson(54-142)
🪛 Biome (2.1.2)
emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js
[error] 195-195: Shouldn't redeclare 'errtext'. Consider to delete it or rename it.
'errtext' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (20)
emhttp/plugins/dynamix.vm.manager/scripts/pcicheck.php (3)
19-19: LGTM!The include for SriovHelpers.php follows the existing pattern and provides access to
getSriovInfoJson().
23-23: LGTM!The JSON decode correctly passes
truefor associative array output, and downstreamisset()checks safely handle the case where$sriovisnull.
36-43: LGTM!The PCI ID normalization correctly adds the
0000:domain prefix when missing, matching the key format used ingetSriovInfoJson()output. The SR-IOV VF check properly validates that the device is a PF with active VFs.emhttp/plugins/dynamix/include/SysDevs.php (5)
236-242: LGTM!The fallback logic to use the second entry for filter checking when the first entry is a non-SR-IOV device in an SR-IOV group is correctly implemented. The check for
_has_sriovkey ensures metadata isn't used for filtering.
249-253: LGTM!The iteration correctly handles the new data structure by checking
!is_array($pcidetail)to filter metadata keys, and properly accesses the line via$pcidetail['line'].
463-504: LGTM!The class_id-based handling correctly restricts MAC address UI to network controllers (
0x02) and shows class information for display controllers (0x03). The HTML escaping withhtmlspecialcharsandhtmlentitiesproperly prevents XSS vulnerabilities.
223-226: LGTM!The removed entries correctly use the new
['line' => $line]structure, maintaining consistency with the data structure established at line 111.
389-391: LGTM!The SR-IOV Available VFs display is correctly implemented.
sbin/qemu (1)
16-20: LGTM - SR-IOV root device check is correctly implemented.The new conditional block follows the same pattern as the existing PCI change detection at lines 11-15, providing consistent error handling and exit behavior for SR-IOV root devices.
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2)
173-173: LGTM - Dependency inclusion is correct.The path resolves correctly at runtime since
$docroot/webGuiis a symlink toplugins/dynamix. Based on learnings, this is the expected pattern for the Unraid webgui repository.
918-931: SR-IOV physical function filtering looks correct.The implementation properly:
- Retrieves SR-IOV info and decodes it as an associative array
- Normalizes PCI IDs to include the
0000:domain prefix for consistent lookup- Removes GPU devices that are SR-IOV physical functions with VFs defined
One minor note: the regex pattern
/^[0-9a-fA-F]{4}:/checks for the domain prefix correctly.emhttp/plugins/dynamix.vm.manager/include/VMMachines.php (3)
44-44: LGTM - Efficient SR-IOV data retrieval.Good design choice to fetch SR-IOV info once before the VM loop rather than inside it, avoiding redundant system calls.
220-220: LGTM - SR-IOV warning icon is consistent with PCI warning.The warning icon and tooltip follow the same pattern as the existing PCI warning on line 219, providing consistent user feedback.
138-138: addVMContext function signature is properly updated.The JavaScript function in vmmanager.js correctly accepts both
pcierrorandsrioverroras the final parameters, matching the 13 arguments passed from the PHP sprintf call.emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js (2)
86-86: LGTM - Function signature correctly extended.The
srioverror=falseparameter is properly added with a default value, maintaining backward compatibility.
175-175: LGTM - Guard condition correctly updated.The condition
!pcierror && !srioverrorproperly prevents VM start when either error flag is set.emhttp/plugins/dynamix/include/DashboardApps.php (4)
22-22: LGTM!The include for
SriovHelpers.phpis correctly added and necessary for thegetSriovInfoJson()function used later in this file.
120-120: LGTM!Fetching SR-IOV info once before the loop is efficient. The downstream
isset()check at line 174 handles potentialnullfromjson_decodegracefully.
164-176: LGTM!The SR-IOV PF detection logic correctly normalizes PCI addresses and checks for VFs. The
isset()combined with!empty()ensures safe array access.Minor observation: Line 166 unconditionally prepends
"0000:"while lines 171-173 check first. The defensive check here is the better approach.
201-202: LGTM!The SR-IOV warning icon follows the same pattern as the existing PCI error icon, with appropriate styling and translated tooltip text.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js (1)
86-86: Consider refactoring to an options object in a future PR.The function now has 13 positional parameters, which can make call sites harder to read and maintain. The current change is backward-compatible and follows the existing pattern, so it's acceptable for this PR.
For a future improvement, consider consolidating the trailing boolean/optional parameters into a single options object:
// Example future signature function addVMContext(name, uuid, template, state, vmrcurl, vmrcprotocol, log, options = {}) { const { fstype = "QEMU", consolein = "web;no", usage = false, webui = "", pcierror = false, srioverror = false } = options; // ... }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js
🔇 Additional comments (2)
emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js (2)
175-179: LGTM!The guard condition correctly prevents VM start when either
pcierrororsrioverroris true, ensuring users cannot start a VM with misconfigured PCI or SR-IOV GPU settings.
193-205: LGTM!The error message handling is well-structured, covering all three cases (both errors, PCI only, SR-IOV only) with clear, internationalized messages. The explicit
else if (srioverror)on line 199 improves readability even though a plainelsewould suffice logically.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.