7.x Various VM Updates#1988
Conversation
Templates Export/Import Changes for virgl(Virtio3d) Bios flag.
WalkthroughThis pull request updates several components of the virtual machine manager plugin. The changes include an updated label on the VM addition button and enhanced UI functionality for managing VM templates. New JavaScript functions for saving, importing, exporting, and uploading templates have been added alongside corresponding backend cases. GPU configuration handling in the VM graphics component has been improved, and new methods in the libvirt integration refine capability retrieval and display options. Additional CSS rules have been introduced across multiple theme files, and form logic for GPU and CPU configuration has been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as VMTemplates UI
participant JS as Template JS Functions
participant API as VMajax Endpoint
U->>UI: Click "Save/Import Template"
UI->>JS: Trigger save/import function
JS->>API: Send AJAX request with template data
API-->>JS: Return success/error response
JS-->>UI: Update template list & notify user
sequenceDiagram
participant U as User
participant Form as Configuration Form
participant Func as VMConsoleDriverChange
U->>Form: Change VNC model selection
Form->>Func: Trigger driver change function
Func-->>Form: Adjust visibility of render & display option fields
Poem
Tip 🌐 Web search-backed reviews and chat
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (4)
105-107: LGTM! Consider adding documentation for the new GPU configuration options.The new GPU configuration options for render device and display settings are well implemented. Consider adding comments to document:
- The purpose and valid values for the 'render' field
- The format and options for 'DisplayOptions'
469-471: Consider improving accessibility for CPU migration controls.While the hidden attribute works correctly for visibility control, consider:
- Using ARIA attributes to improve screen reader support
- Adding a tooltip to explain when/why the migration option is hidden
1164-1165: Enhance GPU boot warning with more specific guidance.The warning about vBIOS requirements for primary GPUs is helpful, but consider:
- Adding a link to documentation about obtaining vBIOS files
- Specifying which GPU models typically require vBIOS
- Providing steps to verify if vBIOS is needed
Also applies to: 1299-1301, 1380-1380
2115-2138: Consider refactoring VMConsoleDriverChange for better maintainability.While the function works correctly, consider:
- Using constants for driver types ('virtio3d', 'qxl')
- Extracting repeated DOM manipulation into helper functions
- Using CSS classes instead of direct style manipulation
Example refactor:
function VMConsoleDriverChange(driver) { const VIRTIO3D = 'virtio3d'; const QXL = 'qxl'; const toggleElements = (elements, show) => { elements.forEach(el => { el.style.display = show ? 'inline' : 'none'; el.style.visibility = show ? 'visible' : 'hidden'; }); }; const renderElements = ['vncrender', 'vncrendertext'].map(id => document.getElementById(id)); const displayElements = ['vncdspopt', 'vncdspopttext'].map(id => document.getElementById(id)); toggleElements(renderElements, driver.value === VIRTIO3D); toggleElements(displayElements, driver.value === QXL); }emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)
2245-2256: Consider extracting render node path handling to a helper function.The render node path manipulation logic could be encapsulated in a separate helper function for better maintainability.
- if (!str_contains($var,"pci")) $var = trim(shell_exec("udevadm info -q symlink -r $var")); - $var = str_replace(['/dev/dri/by-path/pci-0000:','-render'],['',''],$var); + private function normalize_render_node_path($path) { + if (!str_contains($path, "pci")) { + $path = trim(shell_exec("udevadm info -q symlink -r $path")); + } + return str_replace(['/dev/dri/by-path/pci-0000:', '-render'], ['', ''], $path); + }
2355-2385: Add input validation for the domain parameter.The method correctly retrieves CPU configuration attributes but should validate the domain parameter.
function domain_get_cpu_custom($domain) { + if (!$domain) { + return $this->_set_last_error(); + } + $domain = $this->get_domain_object($domain); + if (!$domain) { + return $this->_set_last_error(); + } + $tmp = $this->get_xpath($domain, '//domain/cpu/@match', false);
872-889: Consider using constants for default display values.The implementation correctly handles virtio3d and display options, but could benefit from defined constants.
+ private const DEFAULT_QXL_DISPLAY_OPTIONS = "ram='65536' vram='16384' vgamem='16384' heads='1' primary='yes'"; + $strEGLHeadless = ""; $strAccel3d =""; if ($strModelType == "virtio3d") { $strModelType = "virtio"; if (!isset($gpu['render'])) $gpu['render'] = "auto"; if ($gpu['render'] == "auto") { $strEGLHeadless = '<graphics type="egl-headless"><gl enable="yes"/></graphics>'; $strAccel3d = "<acceleration accel3d='yes'/>"; } else { $strEGLHeadless = '<graphics type="egl-headless"><gl enable="yes" rendernode="/dev/dri/by-path/pci-0000:'.$gpu['render'].'-render"/></graphics>'; $strAccel3d ="<acceleration accel3d='yes'/>"; }} $strDisplayOptions = ""; if ($strModelType == "qxl") { - if (empty($gpu['DisplayOptions'])) $gpu['DisplayOptions'] ="ram='65536' vram='16384' vgamem='16384' heads='1' primary='yes'"; + if (empty($gpu['DisplayOptions'])) $gpu['DisplayOptions'] = self::DEFAULT_QXL_DISPLAY_OPTIONS; $strDisplayOptions = $gpu['DisplayOptions']; }emhttp/plugins/dynamix.vm.manager/VMTemplates.page (2)
111-129: Add cleanup for failed downloads.The
downloadJSONfunction should handle failed downloads and clean up temporary resources.Apply this diff to add cleanup:
function downloadJSON(data, filename = 'data.json') { + let url; try { // Convert the data to a JSON string const jsonString = JSON.stringify(data, null, 2); // Beautify with 2 spaces // Create a Blob from the JSON string const blob = new Blob([jsonString], { type: 'application/json' }); // Generate a temporary URL for the Blob - const url = URL.createObjectURL(blob); + url = URL.createObjectURL(blob); // Create a link element const link = document.createElement('a'); link.href = url; link.download = filename; // Specify the file name // Trigger the download by programmatically clicking the link link.click(); + } catch (e) { + console.error('Failed to download JSON:', e); } finally { // Clean up by revoking the object URL - URL.revokeObjectURL(url); + if (url) URL.revokeObjectURL(url); } }
250-267: Add input validation for template names.The dialog templates should include validation for template names to prevent invalid characters.
Add pattern validation to the input fields:
-<input type="text" id="target" autocomplete="off" spellcheck="false" value="" data-pickcloseonfile="true" data-pickfolders="true" data-pickfilter="iso"> +<input type="text" id="target" autocomplete="off" spellcheck="false" value="" pattern="[a-zA-Z0-9_-]+" title="Only alphanumeric characters, hyphens and underscores are allowed" data-pickcloseonfile="true" data-pickfolders="true" data-pickfilter="iso">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
emhttp/plugins/dynamix.vm.manager/VMMachines.page(1 hunks)emhttp/plugins/dynamix.vm.manager/VMTemplates.page(3 hunks)emhttp/plugins/dynamix.vm.manager/include/VMMachines.php(1 hunks)emhttp/plugins/dynamix.vm.manager/include/VMajax.php(1 hunks)emhttp/plugins/dynamix.vm.manager/include/libvirt.php(7 hunks)emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php(6 hunks)emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-azure.css(1 hunks)emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-black.css(1 hunks)emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-gray.css(1 hunks)emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-white.css(1 hunks)emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates.css(1 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(11 hunks)
✅ Files skipped from review due to trivial changes (5)
- emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates.css
- emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-white.css
- emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-gray.css
- emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-black.css
- emhttp/plugins/dynamix.vm.manager/sheets/VMTemplates-azure.css
🔇 Additional comments (12)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.php (2)
81-84: LGTM! GPU driver and render information display is well implemented.The code properly handles:
- GPU model detection and driver info display
- Special case for virtio3d with render settings
- Auto render mode vs specific GPU device selection
91-91: LGTM! Graphics display string construction is consistent.The code maintains a consistent format by:
- Appending GPU driver info to existing protocol info
- Preserving auto-connection status display
Also applies to: 96-96
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (5)
711-711: LGTM! QXL model definition is well-specified.The model definition includes all necessary parameters:
- RAM and VRAM sizes
- Video memory size
- Number of heads
- Primary display flag
713-758: LGTM! Display options array provides comprehensive configuration choices.The array is well-structured with:
- Clear text descriptions for each option
- Consistent XML attribute patterns
- Good coverage of memory sizes (16M to 256M)
- Support for multiple displays (1, 2, 4 heads)
918-925: LGTM! Boot VGA detection is properly implemented.The code:
- Correctly checks device class before reading boot_vga
- Safely handles file read operations
- Properly trims the read value
941-941: LGTM! Boot VGA field addition is consistent with device info structure.The bootvga field is properly added to the device info array.
1546-1546: LGTM! Graphics device handling is simplified.Removal of old graphics device unset logic improves the code by:
- Eliminating unnecessary state manipulation
- Maintaining graphics settings consistency
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (2)
58-71: LGTM! Well-documented domain capabilities implementation.The method is properly implemented with clear parameter documentation and consistent error handling.
2205-2206: LGTM! Improved graphics device type filtering.The XPath query now correctly filters for both spice and vnc graphics types.
emhttp/plugins/dynamix.vm.manager/VMTemplates.page (2)
41-50: LGTM! Dependencies are correctly included.The required CSS and JavaScript files are properly included for jQuery UI components, switch buttons, and file trees.
59-61: LGTM! Template management buttons are well-organized.The buttons for canceling, importing from file, and uploading are properly aligned and centered.
emhttp/plugins/dynamix.vm.manager/VMMachines.page (1)
489-489: LGTM! Button text accurately reflects dual functionality.The button text "Add VM/Templates" correctly indicates that users can add both VMs and templates.
| function uploadFile(files,index,start,time) { | ||
| var file = files[0]; | ||
| var blob = file; | ||
| reader.onloadend = function(e){ | ||
| uploadedData = JSON.parse(e.target.result); | ||
| //alert('File uploaded successfully. Click "Process JSON" to proceed.'); | ||
| saveUserTemplateImport(file.name,uploadedData); | ||
| }; | ||
| reader.readAsText(blob); | ||
| } | ||
|
|
||
| function startUpload(files) { | ||
| if (files.length==0) return; | ||
| reader = new FileReader(); | ||
| //window.onbeforeunload = function(e){return '';}; | ||
| uploadFile(files); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file type validation for uploads.
The file upload functions should validate the file type before processing.
Apply this diff to add validation:
function uploadFile(files,index,start,time) {
var file = files[0];
+ // Validate file type
+ if (!file.name.toLowerCase().endsWith('.json')) {
+ swal({title:"_(Error occurred)_?",text:"Invalid file type. Only JSON files are allowed.",type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"});
+ return;
+ }
var blob = file;
reader.onloadend = function(e){
+ try {
uploadedData = JSON.parse(e.target.result);
+ } catch (e) {
+ swal({title:"_(Error occurred)_?",text:"Invalid JSON format: " + e.message,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"});
+ return;
+ }
saveUserTemplateImport(file.name,uploadedData);
};
reader.readAsText(blob);
}📝 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 uploadFile(files,index,start,time) { | |
| var file = files[0]; | |
| var blob = file; | |
| reader.onloadend = function(e){ | |
| uploadedData = JSON.parse(e.target.result); | |
| //alert('File uploaded successfully. Click "Process JSON" to proceed.'); | |
| saveUserTemplateImport(file.name,uploadedData); | |
| }; | |
| reader.readAsText(blob); | |
| } | |
| function startUpload(files) { | |
| if (files.length==0) return; | |
| reader = new FileReader(); | |
| //window.onbeforeunload = function(e){return '';}; | |
| uploadFile(files); | |
| } | |
| function uploadFile(files,index,start,time) { | |
| var file = files[0]; | |
| // Validate file type | |
| if (!file.name.toLowerCase().endsWith('.json')) { | |
| swal({title:"_(Error occurred)_?", text:"Invalid file type. Only JSON files are allowed.", type:'error', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| return; | |
| } | |
| var blob = file; | |
| reader.onloadend = function(e){ | |
| try { | |
| uploadedData = JSON.parse(e.target.result); | |
| } catch (e) { | |
| swal({title:"_(Error occurred)_?", text:"Invalid JSON format: " + e.message, type:'error', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| return; | |
| } | |
| saveUserTemplateImport(file.name, uploadedData); | |
| }; | |
| reader.readAsText(blob); | |
| } | |
| function startUpload(files) { | |
| if (files.length==0) return; | |
| reader = new FileReader(); | |
| //window.onbeforeunload = function(e){return '';}; | |
| uploadFile(files); | |
| } |
| function saveUserTemplateFile(name,template) { | ||
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"no"},function($return){ | ||
| if ($return.success == false) { | ||
| swal({title:"_(File exists)_?",text:"Replace file: " + name ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(No)_"},function(p){ | ||
| if (p) { | ||
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"yes"},function($return){ | ||
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | ||
| }); | ||
| } | ||
| else { | ||
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | ||
| }; | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function saveUserTemplateImport(name,template) { | ||
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-import',name:name,template:template,replace:"no"},function($return){ | ||
| if ($return.success == false) { | ||
| swal({title:"_(File exists)_?",text:"Replace file: " + name.split(".")[0] ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(No)_"},function(p){ | ||
| if (p) { | ||
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"yes"},function($return){ | ||
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | ||
| if ($return.success == true) refresh(); | ||
| }); | ||
| } | ||
| else { | ||
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | ||
| }; | ||
| }); | ||
| } | ||
| if ($return.success == true) refresh(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing failures.
The template management functions need to handle JSON parsing errors when saving and importing templates.
Apply this diff to add error handling:
function saveUserTemplateFile(name,template) {
+ try {
+ // Validate template is valid JSON
+ JSON.parse(JSON.stringify(template));
+ } catch (e) {
+ swal({title:"_(Error occurred)_?",text:"Invalid template format: " + e.message,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"});
+ return;
+ }
$.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"no"},function($return){
if ($return.success == false) {
swal({title:"_(File exists)_?",text:"Replace file: " + name ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(No)_"},function(p){📝 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 saveUserTemplateFile(name,template) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"no"},function($return){ | |
| if ($return.success == false) { | |
| swal({title:"_(File exists)_?",text:"Replace file: " + name ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(No)_"},function(p){ | |
| if (p) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"yes"},function($return){ | |
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | |
| }); | |
| } | |
| else { | |
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | |
| }; | |
| }); | |
| } | |
| }); | |
| } | |
| function saveUserTemplateImport(name,template) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-import',name:name,template:template,replace:"no"},function($return){ | |
| if ($return.success == false) { | |
| swal({title:"_(File exists)_?",text:"Replace file: " + name.split(".")[0] ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(No)_"},function(p){ | |
| if (p) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"yes"},function($return){ | |
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | |
| if ($return.success == true) refresh(); | |
| }); | |
| } | |
| else { | |
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name + " " + $return.error ,type:'error',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | |
| }; | |
| }); | |
| } | |
| if ($return.success == true) refresh(); | |
| }); | |
| } | |
| function saveUserTemplateFile(name,template) { | |
| try { | |
| // Validate template is valid JSON | |
| JSON.parse(JSON.stringify(template)); | |
| } catch (e) { | |
| swal({title:"_(Error occurred)_?", text:"Invalid template format: " + e.message, type:'error', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| return; | |
| } | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php', {action:'vm-template-save', name:name, template:template, replace:"no"}, function($return){ | |
| if ($return.success == false) { | |
| swal({title:"_(File exists)_?", text:"Replace file: " + name, type:'warning', html:true, showCancelButton:true, confirmButtonText:"_(Proceed)_", cancelButtonText:"_(No)_"}, function(p){ | |
| if (p) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php', {action:'vm-template-save', name:name, template:template, replace:"yes"}, function($return){ | |
| if ($return.success == false) | |
| swal({title:"_(Error occured)_?", text:"Action error " + name + " " + $return.error, type:'warning', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| }); | |
| } | |
| else { | |
| if ($return.success == false) | |
| swal({title:"_(Error occured)_?", text:"Action error " + name + " " + $return.error, type:'error', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| }; | |
| }); | |
| } | |
| }); | |
| } | |
| function saveUserTemplateImport(name,template) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php', {action:'vm-template-import', name:name, template:template, replace:"no"}, function($return){ | |
| if ($return.success == false) { | |
| swal({title:"_(File exists)_?", text:"Replace file: " + name.split(".")[0], type:'warning', html:true, showCancelButton:true, confirmButtonText:"_(Proceed)_", cancelButtonText:"_(No)_"}, function(p){ | |
| if (p) { | |
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php', {action:'vm-template-save', name:name, template:template, replace:"yes"}, function($return){ | |
| if ($return.success == false) | |
| swal({title:"_(Error occured)_?", text:"Action error " + name.split(".")[0] + " " + $return.error, type:'warning', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| if ($return.success == true) refresh(); | |
| }); | |
| } | |
| else { | |
| if ($return.success == false) | |
| swal({title:"_(Error occured)_?", text:"Action error " + name + " " + $return.error, type:'error', html:true, showCancelButton:true, confirmButtonText:"_(OK)_"}); | |
| }; | |
| }); | |
| } | |
| if ($return.success == true) refresh(); | |
| }); | |
| } |
| case 'vm-template-save': | ||
| $template = $_REQUEST['template']; | ||
| $name = $_REQUEST['name']; | ||
| $replace = $_REQUEST['replace']; | ||
|
|
||
| if (is_file($name) && $replace == "no"){ | ||
| $arrResponse = ['success' => false, 'error' => _("File exists.")]; | ||
| } else { | ||
| $error = file_put_contents($name,json_encode($template)); | ||
| if ($error !== false) $arrResponse = ['success' => true]; | ||
| else { | ||
| $arrResponse = ['success' => false, 'error' => _("File write failed.")]; | ||
| } | ||
| } | ||
| break; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The template save case needs input validation and better error handling.
Apply this diff to improve the code:
case 'vm-template-save':
+ // Validate inputs
+ if (empty($_REQUEST['template']) || empty($_REQUEST['name'])) {
+ $arrResponse = ['success' => false, 'error' => _('Missing required parameters.')];
+ break;
+ }
+
$template = $_REQUEST['template'];
$name = $_REQUEST['name'];
$replace = $_REQUEST['replace'];
+ // Validate path
+ $dir = dirname($name);
+ if (!is_dir($dir)) {
+ $arrResponse = ['success' => false, 'error' => _('Invalid directory path.')];
+ break;
+ }
+
+ // Validate write permissions
+ if (!is_writable($dir)) {
+ $arrResponse = ['success' => false, 'error' => _('Directory is not writable.')];
+ break;
+ }
+
if (is_file($name) && $replace == "no"){
$arrResponse = ['success' => false, 'error' => _("File exists.")];
} else {
- $error = file_put_contents($name,json_encode($template));
+ // Ensure template is valid JSON
+ $json = json_encode($template, JSON_PRETTY_PRINT);
+ if ($json === false) {
+ $arrResponse = ['success' => false, 'error' => _('Invalid template format.')];
+ break;
+ }
+
+ $error = file_put_contents($name, $json);
if ($error !== false) $arrResponse = ['success' => true];
else {
$arrResponse = ['success' => false, 'error' => _("File write failed.")];
}
}
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.
| case 'vm-template-save': | |
| $template = $_REQUEST['template']; | |
| $name = $_REQUEST['name']; | |
| $replace = $_REQUEST['replace']; | |
| if (is_file($name) && $replace == "no"){ | |
| $arrResponse = ['success' => false, 'error' => _("File exists.")]; | |
| } else { | |
| $error = file_put_contents($name,json_encode($template)); | |
| if ($error !== false) $arrResponse = ['success' => true]; | |
| else { | |
| $arrResponse = ['success' => false, 'error' => _("File write failed.")]; | |
| } | |
| } | |
| break; | |
| case 'vm-template-save': | |
| // Validate inputs | |
| if (empty($_REQUEST['template']) || empty($_REQUEST['name'])) { | |
| $arrResponse = ['success' => false, 'error' => _('Missing required parameters.')]; | |
| break; | |
| } | |
| $template = $_REQUEST['template']; | |
| $name = $_REQUEST['name']; | |
| $replace = $_REQUEST['replace']; | |
| // Validate path | |
| $dir = dirname($name); | |
| if (!is_dir($dir)) { | |
| $arrResponse = ['success' => false, 'error' => _('Invalid directory path.')]; | |
| break; | |
| } | |
| // Validate write permissions | |
| if (!is_writable($dir)) { | |
| $arrResponse = ['success' => false, 'error' => _('Directory is not writable.')]; | |
| break; | |
| } | |
| if (is_file($name) && $replace == "no"){ | |
| $arrResponse = ['success' => false, 'error' => _("File exists.")]; | |
| } else { | |
| // Ensure template is valid JSON | |
| $json = json_encode($template, JSON_PRETTY_PRINT); | |
| if ($json === false) { | |
| $arrResponse = ['success' => false, 'error' => _('Invalid template format.')]; | |
| break; | |
| } | |
| $error = file_put_contents($name, $json); | |
| if ($error !== false) { | |
| $arrResponse = ['success' => true]; | |
| } else { | |
| $arrResponse = ['success' => false, 'error' => _("File write failed.")]; | |
| } | |
| } | |
| break; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)
2115-2138: Enhance error handling in VMConsoleDriverChange function.The function should handle edge cases where elements don't exist to prevent potential JavaScript errors.
-function VMConsoleDriverChange(driver) { +function VMConsoleDriverChange(driver) { + if (!driver) return; + + const vncrender = document.getElementById("vncrender"); + const vncrendertext = document.getElementById("vncrendertext"); + const vncdspopt = document.getElementById("vncdspopt"); + const vncdspopttext = document.getElementById("vncdspopttext"); + + if (!vncrender || !vncrendertext || !vncdspopt || !vncdspopttext) return; + if (driver.value != "virtio3d") { - document.getElementById("vncrender").style.visibility="hidden"; - document.getElementById("vncrendertext").style.visibility="hidden"; - document.getElementById("vncrender").style.display="none"; - document.getElementById("vncrendertext").style.display="none"; + vncrender.style.visibility = "hidden"; + vncrender.style.display = "none"; + vncrendertext.style.visibility = "hidden"; + vncrendertext.style.display = "none"; } else { - document.getElementById("vncrender").style.display="inline"; - document.getElementById("vncrender").style.visibility="visible"; - document.getElementById("vncrendertext").style.display="inline"; - document.getElementById("vncrendertext").style.visibility="visible"; + vncrender.style.display = "inline"; + vncrender.style.visibility = "visible"; + vncrendertext.style.display = "inline"; + vncrendertext.style.visibility = "visible"; } if (driver.value != "qxl") { - document.getElementById("vncdspopt").style.visibility="hidden"; - document.getElementById("vncdspopttext").style.visibility="hidden"; + vncdspopt.style.visibility = "hidden"; + vncdspopttext.style.visibility = "hidden"; } else { - document.getElementById("vncdspopt").style.display="inline"; - document.getElementById("vncdspopt").style.visibility="visible"; - document.getElementById("vncdspopttext").style.display="inline"; - document.getElementById("vncdspopttext").style.visibility="visible"; + vncdspopt.style.display = "inline"; + vncdspopt.style.visibility = "visible"; + vncdspopttext.style.display = "inline"; + vncdspopttext.style.visibility = "visible"; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(11 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (5)
105-107: LGTM: GPU configuration array enhancement.The addition of
renderandDisplayOptionsfields to the GPU configuration array provides better control over GPU rendering and display settings.
469-471: LGTM: Improved CPU migration UI visibility.The CPU migration dropdown is now properly hidden when not applicable, improving UI clarity.
1299-1301: LGTM: Added GPU boot VGA warning.The warning about vBIOS requirement for primary GPU adapter improves user guidance.
2489-2492: LGTM: Improved GPU boot VGA visibility handling.The code properly handles the visibility of GPU boot VGA warning based on the GPU configuration.
1254-1276: Verify the display options configuration.The new display options for QXL and Virtio3D drivers need verification:
- QXL display options should match the supported configurations
- Virtio3D render device selection should be compatible with the selected GPU
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify QXL and Virtio3D configurations in libvirt # Check QXL display options virsh domcapabilities --virttype kvm | grep -A 10 "<video" # Check Virtio3D support virsh domcapabilities --virttype kvm | grep -A 10 "virtio3d"Length of output: 313
I'll generate a new shell script to search the codebase for references to the QXL display options (i.e. "qxlxml") and for occurrences of "virtio3d". This will help us verify whether the configurations in the PHP template match the supported settings in the codebase.
🏁 Script executed:
#!/bin/bash echo "Searching for QXL display options configuration ('qxlxml')..." rg -i "qxlxml" echo "" echo "Searching for Virtio3D references..." rg -i "virtio3d"Length of output: 3339
Verification of Display Options Configuration
• The QXL display options in Custom.form.php use the "qxlxml" keys defined in libvirt_helpers.php. These definitions (e.g., RAM, VRAM settings, etc.) match the supported configurations.
• The virtio3d option correctly toggles the visibility of the render device selection based on the GPU model, ensuring that the render device can be specified when virtio3d is selected.Based on the code references and our search results, the configurations for both QXL and virtio3d appear to be consistent with the supported definitions.
2 participants
Summary by CodeRabbit
New Features
Style