Added UnRAID VM template.#2006
Conversation
…will create a VM with with a 2gb virtual USB drive with UnRAID installed.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThe update enhances the UnRAID virtual machine management interface by introducing a comprehensive PHP form for creating, updating, and deleting virtual machines (VMs). It establishes the document root and retrieves configurations such as machine types and devices. The form allows users to specify VM parameters and supports both XML and form-based input. AJAX requests manage VM operations, including image downloads and disk space checks, with error handling for various tasks. The interface features dynamic updates based on user input, improving interaction through styling and JavaScript enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as VM Manager Form
participant Backend as Server
participant Resources as Resource Validator
User->>UI: Input VM parameters (name, memory, etc.)
UI->>Backend: Submit form via AJAX (create/update/delete VM)
Backend->>Resources: Validate disk space, device availability, and paths
Resources-->>Backend: Return validation results
Backend-->>UI: Return operation status and feedback
UI->>User: Display dynamic feedback and errors
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/templates/UnRAID.form.php (2)
1076-1078: Remove commented-out code to improve clarity.
These lines (1076-1078) contain commented logic that’s likely obsolete:- // $extra .= ' disabled="disabled"';Removing dead or commented-out code clarifies the intended behavior for device pass-through.
1-14: Add unit tests for new VM template logic.
All code here is new, and it’s critical to ensure coverage (download process, disk creation, etc.). Unit tests or integration tests will protect against regressions.I can help create a set of PHPUnit or functional tests. Would you like me to open a separate PR for test coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/templates/UnRAID.form.php(1 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix.vm.manager/templates/UnRAID.form.php (1)
282-307: Validate paths and command outputs for system calls.
The process that creates and updates VMs (lines 282-307, 309-405) calls shell commands (e.g., parted, dd, unzip) but lacks robust error checking or rollback handling if any subcommand fails. This might lead to inconsistent states.Would you like me to generate a shell script that simulates partial failures (disk full, missing parted, etc.) to verify how the code handles these conditions?
Also applies to: 309-405
| <input type="number" size="5" maxlength="5" id="nic[<?=$i?>][boot]" class="narrow bootorder" <?=$bootdisable?> style="width: 50px;" name="nic[<?=$i?>][boot]" title="_(Boot order)_" value="<?=$arrNic['boot']?>" > | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| <?if ($i == 0) {?> |
There was a problem hiding this comment.
Avoid using undefined $bootdisable in NIC and related fields.
The $bootdisable variable is only set for other PCI devices, yet NIC and USB sections also reference . This triggers undefined variable notices and prevents correct boot order logic.
Remove the invalid references or ensure $bootdisable is set before use:
- <input type="number" ... class="narrow bootorder" <?=$bootdisable?> ...
+ <input type="number" ... class="narrow bootorder" ...Also applies to: 1000-1023
| $oldDir = $domain_cfg['DOMAINDIR'].$oldName; | ||
| $newDir = $domain_cfg['DOMAINDIR'].$newdName; | ||
| if ($oldName && $newName && is_dir($oldDir) && !is_dir($newDir)) { | ||
| // mv domain/vmname folder | ||
| if (rename($oldDir, $newDir)) { | ||
| // replace all disk paths in xml | ||
| foreach ($_POST['disk'] as &$arrDisk) { | ||
| if ($arrDisk['new']) $arrDisk['new'] = str_replace($oldDir, $newDir, $arrDisk['new']); | ||
| if ($arrDisk['image']) $arrDisk['image'] = str_replace($oldDir, $newDir, $arrDisk['image']); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the undeclared variable $newdName.
Within the block that renames the domain folder (lines 359-371), $newdName is used instead of $newName, causing a potential “undefined variable” bug:
Apply this diff to use the correct variable:
- $newDir = $domain_cfg['DOMAINDIR'].$newdName;
+ $newDir = $domain_cfg['DOMAINDIR'].$newName;📝 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.
| $oldDir = $domain_cfg['DOMAINDIR'].$oldName; | |
| $newDir = $domain_cfg['DOMAINDIR'].$newdName; | |
| if ($oldName && $newName && is_dir($oldDir) && !is_dir($newDir)) { | |
| // mv domain/vmname folder | |
| if (rename($oldDir, $newDir)) { | |
| // replace all disk paths in xml | |
| foreach ($_POST['disk'] as &$arrDisk) { | |
| if ($arrDisk['new']) $arrDisk['new'] = str_replace($oldDir, $newDir, $arrDisk['new']); | |
| if ($arrDisk['image']) $arrDisk['image'] = str_replace($oldDir, $newDir, $arrDisk['image']); | |
| } | |
| } | |
| } | |
| } | |
| $oldDir = $domain_cfg['DOMAINDIR'].$oldName; | |
| $newDir = $domain_cfg['DOMAINDIR'].$newName; | |
| if ($oldName && $newName && is_dir($oldDir) && !is_dir($newDir)) { | |
| // mv domain/vmname folder | |
| if (rename($oldDir, $newDir)) { | |
| // replace all disk paths in xml | |
| foreach ($_POST['disk'] as &$arrDisk) { | |
| if ($arrDisk['new']) $arrDisk['new'] = str_replace($oldDir, $newDir, $arrDisk['new']); | |
| if ($arrDisk['image']) $arrDisk['image'] = str_replace($oldDir, $newDir, $arrDisk['image']); | |
| } | |
| } | |
| } |
| if (array_key_exists($arrConfig['template']['unraid'], $arrLibreELECVersions)) { | ||
| $arrConfigDefaults['disk'][0]['image'] = $arrLibreELECVersions[$arrConfig['template']['unraid']]['localpath']; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or replace the unused $arrLibreELECVersions reference.
The condition uses $arrLibreELECVersions but this variable is never declared in the file. This may be leftover logic from another template.
- if (array_key_exists($arrConfig['template']['unraid'], $arrLibreELECVersions)) {
- $arrConfigDefaults['disk'][0]['image'] = $arrLibreELECVersions[$arrConfig['template']['unraid']]['localpath'];
- }Remove or update it to a relevant array to avoid undefined variable errors.
📝 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 (array_key_exists($arrConfig['template']['unraid'], $arrLibreELECVersions)) { | |
| $arrConfigDefaults['disk'][0]['image'] = $arrLibreELECVersions[$arrConfig['template']['unraid']]['localpath']; | |
| } |
| <label for="usb<?=$i?>">    <input type="checkbox" name="usb[]" id="usb<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if (count(array_filter($arrConfig['usb'], function($arr) use ($arrDev) { return ($arr['id'] == $arrDev['id']); }))) echo 'checked="checked"';?> | ||
| />         <input type="checkbox" name="usbopt[<?=htmlspecialchars($arrDev['id'])?>]]" id="usbopt<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if ($arrDev["startupPolicy"] =="optional") echo 'checked="checked"';?>/>      | ||
| <input type="number" size="5" maxlength="5" id="usbboot<?=$i?>" class="narrow bootorder" <?=$bootdisable?> style="width: 50px;" name="usbboot[<?=htmlspecialchars($arrDev['id'])?>]]" title="_(Boot order)_" value="<?=$arrDev['usbboot']?>" > | ||
| <?=htmlspecialchars($arrDev['name'])?> (<?=htmlspecialchars($arrDev['id'])?>)</label><br/> | ||
| <? |
There was a problem hiding this comment.
Correct extra bracket in name attributes for USB checkboxes.
The form fields usbopt[...] and usbboot[...] have an extra bracket ("]]"), which could cause issues persisting or retrieving data from POST.
- <input type="checkbox" name="usbopt[<?=htmlspecialchars($arrDev['id'])?>]]" ...
- <input type="number" ... name="usbboot[<?=htmlspecialchars($arrDev['id'])?>]]" ...
+ <input type="checkbox" name="usbopt[<?=htmlspecialchars($arrDev['id'])?>]" ...
+ <input type="number" ... name="usbboot[<?=htmlspecialchars($arrDev['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.
| <label for="usb<?=$i?>">    <input type="checkbox" name="usb[]" id="usb<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if (count(array_filter($arrConfig['usb'], function($arr) use ($arrDev) { return ($arr['id'] == $arrDev['id']); }))) echo 'checked="checked"';?> | |
| />         <input type="checkbox" name="usbopt[<?=htmlspecialchars($arrDev['id'])?>]]" id="usbopt<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if ($arrDev["startupPolicy"] =="optional") echo 'checked="checked"';?>/>      | |
| <input type="number" size="5" maxlength="5" id="usbboot<?=$i?>" class="narrow bootorder" <?=$bootdisable?> style="width: 50px;" name="usbboot[<?=htmlspecialchars($arrDev['id'])?>]]" title="_(Boot order)_" value="<?=$arrDev['usbboot']?>" > | |
| <?=htmlspecialchars($arrDev['name'])?> (<?=htmlspecialchars($arrDev['id'])?>)</label><br/> | |
| <? | |
| <label for="usb<?=$i?>">    <input type="checkbox" name="usb[]" id="usb<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if (count(array_filter($arrConfig['usb'], function($arr) use ($arrDev) { return ($arr['id'] == $arrDev['id']); }))) echo 'checked="checked"';?> | |
| />         <input type="checkbox" name="usbopt[<?=htmlspecialchars($arrDev['id'])?>]" id="usbopt<?=$i?>" value="<?=htmlspecialchars($arrDev['id'])?>" <?if ($arrDev["startupPolicy"] =="optional") echo 'checked="checked"';?>/>      | |
| <input type="number" size="5" maxlength="5" id="usbboot<?=$i?>" class="narrow bootorder" <?=$bootdisable?> style="width: 50px;" name="usbboot[<?=htmlspecialchars($arrDev['id'])?>]" title="_(Boot order)_" value="<?=$arrDev['usbboot']?>" > | |
| <?=htmlspecialchars($arrDev['name'])?> (<?=htmlspecialchars($arrDev['id'])?>)</label><br/> | |
| <? |
| if (isset($_POST['delete_version'])) { | ||
| $arrDeleteUnRAID = []; | ||
| if (array_key_exists($_POST['delete_version'], $arrUnRAIDVersions)) { | ||
| $arrDeleteUnRAID = $arrUnRAIDVersions[$_POST['delete_version']]; | ||
| } | ||
| $reply = []; | ||
| if (empty($arrDeleteUnRAID)) { | ||
| $reply = ['error' => 'Unknown version: ' . $_POST['delete_version']]; | ||
| } else { | ||
| // delete img file | ||
| @unlink($arrDeleteUnRAID['localpath']); | ||
|
|
||
| // Save to strUnRAIDConfig | ||
| unset($arrUnRAIDConfig[$_POST['delete_version']]); | ||
| $text = ''; | ||
| foreach ($arrUnRAIDConfig as $key => $value) $text .= "$key=\"$value\"\n"; | ||
| file_put_contents($strUnRAIDConfig, $text); | ||
| $reply = ['status' => 'ok']; | ||
| } | ||
|
|
||
| echo json_encode($reply); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Consider adding CSRF protection for delete and download operations.
The code immediately processes POST requests for deleting or downloading specific resources (lines 57-79 and also 81-197) without any CSRF token checks. This can potentially allow unauthorized requests to manipulate system files.
You could mitigate this by verifying a CSRF token or nonce before processing sensitive operations:
+ // Example snippet for CSRF token check
+ if (!hash_equals($_SESSION['csrf_token'], $_POST['csrf_token'] ?? '')) {
+ http_response_code(403);
+ echo json_encode(['error' => 'Invalid CSRF token']);
+ exit;
+ }📝 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 (isset($_POST['delete_version'])) { | |
| $arrDeleteUnRAID = []; | |
| if (array_key_exists($_POST['delete_version'], $arrUnRAIDVersions)) { | |
| $arrDeleteUnRAID = $arrUnRAIDVersions[$_POST['delete_version']]; | |
| } | |
| $reply = []; | |
| if (empty($arrDeleteUnRAID)) { | |
| $reply = ['error' => 'Unknown version: ' . $_POST['delete_version']]; | |
| } else { | |
| // delete img file | |
| @unlink($arrDeleteUnRAID['localpath']); | |
| // Save to strUnRAIDConfig | |
| unset($arrUnRAIDConfig[$_POST['delete_version']]); | |
| $text = ''; | |
| foreach ($arrUnRAIDConfig as $key => $value) $text .= "$key=\"$value\"\n"; | |
| file_put_contents($strUnRAIDConfig, $text); | |
| $reply = ['status' => 'ok']; | |
| } | |
| echo json_encode($reply); | |
| exit; | |
| } | |
| if (isset($_POST['delete_version'])) { | |
| // Example snippet for CSRF token check | |
| if (!hash_equals($_SESSION['csrf_token'], $_POST['csrf_token'] ?? '')) { | |
| http_response_code(403); | |
| echo json_encode(['error' => 'Invalid CSRF token']); | |
| exit; | |
| } | |
| $arrDeleteUnRAID = []; | |
| if (array_key_exists($_POST['delete_version'], $arrUnRAIDVersions)) { | |
| $arrDeleteUnRAID = $arrUnRAIDVersions[$_POST['delete_version']]; | |
| } | |
| $reply = []; | |
| if (empty($arrDeleteUnRAID)) { | |
| $reply = ['error' => 'Unknown version: ' . $_POST['delete_version']]; | |
| } else { | |
| // delete img file | |
| @unlink($arrDeleteUnRAID['localpath']); | |
| // Save to strUnRAIDConfig | |
| unset($arrUnRAIDConfig[$_POST['delete_version']]); | |
| $text = ''; | |
| foreach ($arrUnRAIDConfig as $key => $value) $text .= "$key=\"$value\"\n"; | |
| file_put_contents($strUnRAIDConfig, $text); | |
| $reply = ['status' => 'ok']; | |
| } | |
| echo json_encode($reply); | |
| exit; | |
| } |
|
Would this be better as a plugin? It's probably not useful for the average user. |
1 participant
Added UnRAID VM template. You can select a version to install and it will create a VM with a 2gb virtual USB drive with UnRAID installed.
Summary by CodeRabbit