feat(Internal_Boot): add initial boot options#2538
feat(Internal_Boot): add initial boot options#2538SimonFair wants to merge 41 commits intounraid:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Boot Device Settings UI with backup/cleanup flow; implements GRUB support across BootParameters UI, backend handler, and management script; introduces mkbootpool endpoint and script for bootable pools; enriches device↔pool mapping and block-device helpers; augments CacheDevices boot‑pool UI and guards DeviceInfo form access. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant UI as Web UI (JS)
participant PHP as emhttp PHP
participant Script as manage_boot_params.sh
participant FS as Filesystem
Browser->>UI: user action (apply params / start backup / create boot pool)
UI->>PHP: POST apply / request backup / create boot pool
PHP->>Script: invoke bootloader-aware operation (read/write/list/restore) or mkbootpool
Script->>FS: read/update grub.cfg or syslinux.cfg, manage backups, efibootmgr changes
FS-->>Script: operation result
Script-->>PHP: result/status (includes bootloader_type, backup path, efiboot output)
PHP-->>UI: response (success/error, download URL, logs)
UI->>Browser: show preview/result or redirect to backup ZIP
Browser->>UI: on backup completion trigger cleanup
UI->>PHP: request unlink backup file
PHP->>FS: remove zip
FS-->>PHP: confirmation
PHP-->>UI: cleanup confirmation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🔧 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: 6
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/BootInfo.page`:
- Around line 58-60: The external anchor element linking to
"https://unraid.net/contact" with text '_(Contact Support)_' uses
target="_blank" and should include rel="noopener noreferrer" to prevent
tab-nabbing; update that <a> tag in BootInfo.page (the anchor with href
"https://unraid.net/contact" and text '_(Contact Support)_') to add
rel="noopener noreferrer" while keeping target="_blank".
- Around line 47-54: The template currently prints raw device metadata via
$var['flashVendor'], $var['flashProduct'], and $var['flashGUID']; escape these
values before rendering to prevent HTML injection by applying an HTML-escaping
function (e.g., htmlspecialchars with ENT_QUOTES and UTF-8) wherever these
variables are output in BootInfo.page so the rendered text is safe even if
descriptors contain markup or quotes.
In `@emhttp/plugins/dynamix/BootParameters.page`:
- Around line 1917-1919: Normalize the bootloader type string by lowercasing the
value assigned to bootloaderType (from config.bootloader_type) before any
comparisons; update the assignment for bootloaderType and ensure all downstream
checks that use bootloaderType (e.g., any conditionals or switch statements
referencing bootloaderType) operate on the normalized lowercase value so inputs
like "GRUB" or "Syslinux" match correctly against "grub" and "syslinux".
- Around line 1244-1285: The dropdown currently uses deciseconds (timeoutDeci /
timeoutOptions) for option values causing a 10x mismatch on submit; update the
value logic so the <select id="timeout-dropdown"> stores seconds (not
deciseconds) that the backend expects: convert parsedTimeout (from set timeout=)
back to seconds for the selected value, build option .val() using
Math.round(deci/10) (or keep a data attribute like data-deci if you must
preserve precision) and ensure the customOption uses the original parsed
seconds; keep UI labels in seconds via formatSeconds but submit seconds, and
ensure markFormModified() remains on change.
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 288-290: Replace regex matching of GRUB menuentry labels with
AWK's literal substring matching: wherever the script sets in_section using the
regex operator against "$0" and the quoted label (e.g., the block matching
"menuentry" that assigns in_section = ($0 ~ "\"" label "\"")), change the logic
to use AWK's index() to test for the literal substring of "\"" label "\""
instead of "~" so parentheses in labels are treated literally; apply the same
replacement for the analogous assignments around the other occurrences
referenced (the blocks around the symbols handling menuentry, label, and
in_section at the other two locations).
- Around line 291-295: The AWK uses the GNU-only \s in gsub and should be made
POSIX-compliant: in the block matching linux/linuxefi (the match(...) line and
the subsequent gsub(...)), replace instances of \s with the POSIX whitespace
class [[:space:]] (e.g. change gsub(/^\s+|\s+$/, "", args) to
gsub(/^[[:space:]]+|[[:space:]]+$/, "", args)); keep the existing [ \t] regex
for explicit space/tab matching elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 0
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/nchan/device_list (1)
551-622:⚠️ Potential issue | 🟠 MajorRemove duplicated device cells in the DISK_NP branch.
There are four identical<td>outputs, which breaks table column alignment.🧩 Suggested fix
if (in_array(_var($disk,'name'),$pools) || $fstype=='zfs') { $echo[] = "<td>".device_info($disk,true,$poolName,$poolstatusData,$options)."</td>"; - $echo[] = "<td>".device_info($disk,true,$poolName,$poolstatusData,$options)."</td>"; - $echo[] = "<td>".device_info($disk,true,$poolName,$poolstatusData,$options)."</td>"; - $echo[] = "<td>".device_info($disk,true,$poolName,$poolstatusData,$options)."</td>"; $echo[] = "<td><a class='static'><i class='icon-disk icon'></i><span></span></a><em>".($fstype=='zfs' ? _('Not present') : _('Not installed'))."</em></td>"; $echo[] = "<td colspan='4'></td>"; $echo[] = $showFsInfo ? fs_info($disk,true) : "<td colspan='4'></td>"; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 628-637: In the DISK_NP case remove the duplicated device_info()
calls so only a single
device_info($disk,true,$poolName,$poolstatusData,$options) is appended to the
$echo array for the device info column; ensure the remaining line produces
exactly one <td> worth of output and keep the subsequent lines that append the
"Not present/Not installed" <td>, colspan placeholders and fs_info() logic
untouched so the table HTML remains valid.
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/Helpers.php (1)
1686-1700: Function only returns SATA/SAS disks, excluding NVMe and other device types.The regex
/^sd[a-z]+$/filters out NVMe devices (nvme*), virtio devices (vd*), and other block device types. Since this is used for duplicate disk detection, systems with NVMe drives won't have duplicates detected for those devices.Consider expanding the pattern to include other device types:
♻️ Proposed fix to include NVMe and other device types
function get_block_devices(): array { $out = []; exec('lsblk -ndo NAME 2>/dev/null', $out); $devs = []; foreach ($out as $l) { - if (preg_match('/^sd[a-z]+$/', trim($l))) { + if (preg_match('/^(sd[a-z]+|nvme\d+n\d+|vd[a-z]+|hd[a-z]+)$/', trim($l))) { $devs[] = trim($l); } } sort($devs); return $devs; }emhttp/plugins/dynamix/nchan/device_list (2)
385-393: Remove commented-out debug code.Line 390 contains commented-out debug code that should be removed before merging.
🧹 Proposed cleanup
if (!$online && isset($duplicate_devices[$fullDevName])) { $duplicate = isset($duplicate_devices[$fullDevName]['other_devices']) ? implode("<br>", $duplicate_devices[$fullDevName]['other_devices'])."<br>" : ""; - #$duplicate = json_encode($duplicate_devices[$fullDevName]); $isduplicate = isset($duplicate_devices[$fullDevName]) ? "true" : "false"; $warndup .= "<a class='info'><i class='fa fa-clone fa-fw orange-text'></i><span>".$duplicate._('Duplicate device identifier detected (WWID or Serial)')." </span></a>"; }
391-391: Unused variable$isduplicate.The variable
$isduplicateis assigned but never used. Consider removing it.🧹 Proposed cleanup
if (!$online && isset($duplicate_devices[$fullDevName])) { $duplicate = isset($duplicate_devices[$fullDevName]['other_devices']) ? implode("<br>", $duplicate_devices[$fullDevName]['other_devices'])."<br>" : ""; - $isduplicate = isset($duplicate_devices[$fullDevName]) ? "true" : "false"; $warndup .= "<a class='info'><i class='fa fa-clone fa-fw orange-text'></i><span>".$duplicate._('Duplicate device identifier detected (WWID or Serial)')." </span></a>"; }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Around line 119-130: The "_(Activate and Reboot)_" button handler currently
duplicates the "_(Activate)_" logic (calls validate(), submits the form, closes
the dialog) but never triggers a reboot; update the "_(Activate and Reboot)_"
handler in CacheDevices.page so that after validate() it sets a hidden form
field (e.g., add or set input name="reboot" value="true" on the same form) or
otherwise marks the form payload to indicate a reboot before calling
form.submit() and then closing the dialog; locate the handler by the literal
"_(Activate and Reboot)_" string and the validate() /
$(this).find('form').submit() calls to implement the change.
- Around line 355-369: The device label/value logic can produce a leading-space
label and an empty option value when $devId is empty; update the construction in
the loop (variables $devId, $devName, $devLabel inside the foreach) so that if
$devId is non-empty you use $devId as the primary label and set value to $devId
(append " ($devName)" if $devName present), otherwise set both the option value
and label to $devName (no leading space); ensure you still run htmlspecialchars
on the chosen value and label before echoing.
- Around line 316-321: The page currently renders an "Add Bootable Pool" button
twice and a new variant unconditionally; remove the unconditional duplicate and
gate the "Add Bootable Pool New" button behind the same bootEligible check so
both addBootPoolPopup and addBootPoolPopupNew are only output when
_var($var,'bootEligible') == "yes" (remove the lone input with
onclick="addBootPoolPopup()" and wrap the input with
onclick="addBootPoolPopupNew()" inside the existing <?if
(_var($var,'bootEligible')=="yes"):?> ... <?endif;?> block).
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/CacheDevices.page (1)
146-181: Slot rendering logic is well-structured; minor note on event handler rebinding.The implementation correctly preserves existing selections, clones options from the hidden source, and syncs device availability. One thing to be aware of:
bootPoolSyncDeviceOptionsrebinding.on('change', …)on line 178 will accumulate handlers ifbootPoolRenderSlotsis called multiple times without the DOM elements being removed first. However, since line 155 removes.bootpool-device-rowelements (destroying old selects) before creating new ones, this is safe in practice.Consider using event delegation on
$containerinstead, which would avoid re-binding entirely:Optional: event delegation approach
- $container.find('select[name="poolDevice[]"]').on('change', function(){ - bootPoolSyncDeviceOptions(popup); - }); + // Bind once in bootPoolInit instead: + // popup.find('#bootPoolDeviceSlots').on('change', 'select[name="poolDevice[]"]', function(){ + // bootPoolSyncDeviceOptions(popup); + // });
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Line 395: The tooltip text concatenation currently assigned to $errorLines
(the expression _("Pool name").":".$poolLabel) omits a space after the colon;
update that assignment so the colon is followed by a space (e.g. change the
_("Pool name").":".$poolLabel concatenation to include ": " between the label
and $poolLabel) so tooltips render like "Pool name: Cache".
- Around line 38-44: normalize_device_name currently uses a single regex
'/p?\d+$/' which treats the 'p' as optional and therefore strips the final digit
from NVMe base names like "nvme0n1"; fix by detecting NVMe-style names first
(e.g. preg_match('/^nvme\d+n\d+/', $device) inside normalize_device_name) and
for those only strip a trailing partition token with a required 'p' (use
preg_replace('/p\d+$/', '', $device)), otherwise keep the existing behavior for
non-NVMe devices (use preg_replace('/p?\d+$/', '', $device)).
🧹 Nitpick comments (4)
emhttp/plugins/dynamix/nchan/device_list (4)
64-104: Duplicated pool-resolution logic betweenget_fs_errorsandget_fs_error_tooltip.The device-key normalization →
$devicePoolslookup →find_pools_for_device_in_statusmerge → dedup → member-matching loop is copy-pasted across both functions (Lines 64-101 vs Lines 350-399). Extracting a shared helper (e.g.,resolve_pools_for_device) would keep the two call sites in sync and reduce ~40 lines of duplication.Also,
array_uniqueon Line 85 is redundant —$poolsToCheckwas already deduplicated on Line 80.
300-348: Consider using an options array forpool_function_rowto reduce its 12-parameter signature.The function has 12 positional parameters, most of which are optional with defaults. This makes call sites hard to read and error-prone (see the 12-argument invocations at Lines 1047-1063, 1067-1080, 1163-1179, 1192-1205). An
$optionsassociative array — like you already use fordevice_infoandarray_online— would improve readability and reduce the risk of argument-order bugs.
492-500: Dead code:$isduplicateis assigned but never read; debug line should be removed.Line 498 sets
$isduplicatewhich is unused. Line 497 has a commented-out debug statement.Proposed cleanup
$warndup = ''; $devName = _var($disk,'device',''); $fullDevName = "/dev/" . _var($disk,'device', ''); if (!$online && isset($duplicate_devices[$fullDevName])) { $duplicate = isset($duplicate_devices[$fullDevName]['other_devices']) ? implode("<br>", $duplicate_devices[$fullDevName]['other_devices'])."<br>" : ""; - #$duplicate = json_encode($duplicate_devices[$fullDevName]); - $isduplicate = isset($duplicate_devices[$fullDevName]) ? "true" : "false"; $warndup .= "<a class='info'><i class='fa fa-clone fa-fw orange-text'></i><span>"._('Duplicate device identifier detected (WWID or Serial)')."<br>$duplicate</span></a>"; }
1034-1042: Nit:$bootPoolNameis computed on Line 1034 then identically reassigned on Line 1040.Same pattern occurs in the started-state block at Lines 1140 vs 1144. The second assignment can be removed.
Proposed fix (stopped-state block)
$bootDisk = find_boot_disk_for_pool($pool, $Boot, $Cache); $bootPoolName = prefix(_var($bootDisk,'name','')); $bootMetrics = get_boot_pool_metrics($Boot, $bootPoolName, $bootDisk); $dataMetrics = get_pool_data_metrics($poolDisk, $bootMetrics); $poolLabel = ucfirst($pool); $summaryName = $poolLabel; if (!empty($bootDisk)) { - $bootPoolName = prefix(_var($bootDisk,'name','')); $bootLabel = ucfirst($bootPoolName);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/BootParameters.page`:
- Around line 1088-1157: The parseGrubConfig function only captures
double-quoted menuentry labels; update the menuentry parsing in parseGrubConfig
to accept both single and double quotes (e.g. match
menuentry\s+["']([^"']+)["']) and strip surrounding quotes from the captured
label so labels like menuentry 'My OS' are parsed correctly; keep the existing
fallback that uses trimmed.substring('menuentry '.length).trim() when no quotes
are present and ensure entry.label is stored unquoted so the defaultValue
comparison (which already strips double quotes) works for single-quoted defaults
as well.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Around line 388-410: The file contains a duplicated HTML template with id
"templatePopupSubpool" which causes invalid duplicate IDs and ambiguous
selectors; fix by either removing this duplicate template block or giving it a
unique id (e.g., change id on the duplicate and update any JS/CSS selectors that
reference "templatePopupSubpool"); ensure any references to the template
(selectors used to query the DOM, event handlers, or code that clones the
template) are updated to the new id, and confirm the remaining template keeps
the form fields (select[name="subpool"], select[name="poolSlots"], and the
hidden inputs) so behavior is unchanged.
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 287-302: The GRUB menuentry matching only handles double-quoted
labels; update parse_grub_linux_args, update_grub_entry, and update_grub_default
to accept either single or double quotes by changing the pattern that looks for
"\"" label "\"" to a regex that matches either "'"label"'" or "\""label"\"";
ensure the awk/grep/sed expressions and index/match checks account for both
quote characters (e.g., check for index/$0 ~ /['"]label['"]/ or use a regex
alternation) so menuentry 'Label' and menuentry "Label" are both recognized and
processed.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 1154-1157: The current conditional lets GRUB configs skip
validation; update the logic around BOOTLOADER_TYPE, validate_config, temp_file
and target_cfg so GRUB still undergoes a basic sanity check before mv: when
BOOTLOADER_TYPE == "grub" run a minimal validator (e.g., non-empty and contains
"menuentry") — similar to write_config_grub's checks — and only proceed to mv
"$temp_file" "$target_cfg" if that basic check passes (otherwise abort/log an
error).
- Around line 529-539: The reserved-directive check in the function that uses
lower_param with BOOTLOADER_TYPE incorrectly rejects parameters that merely
start with reserved words (e.g., "setpci") because the regexes (used in the GRUB
branch and the syslinux branch) match prefixes like "set" or "menu" without
requiring a separator; update those regexes to only match exact directive names
or the directive followed by a separator (e.g., end-of-string or '='), so modify
the patterns used in the if [[ "$lower_param" =~ ... ]] checks to include a word
boundary or explicit separator (or anchor with ($|=)) for the reserved tokens
before calling error_exit, ensuring legitimate kernel params like setpci are
allowed.
- Around line 1108-1116: The write_config_grub function currently
unconditionally moves the temp file into GRUB_CFG; change it to validate the
temp file (temp_file) before mv: ensure the file is non-empty and contains at
least one "menuentry" (or other expected grub stanza) and only then perform mv
"$temp_file" "$GRUB_CFG"; if validation fails, do not overwrite GRUB_CFG, log an
error (echo or process logger) and exit with non-zero status so callers detect
failure—leave the original GRUB_CFG intact and surface the failure to the
caller.
- Around line 287-302: The AWK snippet in manage_boot_params.sh incorrectly
embeds a literal single-quote sequence "'" label "'" which breaks the outer
single-quoted shell string and causes AWK to search for the literal text " label
"; fix each occurrence by replacing the problematic "'" label "'" embedding with
an escaped single-quote byte sequence such as "\x27" label "\x27" so the AWK
expression index($0, "\"" label "\"") || index($0, "\x27" label "\x27")
correctly matches both double- and single-quoted menuentry names (apply this
change to all three occurrences of the pattern in the AWK/menuentry matching
logic).
🧹 Nitpick comments (9)
emhttp/plugins/dynamix/CacheDevices.page (3)
79-83:bootPoolInitis a no-op foraddBootPoolPopup.
templatePopupBootPool(lines 412-431) doesn't contain#bootPoolDeviceSlotsor#bootPoolDevicesSource, sobootPoolInit→bootPoolRenderSlots→bootPoolSyncDeviceOptionsall operate on empty jQuery sets. It also needlessly binds achangelistener onpoolSlotsthat re-runs the no-op on every change.Consider guarding or removing this call for the old flow, keeping
bootPoolInitonly foraddBootPoolPopupNew.
169-171: Attribute selector escaping is incomplete — CSS meta-characters in device values could break the lookup.
val.replace(/"/g, '\\"')only escapes double quotes. If a device ID contains],\, or other characters meaningful in CSS selectors, theoption[value="…"]lookup silently fails and the previously selected value is lost.Consider using
$.escapeSelector()(jQuery 3+) or.filter()for a robust match:Proposed fix
if (existing[i]) { var val = existing[i]; - if ($select.find('option[value="' + val.replace(/"/g, '\\"') + '"]').length) { + if ($select.find('option').filter(function(){ return $(this).attr('value') === val; }).length) { $select.val(val); } }
412-431: Consider consolidatingtemplatePopupBootPoolandtemplatePopupBootPoolNew.These two templates share most of their structure (name input, slot select capped at 2, boot size field, same hidden inputs). The "new" template extends the original with device-slot scaffolding. If the intent is for
addBootPoolPopupNewto eventually replaceaddBootPoolPopup, you could simplify maintenance by having a single template and conditionally showing the device section.This is non-blocking — fine to defer until the flows stabilize.
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (6)
483-506: Minor: Inconsistent JSON indentation.Lines 485 and 504 use 4-space indentation while lines 486–503 use 2-space indentation. This doesn't break functionality but makes the JSON output inconsistent.
722-728: Simplify: remove unnecessaryecho "$(...)".
echo "$(command)"is equivalent to just running the pipeline directly. This avoids an extra subshell and is more idiomatic.♻️ Suggested simplification
build_kernel_args() { - echo "$(build_append_line | sed 's/^initrd=\/bzroot[ ]*//')" + build_append_line | sed 's/^initrd=\/bzroot[ ]*//' } build_kernel_args_gui_safe() { - echo "$(build_append_line_gui_safe | sed 's/^initrd=\/bzroot[ ]*//')" + build_append_line_gui_safe | sed 's/^initrd=\/bzroot[ ]*//' }
1014-1039:BEGINblock placement is unconventional; silent return on missing label may hide config errors.The
BEGIN { idx=0 }on line 1026 is placed after the main rule. AWK executesBEGINfirst regardless of position, so this works, but placing it before the main rule is more conventional and readable.More importantly, if the requested
DEFAULT_BOOT_ENTRYlabel doesn't exist in the GRUB config, the function silently returns (line 1030) without any warning. If a user mistypes a label name, the default won't change and there's no feedback. Consider logging a warning.
1189-1196:lsiteration and unescaped dots in sed are fragile but low-risk.Line 1189: ShellCheck SC2045 flags
for backup in $(ls ...)as fragile. Since backup filenames are tightly controlled (no spaces), this is safe in practice.Line 1196: The
sedpattern uses${backup_prefix}without escaping dots, sosyslinux.cfgmatchessyslinuxXcfgin theory. Again, no practical risk given the controlled naming.Both are fine to defer.
1058-1070: ShellCheck SC2155:localmasks return values from command substitutions.Multiple lines in this function (and throughout the file) use
local var=$(cmd). Underset -e, if the command fails,localswallows the non-zero exit code. The highest-risk instance is line 1058 wherebuild_kernel_argsfailure would silently produce empty args that get written to the GRUB config.The idiomatic fix is to separate declaration and assignment:
local new_args new_args=$(build_kernel_args)This pattern applies to lines 530, 636, 711, 1018, 1058, 1059, 1065, 1070, 1149, 1195, and 1196.
1130-1137: Repeated bootloader-type branching for paths could be consolidated.The pattern of setting
target_cfg,target_backup_dir, andbackup_prefixbased onBOOTLOADER_TYPEis duplicated inwrite_raw_config,list_backups,restore_backup, anddelete_all_backups. Consider extracting this to a helper or setting these as global variables alongsideBOOTLOADER_TYPE(near line 39).💡 Example consolidation
# Near line 39, after BOOTLOADER_TYPE is set: if [[ "$BOOTLOADER_TYPE" == "grub" ]]; then ACTIVE_CFG="$GRUB_CFG" ACTIVE_BACKUP_DIR="$GRUB_BACKUP_DIR" BACKUP_PREFIX="grub.cfg" else ACTIVE_CFG="$SYSLINUX_CFG" ACTIVE_BACKUP_DIR="$BACKUP_DIR" BACKUP_PREFIX="syslinux.cfg" fiAlso applies to: 1174-1179, 1214-1221, 1264-1269
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 1101-1108: The current conversion of TIMEOUT deciseconds to GRUB
integer seconds in manage_boot_params.sh uses truncation
(timeout_seconds=$((TIMEOUT / 10))) which drops fractional seconds; change this
to round to the nearest second (e.g. compute timeout_seconds with integer
rounding such as timeout_seconds=$(((TIMEOUT + 5) / 10))) before updating
temp_file so a UI value like 15 deciseconds becomes 2 seconds rather than 1;
update the assignment of timeout_seconds and leave the existing sed logic that
writes/updates "set timeout=" unchanged.
🧹 Nitpick comments (5)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (5)
483-506: Minor: inconsistent JSON indentation.Lines 485 and 504 use 4-space indentation while all other properties use 2-space. This won't affect parsing but is a cosmetic inconsistency.
- "bootloader_type": "$BOOTLOADER_TYPE", + "bootloader_type": "$BOOTLOADER_TYPE",- "timeout": "$timeout" + "timeout": "$timeout"
653-717:build_append_line_gui_safeis nearly identical tobuild_append_line— consider extracting shared logic.The two functions differ only in whether framebuffer parameters are included (lines 574–577). The rest (~60 lines of PCI merging, power management, custom params, and unraiduuid handling) is duplicated verbatim. A single function with a
skip_framebufferflag would eliminate this maintenance burden.
722-728: Uselessecho "$(…)"wrapper — pipe directly.
echo "$(cmd | sed …)"spawns an extra subshell. Simplify:build_kernel_args() { - echo "$(build_append_line | sed 's/^initrd=\/bzroot[ ]*//')" + build_append_line | sed 's/^initrd=\/bzroot[ ]*//' } build_kernel_args_gui_safe() { - echo "$(build_append_line_gui_safe | sed 's/^initrd=\/bzroot[ ]*//')" + build_append_line_gui_safe | sed 's/^initrd=\/bzroot[ ]*//' }
1196-1204: Iterating overlsoutput is fragile (SC2045); also unescaped dots in sed pattern.While backup filenames are tightly controlled,
for backup in $(ls -t ...)will break on whitespace in paths. A safer alternative:- for backup in $(ls -t "$target_backup_dir"/${backup_prefix}.bak.* 2>/dev/null); do + while IFS= read -r -d '' backup; dousing
find ... -print0 | sort -z -rinstead ofls -t.Additionally, on line 1203,
${backup_prefix}(e.g.,grub.cfg) contains literal dots that are unescaped in the sed regex, so they match any character. Low risk here given the naming pattern, but worth noting.
1230-1241: Comments reference only syslinux format, but now GRUB is supported too.The block comment (lines 1231–1239) documents only the
syslinux.cfg.bak.*format/example. Now thatgrub.cfg.bak.*is also valid, the comments should mention both formats or be made generic. Minor documentation nit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Around line 147-188: In bootPoolSizeInit, remove the no-op conditional that
sets $input.val('') when already empty (the check inside syncBootSize) and
simply ensure the custom input is toggled readonly/visibility only (function:
bootPoolSizeInit / syncBootSize; element: $input), change the validation in the
$input 'input change' handler from allowing zero to require positive values
(replace gb >= 0 with gb > 0; references: $input handler, $hidden), and update
the templates that render the poolBootSizeCustomGb numeric input to use HTML min
and max attributes (and remove maxlength) so the browser enforces a valid
numeric range for that input (targets: the poolBootSizeCustomGb input elements
in both template instances).
- Around line 467-497: templatePopupBootPool is missing the device-source and
device-slot containers that bootPoolInit -> bootPoolRenderSlots expects
(`#bootPoolDevicesSource` and `#bootPoolDeviceSlots`), causing slot rendering to
no-op; fix by adding the same device-selection markup (elements with IDs
`#bootPoolDevicesSource` and `#bootPoolDeviceSlots` used by bootPoolRenderSlots)
into templatePopupBootPool so bootPoolRenderSlots can find them, or
alternatively add a guard at the start of bootPoolRenderSlots to early-return if
popup.find('#bootPoolDevicesSource').length===0 ||
popup.find('#bootPoolDeviceSlots').length===0.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/CacheDevices.page (1)
426-438: Inconsistent inline style between the two boot-size<select>elements.Line 427 uses
max-width: 180pxwhile the equivalent select intemplatePopupBootPool(line 483) usesmin-width: 180px. This will cause different sizing behavior between the two templates. Unify to one or the other.Pick one — e.g., min-width in both
-: <span><select name="poolBootSizePreset" class="wide bootpool-size-preset" style="max-width: 180px;"> +: <span><select name="poolBootSizePreset" class="wide bootpool-size-preset" style="min-width: 180px;">
SimonFair
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 1060-1063: The code currently sets $bootFsStatus = 'Mounted'
whenever $bootFsStatus is empty but $bootFsType is non-empty, which wrongly
assumes a filesystem is mounted; instead, update the logic in both places where
this occurs (the blocks using get_boot_pool_fs_info / $bootFsInfo and $bootDisk)
to first try to determine actual mount state (e.g., check an existing
mountPoint/_var($bootDisk,'mountPoint'), call any available
is_mounted/get_mount_status helper, or inspect /proc/mounts) and only set
$bootFsStatus based on that result; if no definitive mount info is available
leave $bootFsStatus empty or set to a neutral value like 'Unknown' rather than
defaulting to 'Mounted'.
- Around line 496-504: The warning tooltip concatenates raw values from
$duplicate_devices[$fullDevName]['other_devices'] into HTML via implode("<br>",
...) which can lead to HTML injection; before joining, htmlspecialchars each
element (use ENT_QUOTES) to escape <, >, " etc., then implode with "<br>" and
use that escaped string when building $duplicate and $warndup (keep existing
htmlspecialchars usage for $dupValue and reuse the same escaping approach).
🧹 Nitpick comments (4)
emhttp/plugins/dynamix/nchan/device_list (4)
82-88: Redundantarray_uniquecall.Line 83 already deduplicates
$poolsToCheckviaarray_unique(array_filter(...)). Line 88 wraps it inarray_unique()again unnecessarily. The same pattern also appears inget_fs_error_tooltip(lines 370-372).Proposed fix
- foreach (array_unique($poolsToCheck) as $pool) { + foreach ($poolsToCheck as $pool) {
303-304: Consider consolidatingpool_function_rowparameters into an options array.This function takes 12 positional parameters, many of them booleans. Call sites (e.g., lines 1064–1080, 1084–1097, 1180–1196, 1209–1222) are difficult to read because the meaning of each positional
true/false/''is opaque without checking the signature. An associative$optionsarray (similar to howdevice_infoalready works) would improve readability and make future additions non-breaking.
1041-1098: Significant code duplication between Stopped and Started pool rendering branches.The Stopped branch (lines ~1041–1098) and Started branch (lines ~1154–1222) share nearly identical logic: computing boot/pool metrics, building links and summary names, generating boot/data rows via
pool_function_row. Consider extracting a shared helper (e.g.,render_pool_partition_rows(...)) to avoid maintaining the same logic in two places.
222-238: Inconsistency betweenget_pool_boot_metricsandget_boot_pool_metrics.These two functions have very similar names but different purposes.
get_pool_boot_metricstakes a single disk,get_boot_pool_metricstakes an array of disks and a pool name. The naming is confusing — consider renaming one to clearly distinguish them (e.g.,get_single_boot_disk_metricsvsget_boot_pool_metrics).
Revise boot entry processing.
|
@CodeRabbit please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
emhttp/plugins/dynamix/CacheDevices.page (3)
247-247: Boot reserved size is capped at half the smallest device — validate this business rule.Line 247 caps
maxMiBatMath.floor(minMiB / 2), meaning the boot partition can use at most 50% of the smallest selected drive. This is a significant constraint that isn't documented in the UI. Consider adding a tooltip or note near the size selector explaining why certain options become hidden when smaller devices are selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/CacheDevices.page` at line 247, The code sets maxMiB to Math.floor(minMiB / 2) which enforces a business rule capping boot reserved size at 50% of the smallest device (variable: maxMiB, source: minMiB) without any UI explanation; update the UI near the size selector (the component/markup that reads maxMiB/minMiB) to display a tooltip or inline note explaining “Boot reserved size is capped at 50% of the smallest selected drive” and ensure any disabled/hidden options that result from this cap show the explanatory message so users understand why options change when smaller devices are chosen.
374-377:cmdPreviewis constructed but never used.These lines build a command preview string that is never displayed or referenced anywhere else. Remove this dead code.
- var cmdArgs = [selection.poolName, selection.bootSize].concat(selection.devices); - if (reboot) cmdArgs.push('reboot'); - cmdArgs.push('update'); - var cmdPreview = "/plugins/dynamix/scripts/mkbootpool " + cmdArgs.join(' ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/CacheDevices.page` around lines 374 - 377, Remove the unused command preview string: delete the construction and assignment of cmdPreview (the line "var cmdPreview = \"/plugins/dynamix/scripts/mkbootpool \" + cmdArgs.join(' ');") so no dead variable remains; keep the existing cmdArgs building and pushes (used elsewhere) and ensure no other references to cmdPreview exist before committing.
598-599: Boot pool form has nomethodoractionand relies entirely on JS prevention.The
<form>tag on line 599 hastarget="progressFrame"but nomethodoraction. If the JSsubmitprevention (line 109-111) fails for any reason (JS error, race condition), the form would submit as a GET to the current page URL, potentially exposing form data in the URL. Add explicit attributes for defense-in-depth:Proposed fix
-<form markdown="1" target="progressFrame" > +<form markdown="1" method="POST" action="javascript:void(0)">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/CacheDevices.page` around lines 598 - 599, The form in templatePopupBootPool currently has no method or action and relies solely on JS submit-prevention (see the submit handler around lines 109-111); to harden against JS failures, update the <form> in templatePopupBootPool to include explicit attributes such as method="post" and action="javascript:void(0);" (or action="#") so it won’t perform a GET to the current URL if JS fails, and keep the existing JS submit handling intact.emhttp/plugins/dynamix/scripts/mkbootpool (1)
126-128: Debug mode is always enabled unconditionally.
run_emcmd "debug=cmdCreatePool,cmdAssignDisk,cmdMakeBootable"runs on every invocation. This likely enables verbose/debug logging inemcmdand should be gated behind a flag or removed before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/scripts/mkbootpool` around lines 126 - 128, The script unconditionally enables emcmd debug by calling run_emcmd "debug=cmdCreatePool,cmdAssignDisk,cmdMakeBootable" (and logging via log_action "debug"...), so either remove that call or gate it behind a debug flag; modify the mkbootpool logic around the run_emcmd and log_action calls to only execute when a DEBUG/--debug flag (or an environment variable like DEBUG=1) is set, e.g., check the chosen debug variable before invoking run_emcmd and logging, or drop the debug invocation entirely for production.emhttp/plugins/dynamix/nchan/device_list (3)
309-329: Pool status text is interpolated into HTML without escaping.On lines 322 and 325,
_($status_text)is embedded in thetitleattribute and link text. While$status_textcomes from parsed pool status data ($poolstatusData['pools'][$poolName]['overall_status']), if this value ever contains quotes or HTML entities, it could break the HTML structure. Usehtmlspecialcharson the output:Proposed fix
if (!$isOnline) { - return " <a href='$deviceUrl' title='"._('View pool details')."'>("._($status_text).")</a><span title='"._('Check pool status')."'><i class='fa fa-warning fa-fw orange-text'></i></span>"; + return " <a href='$deviceUrl' title='".htmlspecialchars(_('View pool details'), ENT_QUOTES)."'>(".htmlspecialchars(_($status_text), ENT_QUOTES).")</a><span title='".htmlspecialchars(_('Check pool status'), ENT_QUOTES)."'><i class='fa fa-warning fa-fw orange-text'></i></span>"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/nchan/device_list` around lines 309 - 329, In pool_status_html, the unescaped translated status text ($status_text and $displayText) is interpolated into HTML title attributes and link text; wrap the translated strings (e.g., _($status_text) and $displayText) with htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before embedding to prevent quote/HTML injection and broken markup (update uses in the non‑online branch, hasErrors branch, and the final return); keep deviceUrl as-is (urlencode already used).
1079-1137: Significant code duplication between stopped-state and started-state pool rendering.The boot partition discovery and row generation logic (~lines 1079–1137 for stopped, ~lines 1193–1266 for started) is nearly identical, with the started-state version adding browse links and flash SMB warnings. This duplication increases the risk of the two paths diverging when one is updated but not the other (which has already happened — the stopped-state path lacks the browse links and flash warning).
Consider extracting the common boot/data partition row logic into a shared helper function that accepts a mode parameter.
Also applies to: 1193-1266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/nchan/device_list` around lines 1079 - 1137, The stopped vs started pool rendering duplicates boot/data partition discovery and row creation (see use of get_pool_first_member, find_boot_disk_for_pool, get_boot_pool_metrics, get_pool_data_metrics, get_boot_pool_fs_info, get_mount_status_from_mountpoint and pool_function_row); extract that common logic into a single helper (e.g., render_pool_partition_rows or build_pool_rows) which accepts the pool name, $Boot/$Cache/$disks context and a mode/options parameter (e.g., mode => 'stopped'|'started' or flags like addBrowseLinks, addFlashSmbWarning) and returns the $bootRow/$dataRow (or an array of rows) so both the stopped and started code paths call this helper, with the started caller enabling browse links and flash SMB warning while the stopped caller keeps them off. Ensure the helper reproduces the existing behavior: determine $poolDisk, $firstMember, $bootDisk, compute $bootFsType/$bootFsStatus (using get_boot_pool_fs_info and get_mount_status_from_mountpoint fallback), build $summaryName and call pool_function_row with correct flags, and keep the callers responsible only for mode-specific flags and adding non-duplicated wrapping logic.
563-565: Remove commented-out debug code.Line 564 contains a commented-out debug
$poolWarningassignment. This should be cleaned up before merge.- # $poolWarning = "<span title='".sprintf(_('Pool %s Device has an error status: %s'), $poolName,(!empty($poolstatusData['pools'][$poolName]) ? "NotEmpty" : "Empty"))."'><i class='fa fa-warning fa-fw red-text'></i></span>";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/nchan/device_list` around lines 563 - 565, Remove the commented-out debug assignment for $poolWarning (the line starting with "// Add red warning triangle..." and the commented //$poolWarning = "...") so only the intended $poolWarning = '' remains; locate the $poolWarning variable in the device_list block (the assignment and its commented variant) and delete the commented debug line to clean up the code.emhttp/plugins/dynamix/include/mkbootpool.php (1)
185-193: Writing to/boot/config/persists logs on the USB flash drive.
/boot/config/internal_boot/output.logis written on every invocation. The/boot/mount is the USB flash drive in Unraid, and frequent writes can degrade flash storage. Consider writing to a tmpfs location (e.g.,/tmp/) for transient logs, and only persisting to/boot/config/when configuration state needs to survive reboots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/include/mkbootpool.php` around lines 185 - 193, The code currently always writes transient logs to $outputDir = '/boot/config/internal_boot' using `@mkdir` and `@file_put_contents`('/boot/config/internal_boot/output.log', ...) which causes frequent writes to the USB-mounted /boot; change the transient logging path to a tmpfs location (e.g. '/tmp/internal_boot/output.log') for the regular `@file_put_contents` and only write to '/boot/config/internal_boot/output.log' when explicitly persisting configuration state (add an explicit flag or check before writing to $outputDir '/boot/config/internal_boot'); update the references around the mkdir, file_put_contents and the echo json_encode response so output still returns the command result but persistent writes are done conditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Around line 364-407: The confirmation dialog concatenates unescaped
user-controlled strings in bootPoolConfirmActivate (notably selection.devices
items and selection.poolName) which are rendered with html: true; update the
function to pass all user-derived values through bootPoolEscapeHtml before
building deviceList and the text (e.g., escape each selection.devices[i] when
appending to deviceList and escape selection.poolName where used); leave
bootPoolFormatBootSizeGb and selection.updateBios handling as-is but ensure any
other displayed selection fields are similarly escaped to prevent XSS.
In `@emhttp/plugins/dynamix/include/mkbootpool.php`:
- Around line 163-176: The boot-order strings ($desiredOrder -> $orderList and
$nextBoot) are interpolated into $orderCmd and passed to exec() unsafely;
sanitize them by applying escapeshellarg to the entire comma-joined $orderList
and to $nextBoot when building $orderCmd (i.e. use escapeshellarg($orderList)
and escapeshellarg($nextBoot) in the $orderCmd construction) so the exec(...)
call only receives properly escaped arguments and prevents command injection.
- Around line 82-90: The loop over $bootLabelMap currently deletes every EFI
entry; update the logic in the foreach (referencing $bootLabelMap, $labelText,
$bootNum and $deleteCmd) to only attempt deletion when the label matches Unraid
(e.g., case-insensitive match for "Unraid" or a stricter regex) and skip/log any
non-Unraid entries instead of deleting them; ensure skipped entries are recorded
in $output and that deletion still uses the existing exec flow for matched
entries.
- Around line 57-64: The current construction uses escapeshellcmd($script) then
escapeshellarg() for each element of $args which mixes escaping strategies;
change to use escapeshellarg() for the script and keep escapeshellarg() for all
arguments so the entire command is composed of individually quoted/escaped
tokens (update the code that builds $cmd from $script and $args), ensuring
$script has already been validated via is_file() before calling exec($cmd.'
2>&1', $output, $rc).
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 331-379: pool_function_row currently mutates $disk (derived from
$poolDisk or $firstMember) which can modify the shared $disks entries; make a
local, non-referenced copy before any changes (e.g. after computing $disk from
$poolDisk/$firstMember, copy it via an array copy) and perform all subsequent
modifications and reads against that copy; ensure calls to device_info(...) and
fs_info(...) use the copied $disk so the global $disks, $poolDisk and
$firstMember are not modified.
- Around line 225-226: The function get_mount_status_from_mountpoint uses a
static $mounts cache that never refreshes, causing stale mount info; change
$mounts to be non-static (or add a $refresh parameter to
get_mount_status_from_mountpoint that forces reload) so the mount list is
re-read on demand, update the function to reload /proc/mounts when $refresh is
true (or simply on every call), and ensure the polling loop calls
get_mount_status_from_mountpoint with $refresh=true (or otherwise resets the
cache) once per iteration so mount state reflects recent changes.
In `@emhttp/plugins/dynamix/scripts/mkbootpool`:
- Around line 8-10: Fix the typos in the usage comment block in mkbootpool:
change "pool-nmae" to "pool-name" and "asssign" to "assign" so the lines
describing "pool-name", "boot-size" and "device-idN" read correctly; update the
comment text that currently references "pool-nmae" and "asssign" to the
corrected spellings.
- Around line 155-158: The script's reboot branch (in mkbootpool) never actually
reboots because the reboot command is commented out; either uncomment the reboot
call so that when dryRun=false and rebootRequested=true the script executes
reboot, or if rebooting is handled by the PHP layer (mkbootpool.php /
bootPoolRebootNow()) remove the reboot flag/rebootRequested logic and change the
echo message to indicate reboot will be performed by the PHP layer (and update
mkbootpool.php to rely on the script exit code only). Ensure changes touch the
conditional that checks dryRun and rebootRequested and the echo at that location
so behavior and messaging are consistent.
- Around line 61-78: The empty-array expansion of filtered can trigger an
"unbound variable" with set -u on older Bash; modify the code around the set --
invocation so it checks filtered's length and only expands when non-empty (e.g.
test ${`#filtered`[@]} > 0), otherwise call set -- with no arguments to clear
positional parameters; refer to the filtered array and the set --
"${filtered[@]}" line and implement the conditional guard to avoid expanding an
empty array.
---
Duplicate comments:
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Around line 94-99: The JS buttons now call bootPoolConfirmActivate(popup,
false/true) which triggers bootPoolSubmitMkbootpool and, when reboot=true,
bootPoolRebootNow(); however the mkbootpool script still has its reboot command
commented out (line ~156) so there are two potential reboot paths; confirm
intended behavior and make them consistent by either uncommenting the reboot in
the mkbootpool script (so the script performs the reboot) or removing/commenting
out the script reboot and relying solely on the JS path (bootPoolRebootNow and
Boot.php) — update mkbootpool or the JS flow accordingly and ensure any
duplicated reboot logic is removed to avoid double reboots.
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 425-428: The tooltip concatenation missing a space is in the block
that builds $errorLines (uses $poolLines, $errorLines and $poolLabel); update
the concatenation in that block (where _("Pool name").":".$poolLabel is used) to
include a space after the colon (e.g. use ": " or format with sprintf) so the
result becomes "Pool name: Cache" instead of "Pool name:Cache".
- Around line 792-801: In the DISK_NP switch block remove the three duplicate
calls to device_info so only a single
"<td>".device_info($disk,true,$poolName,$poolstatusData,$options)."</td>"
remains (locate the case 'DISK_NP' block and the consecutive device_info(...)
lines), and verify the surrounding table cells (the subsequent "<td><a
...></td>", the "<td colspan='4'></td>" and the $showFsInfo ?
fs_info($disk,true) : "<td colspan='4'></td>") still produce the correct column
count after removing the duplicates; adjust colspans if necessary.
---
Nitpick comments:
In `@emhttp/plugins/dynamix/CacheDevices.page`:
- Line 247: The code sets maxMiB to Math.floor(minMiB / 2) which enforces a
business rule capping boot reserved size at 50% of the smallest device
(variable: maxMiB, source: minMiB) without any UI explanation; update the UI
near the size selector (the component/markup that reads maxMiB/minMiB) to
display a tooltip or inline note explaining “Boot reserved size is capped at 50%
of the smallest selected drive” and ensure any disabled/hidden options that
result from this cap show the explanatory message so users understand why
options change when smaller devices are chosen.
- Around line 374-377: Remove the unused command preview string: delete the
construction and assignment of cmdPreview (the line "var cmdPreview =
\"/plugins/dynamix/scripts/mkbootpool \" + cmdArgs.join(' ');") so no dead
variable remains; keep the existing cmdArgs building and pushes (used elsewhere)
and ensure no other references to cmdPreview exist before committing.
- Around line 598-599: The form in templatePopupBootPool currently has no method
or action and relies solely on JS submit-prevention (see the submit handler
around lines 109-111); to harden against JS failures, update the <form> in
templatePopupBootPool to include explicit attributes such as method="post" and
action="javascript:void(0);" (or action="#") so it won’t perform a GET to the
current URL if JS fails, and keep the existing JS submit handling intact.
In `@emhttp/plugins/dynamix/include/mkbootpool.php`:
- Around line 185-193: The code currently always writes transient logs to
$outputDir = '/boot/config/internal_boot' using `@mkdir` and
`@file_put_contents`('/boot/config/internal_boot/output.log', ...) which causes
frequent writes to the USB-mounted /boot; change the transient logging path to a
tmpfs location (e.g. '/tmp/internal_boot/output.log') for the regular
`@file_put_contents` and only write to '/boot/config/internal_boot/output.log'
when explicitly persisting configuration state (add an explicit flag or check
before writing to $outputDir '/boot/config/internal_boot'); update the
references around the mkdir, file_put_contents and the echo json_encode response
so output still returns the command result but persistent writes are done
conditionally.
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 309-329: In pool_status_html, the unescaped translated status text
($status_text and $displayText) is interpolated into HTML title attributes and
link text; wrap the translated strings (e.g., _($status_text) and $displayText)
with htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before embedding to prevent
quote/HTML injection and broken markup (update uses in the non‑online branch,
hasErrors branch, and the final return); keep deviceUrl as-is (urlencode already
used).
- Around line 1079-1137: The stopped vs started pool rendering duplicates
boot/data partition discovery and row creation (see use of
get_pool_first_member, find_boot_disk_for_pool, get_boot_pool_metrics,
get_pool_data_metrics, get_boot_pool_fs_info, get_mount_status_from_mountpoint
and pool_function_row); extract that common logic into a single helper (e.g.,
render_pool_partition_rows or build_pool_rows) which accepts the pool name,
$Boot/$Cache/$disks context and a mode/options parameter (e.g., mode =>
'stopped'|'started' or flags like addBrowseLinks, addFlashSmbWarning) and
returns the $bootRow/$dataRow (or an array of rows) so both the stopped and
started code paths call this helper, with the started caller enabling browse
links and flash SMB warning while the stopped caller keeps them off. Ensure the
helper reproduces the existing behavior: determine $poolDisk, $firstMember,
$bootDisk, compute $bootFsType/$bootFsStatus (using get_boot_pool_fs_info and
get_mount_status_from_mountpoint fallback), build $summaryName and call
pool_function_row with correct flags, and keep the callers responsible only for
mode-specific flags and adding non-duplicated wrapping logic.
- Around line 563-565: Remove the commented-out debug assignment for
$poolWarning (the line starting with "// Add red warning triangle..." and the
commented //$poolWarning = "...") so only the intended $poolWarning = ''
remains; locate the $poolWarning variable in the device_list block (the
assignment and its commented variant) and delete the commented debug line to
clean up the code.
In `@emhttp/plugins/dynamix/scripts/mkbootpool`:
- Around line 126-128: The script unconditionally enables emcmd debug by calling
run_emcmd "debug=cmdCreatePool,cmdAssignDisk,cmdMakeBootable" (and logging via
log_action "debug"...), so either remove that call or gate it behind a debug
flag; modify the mkbootpool logic around the run_emcmd and log_action calls to
only execute when a DEBUG/--debug flag (or an environment variable like DEBUG=1)
is set, e.g., check the chosen debug variable before invoking run_emcmd and
logging, or drop the debug invocation entirely for production.
| foreach ($bootLabelMap as $labelText => $bootNum) { | ||
| $deleteCmd = 'efibootmgr -b '.escapeshellarg($bootNum).' -B'; | ||
| $output[] = 'Running: '.$deleteCmd; | ||
| $deleteOut = []; | ||
| $deleteRc = 0; | ||
| exec($deleteCmd.' 2>&1', $deleteOut, $deleteRc); | ||
| if (!empty($deleteOut)) $output = array_merge($output, $deleteOut); | ||
| if ($deleteRc !== 0) $output[] = 'efibootmgr failed to delete boot entry '.$bootNum.' (rc='.$deleteRc.')'; | ||
| } |
There was a problem hiding this comment.
Deleting ALL EFI boot entries is extremely destructive.
This loop deletes every EFI boot entry on the system, not just Unraid-related ones. If the machine dual-boots or has OEM recovery entries, firmware update entries, or other OS boot entries, they will all be permanently wiped. This could render the system unbootable if the Unraid entry creation subsequently fails.
Consider filtering to only delete entries matching "Unraid" labels:
Proposed fix
foreach ($bootLabelMap as $labelText => $bootNum) {
+ if (stripos($labelText, 'Unraid') === false) continue;
$deleteCmd = 'efibootmgr -b '.escapeshellarg($bootNum).' -B';📝 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.
| foreach ($bootLabelMap as $labelText => $bootNum) { | |
| $deleteCmd = 'efibootmgr -b '.escapeshellarg($bootNum).' -B'; | |
| $output[] = 'Running: '.$deleteCmd; | |
| $deleteOut = []; | |
| $deleteRc = 0; | |
| exec($deleteCmd.' 2>&1', $deleteOut, $deleteRc); | |
| if (!empty($deleteOut)) $output = array_merge($output, $deleteOut); | |
| if ($deleteRc !== 0) $output[] = 'efibootmgr failed to delete boot entry '.$bootNum.' (rc='.$deleteRc.')'; | |
| } | |
| foreach ($bootLabelMap as $labelText => $bootNum) { | |
| if (stripos($labelText, 'Unraid') === false) continue; | |
| $deleteCmd = 'efibootmgr -b '.escapeshellarg($bootNum).' -B'; | |
| $output[] = 'Running: '.$deleteCmd; | |
| $deleteOut = []; | |
| $deleteRc = 0; | |
| exec($deleteCmd.' 2>&1', $deleteOut, $deleteRc); | |
| if (!empty($deleteOut)) $output = array_merge($output, $deleteOut); | |
| if ($deleteRc !== 0) $output[] = 'efibootmgr failed to delete boot entry '.$bootNum.' (rc='.$deleteRc.')'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@emhttp/plugins/dynamix/include/mkbootpool.php` around lines 82 - 90, The loop
over $bootLabelMap currently deletes every EFI entry; update the logic in the
foreach (referencing $bootLabelMap, $labelText, $bootNum and $deleteCmd) to only
attempt deletion when the label matches Unraid (e.g., case-insensitive match for
"Unraid" or a stricter regex) and skip/log any non-Unraid entries instead of
deleting them; ensure skipped entries are recorded in $output and that deletion
still uses the existing exec flow for matched entries.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…s://github.com/SimonFair/webgui into Feat(Internal_Boot)-add-boot-pages-and-options
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes