Feature: SRIOV GPU Changes #2512
Conversation
WalkthroughPropagated a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Page as SysDevs.page (JS)
participant Helper as SriovHelpers
participant Apply as apply.sriov-cfg.php
participant Script as sbin/sriov-vfsettings.sh
UI->>Page: user triggers "Apply VF settings" (includes class_id)
Page->>Helper: get VF details (includes class_id)
Page->>Page: applyVFSettings(...) routes by class_id
alt class_id == '0x02' (Network)
Page->>Apply: doVFSettingApply(pciId, vd, vfio, mac, ..., class_id)
Apply->>Script: invoke sriov-vfsettings.sh with class_id, vfio, mac
Script->>Script: validate MAC, unbind VF, set MAC, rebind, bind vfio if requested
else class_id == '0x03' (Display)
Page->>Apply: doVFSettingApply(pciId, vd, vfio, null, ..., class_id)
Apply->>Script: invoke sriov-vfsettings.sh with class_id, vfio (MAC skipped)
Script->>Script: skip MAC path, perform VFIO bind if requested
end
Script-->>Apply: success/failure
Apply-->>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
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.
| // Device class + numeric class + name | ||
| [$class, $class_id, $name] = getPciClassNameAndId($vf_pci); | ||
| if ($includeVfDetails) { | ||
| // Vendor:Device formatted string | ||
| $vendorFile = "/sys/bus/pci/devices/{$vf_pci}/vendor"; | ||
| $deviceFile = "/sys/bus/pci/devices/{$vf_pci}/device"; | ||
| $vendor = is_readable($vendorFile) ? trim(file_get_contents($vendorFile)) : null; | ||
| $device = is_readable($deviceFile) ? trim(file_get_contents($deviceFile)) : null; | ||
| $vf_entry['vd'] = ($vendor && $device) ? sprintf('%s:%s', substr($vendor, 2), substr($device, 2)) : null; | ||
|
|
||
| $vf_entry['class'] = $class; | ||
| $vf_entry['class_id'] = $class_id; | ||
| // Network interface info |
There was a problem hiding this comment.
Variable shadowing overwrites PF class info with VF class info.
The variables $class, $class_id, and $name are first set at line 76 for the Physical Function (PF), but line 85 reassigns them inside the VF loop. After the loop completes, $results[$pci] (lines 127-129) will incorrectly contain the last VF's class information instead of the PF's class info.
🐛 Proposed fix: Use distinct variable names for VF class info
// Device class + numeric class + name
- [$class, $class_id, $name] = getPciClassNameAndId($vf_pci);
+ [$vf_class, $vf_class_id, $vf_name] = getPciClassNameAndId($vf_pci);
if ($includeVfDetails) {
// Vendor:Device formatted string
$vendorFile = "/sys/bus/pci/devices/{$vf_pci}/vendor";
$deviceFile = "/sys/bus/pci/devices/{$vf_pci}/device";
$vendor = is_readable($vendorFile) ? trim(file_get_contents($vendorFile)) : null;
$device = is_readable($deviceFile) ? trim(file_get_contents($deviceFile)) : null;
$vf_entry['vd'] = ($vendor && $device) ? sprintf('%s:%s', substr($vendor, 2), substr($device, 2)) : null;
- $vf_entry['class'] = $class;
- $vf_entry['class_id'] = $class_id;
+ $vf_entry['class'] = $vf_class;
+ $vf_entry['class_id'] = $vf_class_id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Device class + numeric class + name | |
| [$class, $class_id, $name] = getPciClassNameAndId($vf_pci); | |
| if ($includeVfDetails) { | |
| // Vendor:Device formatted string | |
| $vendorFile = "/sys/bus/pci/devices/{$vf_pci}/vendor"; | |
| $deviceFile = "/sys/bus/pci/devices/{$vf_pci}/device"; | |
| $vendor = is_readable($vendorFile) ? trim(file_get_contents($vendorFile)) : null; | |
| $device = is_readable($deviceFile) ? trim(file_get_contents($deviceFile)) : null; | |
| $vf_entry['vd'] = ($vendor && $device) ? sprintf('%s:%s', substr($vendor, 2), substr($device, 2)) : null; | |
| $vf_entry['class'] = $class; | |
| $vf_entry['class_id'] = $class_id; | |
| // Network interface info | |
| // Device class + numeric class + name | |
| [$vf_class, $vf_class_id, $vf_name] = getPciClassNameAndId($vf_pci); | |
| if ($includeVfDetails) { | |
| // Vendor:Device formatted string | |
| $vendorFile = "/sys/bus/pci/devices/{$vf_pci}/vendor"; | |
| $deviceFile = "/sys/bus/pci/devices/{$vf_pci}/device"; | |
| $vendor = is_readable($vendorFile) ? trim(file_get_contents($vendorFile)) : null; | |
| $device = is_readable($deviceFile) ? trim(file_get_contents($deviceFile)) : null; | |
| $vf_entry['vd'] = ($vendor && $device) ? sprintf('%s:%s', substr($vendor, 2), substr($device, 2)) : null; | |
| $vf_entry['class'] = $vf_class; | |
| $vf_entry['class_id'] = $vf_class_id; | |
| // Network interface info |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix/include/SriovHelpers.php around lines 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.
| $class_id = substr($vrf['class_id'],0,4); | ||
| switch ($class_id) { | ||
| case "0x02": # Network controller | ||
| if (isset($sriov_devices_settings[$pciaddress])) { | ||
| $mac = $sriov_devices_settings[$pciaddress]['mac']; | ||
| } else { | ||
| $mac = null; | ||
| } | ||
| $placeholder = empty($mac) ? _('Dynamic allocation') : ''; | ||
| $saved_mac = empty($mac) ? '' : htmlspecialchars($mac, ENT_QUOTES); | ||
| $current_mac = empty($vrf['mac']) ? '' : htmlspecialchars(strtoupper($vrf['mac']), ENT_QUOTES); | ||
| 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>'; | ||
| $action = _("Apply now VFIO and MAC Address"); | ||
| $saveaction = _("Save MAC and vfio config"); | ||
| break; | ||
| case "0x03": # Display controller | ||
| echo '<span>'._($vrf['class']).'</span>'; | ||
| $mac = null; | ||
| $action = _("Apply now VFIO binding"); | ||
| $saveaction = _("Save Vfio config"); | ||
| break; | ||
| default: | ||
| } |
There was a problem hiding this comment.
Potential undefined variable $mac for unhandled device classes.
If $class_id is neither '0x02' nor '0x03', the switch falls through to the empty default case without setting $mac. This could cause an undefined variable warning on line 460 where $mac is used.
🐛 Proposed fix: Initialize $mac before switch or handle in default
echo "<tr><td></td><td></td><td></td><td></td><td>";
$class_id = substr($vrf['class_id'],0,4);
+ $mac = null;
+ $current_mac = '';
+ $action = _("Apply now VFIO binding");
+ $saveaction = _("Save Vfio config");
switch ($class_id) {
case "0x02": # Network controllerOr add explicit handling in default:
case "0x03": # Display controller
echo '<span>'._($vrf['class']).'</span>';
- $mac = null;
$action = _("Apply now VFIO binding");
$saveaction = _("Save Vfio config");
break;
default:
+ $action = _("Apply now VFIO binding");
+ $saveaction = _("Save Vfio config");
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $class_id = substr($vrf['class_id'],0,4); | |
| switch ($class_id) { | |
| case "0x02": # Network controller | |
| if (isset($sriov_devices_settings[$pciaddress])) { | |
| $mac = $sriov_devices_settings[$pciaddress]['mac']; | |
| } else { | |
| $mac = null; | |
| } | |
| $placeholder = empty($mac) ? _('Dynamic allocation') : ''; | |
| $saved_mac = empty($mac) ? '' : htmlspecialchars($mac, ENT_QUOTES); | |
| $current_mac = empty($vrf['mac']) ? '' : htmlspecialchars(strtoupper($vrf['mac']), ENT_QUOTES); | |
| 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>'; | |
| $action = _("Apply now VFIO and MAC Address"); | |
| $saveaction = _("Save MAC and vfio config"); | |
| break; | |
| case "0x03": # Display controller | |
| echo '<span>'._($vrf['class']).'</span>'; | |
| $mac = null; | |
| $action = _("Apply now VFIO binding"); | |
| $saveaction = _("Save Vfio config"); | |
| break; | |
| default: | |
| } | |
| $class_id = substr($vrf['class_id'],0,4); | |
| $mac = null; | |
| $current_mac = ''; | |
| $action = _("Apply now VFIO binding"); | |
| $saveaction = _("Save Vfio config"); | |
| switch ($class_id) { | |
| case "0x02": # Network controller | |
| if (isset($sriov_devices_settings[$pciaddress])) { | |
| $mac = $sriov_devices_settings[$pciaddress]['mac']; | |
| } else { | |
| $mac = null; | |
| } | |
| $placeholder = empty($mac) ? _('Dynamic allocation') : ''; | |
| $saved_mac = empty($mac) ? '' : htmlspecialchars($mac, ENT_QUOTES); | |
| $current_mac = empty($vrf['mac']) ? '' : htmlspecialchars(strtoupper($vrf['mac']), ENT_QUOTES); | |
| 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>'; | |
| $action = _("Apply now VFIO and MAC Address"); | |
| $saveaction = _("Save MAC and vfio config"); | |
| break; | |
| case "0x03": # Display controller | |
| echo '<span>'._($vrf['class']).'</span>'; | |
| $mac = null; | |
| $action = _("Apply now VFIO binding"); | |
| $saveaction = _("Save Vfio config"); | |
| break; | |
| default: | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix/include/SysDevs.php around lines 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.
| echo ' <a class="info" href="#" title="'._("Action VFs update").'" onclick="applyVFSettings(\''.htmlentities($pciaddress).'\',\''.htmlentities($vd).'\',\''.htmlentities($vfiocheck).'\',\''.$current_mac.'\',\''.$class_id.'\'); return false;"><i title="'.$action.'" class="fa fa-play"></i></a> '; | ||
| if ($class_id == "0x02") { | ||
| if ($vrf['driver'] != "vfio-pci" ) echo _("Current").": "; | ||
| echo $vrf['driver'] == "vfio-pci" ? _("Bound to VFIO") : strtoupper($vrf['mac']); | ||
| } |
There was a problem hiding this comment.
$current_mac may be undefined for non-network device classes.
The variable $current_mac is only set in the 0x02 case (line 434), but it's used in applyVFSettings() call on line 454 for all device classes. For 0x03 (Display) devices, this will trigger an undefined variable warning.
🐛 Proposed fix: Initialize $current_mac before the switch
$class_id = substr($vrf['class_id'],0,4);
+ $current_mac = '';
switch ($class_id) {
case "0x02": # Network controller🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix/include/SysDevs.php around lines 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).
| function saveVFSettingsConfig(pciId,vd,class_id,interactive=1) { | ||
| var message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | ||
| var mac = document.getElementById("vfmac" + pciId).value; | ||
| if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null; | ||
| var vfio = document.getElementById("vfvfio" + pciId).checked; | ||
|
|
||
| $.post( "/plugins/dynamix/include/update.sriov-cfg.php", { type:"sriovsettings", pciid: pciId, vd:vd, vfio:vfio, mac:mac } ) | ||
| $.post( "/plugins/dynamix/include/update.sriov-cfg.php", { type:"sriovsettings", pciid: pciId, vd:vd, vfio:vfio, mac:mac, class_id:class_id } ) |
There was a problem hiding this comment.
Implicit global variable mac - missing var/let/const declaration.
On line 205, mac is assigned without a declaration keyword, making it an implicit global variable. This can cause unintended side effects and is considered bad practice.
🐛 Proposed fix: Declare mac with let or const
function saveVFSettingsConfig(pciId,vd,class_id,interactive=1) {
var message = "_(System Devices)_: _(A reboot is required to apply changes)_";
- if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null;
+ const mac = (class_id == '0x02') ? document.getElementById("vfmac" + pciId).value : null;
var vfio = document.getElementById("vfvfio" + pciId).checked;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function saveVFSettingsConfig(pciId,vd,class_id,interactive=1) { | |
| var message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | |
| var mac = document.getElementById("vfmac" + pciId).value; | |
| if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null; | |
| var vfio = document.getElementById("vfvfio" + pciId).checked; | |
| $.post( "/plugins/dynamix/include/update.sriov-cfg.php", { type:"sriovsettings", pciid: pciId, vd:vd, vfio:vfio, mac:mac } ) | |
| $.post( "/plugins/dynamix/include/update.sriov-cfg.php", { type:"sriovsettings", pciid: pciId, vd:vd, vfio:vfio, mac:mac, class_id:class_id } ) | |
| function saveVFSettingsConfig(pciId,vd,class_id,interactive=1) { | |
| var message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | |
| const mac = (class_id == '0x02') ? document.getElementById("vfmac" + pciId).value : null; | |
| var vfio = document.getElementById("vfvfio" + pciId).checked; | |
| $.post( "/plugins/dynamix/include/update.sriov-cfg.php", { type:"sriovsettings", pciid: pciId, vd:vd, vfio:vfio, mac:mac, class_id:class_id } ) |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix/SysDevs.page around lines 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.
| function applyVFSettings(pciId, vd, currentvfio, currentmac,class_id) { | ||
|
|
||
| const message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | ||
| const mac = document.getElementById("vfmac" + pciId).value; | ||
| if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null; | ||
| const vfio = document.getElementById("vfvfio" + pciId).checked; |
There was a problem hiding this comment.
Same implicit global variable issue for mac.
Line 434 has the same issue as saveVFSettingsConfig - mac is assigned without a declaration keyword.
🐛 Proposed fix: Declare mac with let or const
function applyVFSettings(pciId, vd, currentvfio, currentmac,class_id) {
const message = "_(System Devices)_: _(A reboot is required to apply changes)_";
- if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null;
+ const mac = (class_id == '0x02') ? document.getElementById("vfmac" + pciId).value : null;
const vfio = document.getElementById("vfvfio" + pciId).checked;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function applyVFSettings(pciId, vd, currentvfio, currentmac,class_id) { | |
| const message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | |
| const mac = document.getElementById("vfmac" + pciId).value; | |
| if (class_id == '0x02') mac = document.getElementById("vfmac" + pciId).value; else mac = null; | |
| const vfio = document.getElementById("vfvfio" + pciId).checked; | |
| function applyVFSettings(pciId, vd, currentvfio, currentmac,class_id) { | |
| const message = "_(System Devices)_: _(A reboot is required to apply changes)_"; | |
| const mac = (class_id == '0x02') ? document.getElementById("vfmac" + pciId).value : null; | |
| const vfio = document.getElementById("vfvfio" + pciId).checked; |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix/SysDevs.page around lines 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.
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.
| if [ -z "$VF_INDEX" ]; then | ||
| echo "Error: VF index not found for $VF_PCI under PF $BDF" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Error message references incorrect variable.
The error message says "under PF $BDF" but $BDF is the VF's address, not the PF's. This could confuse debugging.
Proposed fix
if [ -z "$VF_INDEX" ]; then
- echo "Error: VF index not found for $VF_PCI under PF $BDF"
+ echo "Error: VF index not found for $VF_PCI under PF $(basename "$PF_PATH")"
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$VF_INDEX" ]; then | |
| echo "Error: VF index not found for $VF_PCI under PF $BDF" | |
| exit 1 | |
| fi | |
| if [ -z "$VF_INDEX" ]; then | |
| echo "Error: VF index not found for $VF_PCI under PF $(basename "$PF_PATH")" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In @sbin/sriov-vfsettings.sh around lines 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).
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.