Updates for libvirt image removal and use directory #1722
+1,032
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds a hidden OLD_IMAGE_FILE UI field; implements image/directory sync between OLD_IMAGE_FILE and IMAGE_FILE during libvirt init; records VM storage locations on libvirt stop; introduces scripts for discovering, copying, restoring VM XML, NVRAM/snapshot migration, filesystem helpers, and XML persistence in the Libvirt integration. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant UI as VMSettings.page
participant CFG as /boot/config/domain.cfg
participant Init as libvirt_init
participant FS as Storage
participant Libvirt as libvirtd
participant RC as rc.libvirt
participant Copy as libvirtcopy
participant VMSJSON as /boot/config/.../vms.json
participant Save as savehook.php
participant Restore as libvirtrestore
Admin->>UI: change IMAGE_FILE
UI->>CFG: write IMAGE_FILE and OLD_IMAGE_FILE
Init->>CFG: read IMAGE_FILE, OLD_IMAGE_FILE
alt both set and differ
Init->>FS: mount/backup OLD or IMAGE as needed
Init->>FS: rsync data between OLD and IMAGE (exclude images/backups)
Init->>CFG: update OLD_IMAGE_FILE
else skip sync
Init->>CFG: log skipped
end
Admin->>Libvirt: stop
Libvirt->>RC: run pre-stop hooks
RC->>Copy: invoke libvirtcopy
Copy->>Libvirt: query domains
Copy->>VMSJSON: write VM metadata and planned XML copy list
loop per VM stop
Save->>VMSJSON: read metadata
Save->>FS: copy /etc/libvirt/qemu/{vm}.xml -> vm backup path
end
Libvirt start->>Restore: run libvirtrestore
Restore->>VMSJSON: read metadata
Restore->>FS: copy_if_different VM XMLs -> /etc/libvirt/qemu
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Default set to folder.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init (1)
9-63: Well-structured data migration mechanism.The synchronization logic is thoughtfully implemented to handle multiple scenarios (image file to directory, directory to image, directory to directory) while providing proper backups and logging throughout the process.
Consider these minor improvements:
- Add error handling for mount/umount operations
- Add cleanup for the temporary directory
- Add error checking for rsync operations
Example improvement:
+ # Create temporary mount directory mkdir -p "$TMP_MNT" - mount "$OLD_IMAGE_FILE" "$TMP_MNT" + if ! mount "$OLD_IMAGE_FILE" "$TMP_MNT"; then + log "Failed to mount $OLD_IMAGE_FILE - aborting sync" + rm -rf "$TMP_MNT" + exit 1 + fi log "Copying full contents from image to directory $IMAGE_FILE" - rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/" + if ! rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/"; then + log "rsync failed - aborting sync" + umount "$TMP_MNT" + rm -rf "$TMP_MNT" + exit 1 + fi umount "$TMP_MNT" + rm -rf "$TMP_MNT"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
emhttp/plugins/dynamix.vm.manager/VMSettings.page(6 hunks)emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init(1 hunks)emhttp/plugins/dynamix.vm.manager/scripts/libvirtconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/VMSettings.page
🔇 Additional comments (1)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtconfig (1)
18-19: Configuration updated to support directory-based VM image storage.The changes here transition from using a specific image file path to a directory path approach for libvirt VM storage. The added
OLD_IMAGE_FILEentry will be used for synchronization during the migration from file-based to directory-based storage as implemented in thelibvirt_initscript.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
emhttp/languages/en_US/helptext.txt (3)
1624-1626: Refine grammar and acronym capitalization.The sentence could be clearer by capitalizing well‑known acronyms and improving phrasing. For example:
“This is a location for storing previous versions of XML and NVRAM when changes occur.”
1633-1635: Ensure consistent capitalization of ‘libvirt’.Here “Libvirt” is capitalized, whereas elsewhere it’s lowercase. Consider using “libvirt” for consistency, or updating all occurrences to “Libvirt” uniformly.
1637-1639: Improve clarity and grammar for secondary location help.The sentence could be rephrased and acronyms capitalized. For example:
“This directory is used to store previous versions of XML and NVRAM when they change. Specification is optional.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
emhttp/languages/en_US/helptext.txt(2 hunks)emhttp/plugins/dynamix.vm.manager/VMSettings.page(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/VMSettings.page
🔇 Additional comments (1)
emhttp/languages/en_US/helptext.txt (1)
1621-1622: Approve updated terminology for primary storage.The change from “libvirt volume” to “libvirt volume/directory” accurately reflects that both file and directory paths are accepted.
🔧 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
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/languages/en_US/helptext.txt(2 hunks)emhttp/plugins/dynamix.vm.manager/VMSettings.page(3 hunks)emhttp/plugins/dynamix.vm.manager/scripts/libvirtconfig(1 hunks)
🔇 Additional comments (4)
emhttp/plugins/dynamix.vm.manager/VMSettings.page (1)
126-126: LGTM - Hidden field properly captures old value.The hidden field correctly captures the current
IMAGE_FILEvalue and properly escapes it withhtmlspecialchars()for security.emhttp/languages/en_US/helptext.txt (2)
1714-1715: LGTM - Help text accurately reflects volume/directory usage.The updated help text correctly indicates that the libvirt storage can be either a volume or directory.
1727-1728: LGTM - Help text updated to include directory option.The help text now correctly indicates that libvirt can use either an image file or a directory.
emhttp/plugins/dynamix.vm.manager/scripts/libvirtconfig (1)
18-19: Code is functioning correctly; review comment concern is invalid.The synchronization logic on line 22 of
libvirt_initexplicitly gates behind an inequality check:[ "$IMAGE_FILE" != "$OLD_IMAGE_FILE" ]. When both variables default to the same path for fresh installations, this condition evaluates false and the sync logic is safely skipped—which is the correct behavior. No action needed.Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @emhttp/languages/en_US/helptext.txt:
- Around line 1730-1732: Update the help text for
:vms_libvirt_secondary_location_help: to fix grammar and capitalization by
replacing "at change" with a clearer phrase such as "when changed" or "on
change", and capitalize "XML" and "NVRAM" (e.g., "This is a directory for
storing previous versions of XML and NVRAM when changed. Does not need to be
specified.").
- Around line 1717-1719: The help text for :vms_libvirt_secondary_volume_help:
is grammatically awkward—replace "at change" with a clearer phrase such as "when
changed" or "on change" (e.g., "This is a location for storing previous versions
of XML and NVRAM when changed.") and ensure XML and NVRAM are properly
capitalized; update the string for :vms_libvirt_secondary_volume_help:
accordingly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/languages/en_US/helptext.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
🔇 Additional comments (2)
emhttp/languages/en_US/helptext.txt (2)
1713-1715: Approve: Updated help text to reflect directory support.The change from "This is the libvirt volume." to "This is the libvirt volume/directory." correctly communicates the expanded capability introduced in this PR.
1726-1728: Approve: Updated help text to reflect directory support.The change from specifying an "image file" to "image file/directory" and the corresponding update to "file/directory" accurately reflect the expanded configuration options introduced in this PR.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation:
- Around line 111-114: The path construction currently embeds literal quotes
into $path (using $path_root . '/"' . $vm_name . '"'), which is inconsistent
with the is_dir() check and forces consumers to strip quotes; change $path so it
does not include embedded quote characters (build it as $path_root + '/' +
$vm_name when $path_root is set) and ensure any existing logic that strips
quotes before calling is_dir() is removed or adjusted so path handling is
consistent and consumers can perform proper shell escaping themselves.
- Line 136: The write to vms.json using
file_put_contents("/boot/config/plugins/dynamix.vm.manager/vms.json",...) can
fail if the plugin directory doesn't exist; before the file_put_contents call in
libvirtlocation, ensure the parent directory exists by invoking a directory
creation step (e.g., call mkdir on dirname of the target path with recursive
true and permissive mode, or use the error-suppressing @mkdir variant) so that
the directory is created if missing, then proceed to file_put_contents; update
the code around the file_put_contents invocation to perform this check/create
using the same path string or a local $path variable.
🧹 Nitpick comments (1)
etc/rc.d/rc.libvirt (1)
250-252: Add basic error handling for the libvirtlocation script invocation.The script is invoked without error handling or logging. Consider adding minimal safeguards:
- The script requires libvirt to be running (it connects to
qemu:///system), which should be true at this point- Silent failures provide no visibility for debugging
- Script failure shouldn't block VM shutdown, but operators should be aware
💡 Suggested improvements
Option A: Add logging for failures
# Save VM locations - /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation + if ! /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation 2>&1 | logger -t libvirtlocation; then + log "Warning: Failed to save VM locations" + fi #Option B: Simple existence check
# Save VM locations + [[ -x /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation ]] && \ /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation #
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix.vm.manager/VMSettings.pageemhttp/plugins/dynamix.vm.manager/scripts/libvirtlocationetc/rc.d/rc.libvirt
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/VMSettings.page
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
🔇 Additional comments (3)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation (3)
18-43: LGTM!The function correctly parses the domain configuration file, handles missing files gracefully, and normalizes the path by removing trailing slashes.
101-109: LGTM!The storage resolution logic correctly handles both default storage (using
DOMAINDIRfrom config) and explicit Unraid pools, with appropriate case-insensitive comparison for "default".
54-57: Verify the intended scope of domain enumeration.The
libvirt_list_domains()function returns only running domains. The comment on line 53 explicitly acknowledges this design choice ("Running VMs (or all, if you prefer libvirt_list_all_domains)"), suggesting it was deliberate. However, without clarity on the actual use case forvms.json(no consumers of this file were found in the codebase) and when this script is invoked, it's unclear whether the current behavior is correct. If this script is meant to maintain a complete VM inventory for location tracking across all VM states, usinglibvirt_list_all_domains()would be more appropriate.
Comment on lines
111
to
114
| /* Shell-safe path (VM name quoted) */ | ||
| $path = $path_root | ||
| ? $path_root . '/"' . $vm_name . '"' | ||
| : null; |
Contributor
There was a problem hiding this comment.
Reconsider embedding quotes in the path string.
The path construction embeds literal quote characters within the string (e.g., /mnt/user/domains/"VM Name"), which may cause issues for consumers:
- Shell scripts consuming this JSON would need to handle or strip the embedded quotes manually
- Line 117 correctly removes quotes for the
is_dir()check, confirming they're not part of the actual filesystem path - This approach is confusing and error-prone
Consider either:
- Option A (recommended): Remove the embedded quotes and let consumers apply proper shell escaping when needed
- Option B: Document this escaping convention clearly if it's required by existing consumers
🔧 Option A: Remove embedded quotes
- /* Shell-safe path (VM name quoted) */
- $path = $path_root
- ? $path_root . '/"' . $vm_name . '"'
- : null;
+ /* Full filesystem path */
+ $path = $path_root
+ ? $path_root . '/' . $vm_name
+ : null;📝 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.
Suggested change
| /* Shell-safe path (VM name quoted) */ | |
| $path = $path_root | |
| ? $path_root . '/"' . $vm_name . '"' | |
| : null; | |
| /* Full filesystem path */ | |
| $path = $path_root | |
| ? $path_root . '/' . $vm_name | |
| : null; |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtlocation around lines 111 -
114, The path construction currently embeds literal quotes into $path (using
$path_root . '/"' . $vm_name . '"'), which is inconsistent with the is_dir()
check and forces consumers to strip quotes; change $path so it does not include
embedded quote characters (build it as $path_root + '/' + $vm_name when
$path_root is set) and ensure any existing logic that strips quotes before
calling is_dir() is removed or adjusted so path handling is consistent and
consumers can perform proper shell escaping themselves.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy:
- Line 137: Before calling
file_put_contents("/boot/config/plugins/dynamix.vm.manager/vms.json", ...),
ensure the parent directory exists by creating
"/boot/config/plugins/dynamix.vm.manager" if missing (use mkdir with
recursive=true and appropriate mode, e.g., 0755), handle or log failure of
mkdir, then proceed to write with file_put_contents; update the logic around the
file_put_contents call so it does not assume the directory already exists and
fails silently.
- Around line 140-149: The loop currently calls
file_put_contents("/tmp/Stopcopy","") inside the foreach over $vms, which
truncates /tmp/Stopcopy on every iteration and loses previous entries; move the
initialization (the file_put_contents("/tmp/Stopcopy","") call) outside and
before the foreach so the file is cleared only once, then keep the existing
file_put_contents(..., FILE_APPEND) calls inside the loop to append each VM's
line; update references in the loop around $vmdetail['exists'] and the file
paths (/etc/libvirt/qemu/$vm.xml and $vmdetail['path']."/$vm.xml") accordingly.
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore:
- Around line 15-16: The code directly calls
file_get_contents("/boot/config/plugins/dynamix.vm.manager/vms.json") into
$vmsjson and json_decode($vmsjson,true) into $vms without checking for errors;
modify the logic around file_get_contents and json_decode so that: verify
file_get_contents did not return false (handle missing/unreadable file by
logging and exiting or falling back), validate json_decode did not return
null/false for an expected array and check
json_last_error()/json_last_error_msg() to produce a clear error message, and
ensure $vms is an array before using it (fail fast or use a safe default).
Include these checks referencing the existing variables $vmsjson and $vms and
the functions file_get_contents and json_decode.
- Around line 18-27: The loop in foreach ($vms as $vm => $vmdetail) calls
file_put_contents("/tmp/Stopcopy","") on every iteration which wipes
/tmp/Stopcopy repeatedly; either remove that line if /tmp/Stopcopy was a stray
copy/paste, or move a single truncate call before the loop and change any
intended per-iteration writes to use FILE_APPEND (like the existing
/tmp/libvirtrestore writes). Inspect the similar libvirtcopy script for intended
behavior and ensure only one of /tmp/Stopcopy or /tmp/libvirtrestore is written
to—delete the unused one to avoid duplicate/tmp unintended logs.
In @emhttp/plugins/dynamix.vm.manager/scripts/savehook.php:
- Around line 3-4: The code reads $cfg via file_get_contents and immediately
json_decodes it without validating results; add checks around
file_get_contents($cfg) to ensure the file exists and is readable and handle a
false return (log an error and exit/return), then validate json_decode() result
for null/false and check json_last_error() to handle invalid JSON (log the error
and exit/return); update the variables used here ($cfg, $vms) and any downstream
logic to bail out or use a safe default when file read or parse fails.
In @etc/rc.d/rc.libvirt:
- Around line 250-252: The pre-stop hook currently calls
/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy without checks;
update the pre-stop logic to first verify the script exists and is executable
(test -x "/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy"),
and if so execute it capturing its exit code; if the script is missing/not
executable or returns a non-zero status, emit a clear error via logger/echo
(including the command path and exit code) and return a non-zero status from the
pre-stop hook to halt the libvirtd stop sequence; ensure the code references the
exact script path and the pre-stop hook function/name so it’s easy to locate.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy (1)
48-57: Add error handling for libvirt operations.The libvirt connection and domain listing operations use
die()for error handling, which exits immediately. While acceptable for a CLI script, consider logging more diagnostic information (e.g., libvirt error messages) to aid troubleshooting.📝 Proposed improvement
$lv = libvirt_connect('qemu:///system', false); if (!$lv) { - die("Failed to connect to libvirt\n"); + $err = libvirt_get_last_error(); + die("Failed to connect to libvirt: " . ($err ? $err : "Unknown error") . "\n"); } /* Running VMs (or all, if you prefer libvirt_list_all_domains) */ $domains = libvirt_list_domains($lv); if ($domains === false) { - die("Failed to list domains\n"); + $err = libvirt_get_last_error(); + die("Failed to list domains: " . ($err ? $err : "Unknown error") . "\n"); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopyemhttp/plugins/dynamix.vm.manager/scripts/libvirtrestoreemhttp/plugins/dynamix.vm.manager/scripts/savehook.phpetc/rc.d/rc.libvirt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopyemhttp/plugins/dynamix.vm.manager/scripts/savehook.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopyemhttp/plugins/dynamix.vm.manager/scripts/savehook.php
📚 Learning: 2025-02-10T20:28:41.294Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2011
File: etc/rc.d/rc.S:0-0
Timestamp: 2025-02-10T20:28:41.294Z
Learning: When modifying files that are managed outside the repository (like `/etc/php.d/errors-php.ini`), always include a file existence check to ensure the script doesn't fail if the file is missing.
Applied to files:
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/scripts/savehook.php
🔇 Additional comments (2)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy (1)
147-147: Clarify the commented-out copy operation.The copy operation at line 147 is commented out, which means this script only logs copy targets but doesn't perform the actual XML file backup. Is this intentional for the current phase, or should the copy be enabled?
If the copy is intentionally disabled for testing/debugging, consider adding a comment explaining why. If it should be enabled, please uncomment and add error handling for the copy operation.
emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore (1)
25-25: Clarify the commented-out copy operation.Similar to
libvirtcopy, the copy operation is commented out. Is this intentional for the current development phase? If so, add a comment explaining the intent. If the copy should be enabled, uncomment and add error handling.
Comment on lines
+5
to
+11
| if ($argv[2] == 'stopped'){ | ||
| $vm = $argv[1]; | ||
| $from_file = "/etc/libvirt/qemu/$vm.xml"; | ||
| $to_file = $vms[$argv[1]]['path']."/$vm.xml"; | ||
| #echo " from:$from_file to:$to_file"; | ||
| copy($from_file,$to_file); | ||
| } |
Contributor
There was a problem hiding this comment.
Add input validation and error handling.
Multiple issues in the conditional block:
- No bounds checking on
$argvarray (accessing indices 1 and 2 without verification) - No validation that the VM name exists in the
$vmsarray - No directory existence check before attempting copy
- No error handling for the
copy()operation - VM name is not validated, creating potential path traversal vulnerability
🔒 Proposed fix with validation and error handling
-if ($argv[2] == 'stopped'){
- $vm = $argv[1];
- $from_file = "/etc/libvirt/qemu/$vm.xml";
- $to_file = $vms[$argv[1]]['path']."/$vm.xml";
- #echo " from:$from_file to:$to_file";
- copy($from_file,$to_file);
-}
+if ($argc < 3) {
+ error_log("savehook: Insufficient arguments. Usage: savehook.php <vm_name> <state>");
+ exit(1);
+}
+
+if ($argv[2] === 'stopped') {
+ $vm = $argv[1];
+
+ // Validate VM name to prevent path traversal
+ if (!preg_match('/^[a-zA-Z0-9_-]+$/', $vm)) {
+ error_log("savehook: Invalid VM name: $vm");
+ exit(1);
+ }
+
+ if (!isset($vms[$vm])) {
+ error_log("savehook: VM not found in configuration: $vm");
+ exit(1);
+ }
+
+ $from_file = "/etc/libvirt/qemu/$vm.xml";
+ if (!file_exists($from_file)) {
+ error_log("savehook: Source XML file not found: $from_file");
+ exit(1);
+ }
+
+ $to_dir = $vms[$vm]['path'];
+ if (!is_dir($to_dir)) {
+ error_log("savehook: Destination directory does not exist: $to_dir");
+ exit(1);
+ }
+
+ $to_file = "$to_dir/$vm.xml";
+ if (!copy($from_file, $to_file)) {
+ error_log("savehook: Failed to copy $from_file to $to_file");
+ exit(1);
+ }
+}
Contributor
Author
There was a problem hiding this comment.
This is being called by libvirt so should not need this level of checking.
Contributor
There was a problem hiding this comment.
@SimonFair, understood. Since libvirt invokes this hook in a controlled context with known inputs, the extensive validation isn't necessary.
That said, you might still consider minimal error logging for the copy operation itself, in case of filesystem issues:
if (!copy($from_file, $to_file)) {
error_log("savehook: Failed to copy $from_file to $to_file");
}This would help with troubleshooting if the copy fails due to disk space, permissions, or other filesystem issues, but I'll leave that to your discretion.
🧠 Learnings used
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: etc/rc.d/rc.rsyslogd:48-50
Timestamp: 2025-02-27T21:53:37.589Z
Learning: For the unraid/webgui project, simple error handling is preferred over extensive error checking in shell scripts. The copy operation for configuration files like in rc.rsyslogd doesn't require additional error logging, permission checks, or return statements.
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
SimonFair
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php:
- Around line 86-99: The dir_remove function currently ignores the return value
of recursive calls and unlink operations, which can cause parent calls to report
success even if child deletions failed; update dir_remove to check the return
value of dir_remove($path) for directories and the result of @unlink($path) for
files, and if any child deletion returns false immediately return false (or
otherwise propagate failure) so that failures bubble up instead of being
silenced, ensuring the final return from dir_remove reflects any nested failure
rather than only the final rmdir result.
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init:
- Around line 12-19: The current use of eval to read IMAGE_FILE and
OLD_IMAGE_FILE from DOMAIN_CFG is unsafe and allows command injection; instead,
parse DOMAIN_CFG without shell evaluation by extracting the values for the keys
IMAGE_FILE and OLD_IMAGE_FILE (e.g., using grep/sed/awk to capture the RHS
only), assign those captured strings to IMAGE_FILE and OLD_IMAGE_FILE, and then
strip surrounding quotes as you already do; update the code around the
IMAGE_FILE, OLD_IMAGE_FILE, and DOMAIN_CFG handling to remove eval and use
direct parsing to safely set those variables.
- Around line 37-44: The mount/rsync block using variables OLD_IMAGE_FILE,
TMP_MNT, IMAGE_FILE and commands mount/rsync/umount must add error checks:
verify TMP_MNT is not already mounted (use mountpoint or check /proc/mounts)
before calling mount, check the exit status of mount and abort with a logged
error if it fails, only run rsync when mount succeeded, and ensure umount is
executed in a cleanup path (trap or conditional) to avoid leaving a stale mount;
apply the same pattern to the other mount/umount blocks (the one around lines
46-51) and include clear error logs mentioning the relevant variables
(OLD_IMAGE_FILE, TMP_MNT, IMAGE_FILE) so failures are visible.
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate:
- Around line 1-2: Remove the trailing space after /usr/bin/php in the shebang
at the top of the script (the first line of libvirtmigrate) so the interpreter
path is exact (#!/usr/bin/php) and save the file without any leading BOM or
extra whitespace; then ensure the script remains executable.
- Around line 91-98: The rollback unconditionally calls @unlink($dest_file) when
$xml_old_path is missing, which can delete a pre-existing destination; introduce
a boolean flag (e.g. $copied or $created_copy) initialized false, set it to true
only where you actually create/copy $dest_file, and change each XML-not-found
branch (the checks using $xml_old_path that currently call @unlink($dest_file))
to only unlink when that flag is true; apply the same change for the other two
occurrences that call @unlink($dest_file).
🧹 Nitpick comments (5)
emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php (1)
64-84: Consider verifyingscandirfailure and checking for symlink-based path traversal.
scandir()can returnfalseon failure, which would cause a warning on the foreach.- If
$srccontains symlinks,is_dir($s)will follow them, potentially copying content outside the intended source tree.Suggested hardening
function dir_copy($src, $dst) { if (!is_dir($src)) return false; if (!is_dir($dst)) { if (!@mkdir($dst, 0755, true)) return false; } $items = scandir($src); + if ($items === false) return false; foreach ($items as $item) { if ($item === '.' || $item === '..') continue; $s = $src . DIRECTORY_SEPARATOR . $item; $d = $dst . DIRECTORY_SEPARATOR . $item; + // Skip symlinks to avoid unintended traversal + if (is_link($s)) continue; if (is_dir($s)) { if (!dir_copy($s, $d)) return false; } else {emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate (4)
13-13: Short open tag<?may not work on all PHP configurations.Short open tags require
short_open_tag=Onin php.ini. Using<?phpis more portable.-<? +<?php
56-71: Duplicate file comparison logic - usefiles_identicalhelper.This code duplicates the logic already available in
files_identical()from the includedfs_helpers.php. Using the helper improves maintainability.Suggested refactor
// Copy NVRAM file (compare first) $would_copy = false; $copied = false; - if (file_exists($dest_file)) { - $same = false; - if (filesize($src_file) === filesize($dest_file)) { - $hs = @md5_file($src_file); - $hd = @md5_file($dest_file); - if ($hs !== false && $hd !== false && $hs === $hd) { - $same = true; - } - } - if (!$same) $would_copy = true; - } else { + if (!file_exists($dest_file) || !files_identical($src_file, $dest_file)) { $would_copy = true; }
314-326: Script terminates withdie()on libvirt connection failure.For a CLI tool, this is acceptable, but the error message goes to stdout instead of stderr, and the exit code is 0 (default for
die()). Consider usingfwrite(STDERR, ...)and explicit exit code for better integration with calling scripts.Suggested improvement
$lv = libvirt_connect('qemu:///system', false); if (!$lv) { - die("ERROR: Failed to connect to libvirt\n"); + fwrite(STDERR, "ERROR: Failed to connect to libvirt\n"); + exit(2); } // Get all valid VM UUIDs $domains = libvirt_list_domains($lv); if ($domains === false) { - die("ERROR: Failed to list domains\n"); + fwrite(STDERR, "ERROR: Failed to list domains\n"); + exit(2); }
364-364: UUID regex may not match all valid UUID formats.The pattern
[a-f0-9\-]+is permissive but doesn't enforce the standard UUID format (8-4-4-4-12 hex digits). This could match invalid UUIDs or partial matches. Consider using a stricter pattern if UUID validation is important.- if (preg_match('/^([a-f0-9\-]+)(?:S([^_]+))?_VARS/', $basename, $matches)) { + if (preg_match('/^([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})(?:S([^_]+))?_VARS/', $basename, $matches)) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix.vm.manager/include/fs_helpers.phpemhttp/plugins/dynamix.vm.manager/scripts/libvirt_initemhttp/plugins/dynamix.vm.manager/scripts/libvirtcopyemhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrateemhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy
- emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php
🔇 Additional comments (4)
emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php (2)
9-16: LGTM - Well-structured file comparison.The function properly handles edge cases: non-existent files, size mismatch short-circuit (avoiding unnecessary hash computation), and hash computation failures. Using MD5 for file identity comparison is acceptable here since this is not a security context.
18-62: LGTM - Robust copy logic with dry-run support.The function correctly:
- Returns early with error on missing source
- Creates destination directory when needed (respecting dry-run mode)
- Compares files before copying to avoid unnecessary I/O
- Reports comprehensive result metadata
emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init (1)
97-100: LGTM - libvirtrestore invocation.The script correctly invokes libvirtrestore after initialization to restore VM XMLs.
emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate (1)
456-462: LGTM - CLI argument parsing.The argument parsing is clear and well-structured. The default to dry-run mode (
$dry_run = !$confirm) is a safe approach that prevents accidental destructive operations.
Comment on lines
91
to
98
| // Read old XML | ||
| if (!file_exists($xml_old_path)) { | ||
| @unlink($dest_file); // Rollback | ||
| return [ | ||
| 'success' => false, | ||
| 'error' => "XML file not found: $xml_old_path" | ||
| ]; | ||
| } |
Contributor
There was a problem hiding this comment.
Rollback on line 93 happens unconditionally, even when no copy occurred.
If the destination file already existed (wasn't copied), deleting it on XML-not-found error would incorrectly remove the pre-existing file.
Suggested fix
// Read old XML
if (!file_exists($xml_old_path)) {
- @unlink($dest_file); // Rollback
+ if ($copied) @unlink($dest_file); // Rollback only if we copied
return [
'success' => false,
'error' => "XML file not found: $xml_old_path"
];
}Apply the same fix to lines 104 and 119.
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate around lines 91 -
98, The rollback unconditionally calls @unlink($dest_file) when $xml_old_path is
missing, which can delete a pre-existing destination; introduce a boolean flag
(e.g. $copied or $created_copy) initialized false, set it to true only where you
actually create/copy $dest_file, and change each XML-not-found branch (the
checks using $xml_old_path that currently call @unlink($dest_file)) to only
unlink when that flag is true; apply the same change for the other two
occurrences that call @unlink($dest_file).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php`:
- Around line 1825-1829: The code calls manage_domain_xml($domain, $xml, false)
but $domain is a libvirt domain resource (from get_domain_object()) while
manage_domain_xml expects the domain name string; change the call to pass the
domain name instead (e.g. obtain it with libvirt_domain_get_name($dom) or use
the original name variable used when calling get_domain_object()) so replace the
wrong argument to manage_domain_xml with the domain name string while keeping
libvirt_domain_get_xml_desc($dom) and the rest of the flow intact.
- Around line 1641-1694: The manage_domain_xml function is currently a global
function but callers use $this->manage_domain_xml(); convert it into a class
method (e.g., public function manage_domain_xml(...)) so it can be called via
$this; guard the parse_ini_file call by checking $domain_cfg !== false before
accessing $domain_cfg['DOMAINDIR']; avoid assuming every pool is under /mnt by
checking the constructed $storage_path exists and if not falling back to a
sensible default (e.g., the domains fallback used previously) or resolving pool
mounts via existing pool lookup helper if available; change the behavior when
$xml_dir doesn't exist so that on save=true you attempt to create the directory
(mkdir -p with appropriate permissions and error handling) and on save=false
return true if nothing exists to delete; tidy indentation in the
manage_domain_xml method to match surrounding class style.
- Around line 1476-1478: manage_domain_xml is being called with a null domain
(manage_domain_xml(null, $xml, true)) which produces invalid file paths and only
writes XML to disk without re-registering the VM in libvirt; change the call to
supply the actual domain name (e.g., extract $domain from the existing $dom
object or parse $old_xml) when calling manage_domain_xml, and after
libvirt_domain_undefine($dom) add the missing libvirt definition step (call the
appropriate libvirt define function—e.g., libvirt_define or
libvirt_domain_define with the new XML/filename) so the VM is re-registered;
keep existing error handling (use libvirt_get_last_error()) and restore the old
XML via manage_domain_xml($domain, $old_xml, true) on failure.
- Around line 1636-1638: The fallback calls
libvirt_domain_create_xml($this->conn, $dom) with $dom being false because
get_domain_object() overwrote the original domain name; fix by preserving the
original domain identifier before calling get_domain_object() (e.g., save
$origDomain = $domain), then when get_domain_object(...) returns false either
remove the fallback or construct/obtain a valid XML string for
libvirt_domain_create_xml and pass that ($origDomain or built XML) instead of
the boolean; also ensure $this->last_error = libvirt_get_last_error() only runs
after a real libvirt call and return an error/false if no valid XML can be
provided.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (2)
1704-1716: XML persistence logic looks correct, butmanage_domain_xmlfailure is silently ignored.The XML is extracted and saved correctly. However, if
manage_domain_xml()fails (returnsfalse), the function still returns the domain resource as success. Consider logging or handling the failure:♻️ Suggested improvement
$tmp = libvirt_domain_define_xml($this->conn, $xml); if ($tmp) { // Extract domain name from XML to save it if (preg_match('/<name>(.*?)<\/name>/s', $xml, $matches)) { - $this->manage_domain_xml($matches[1], $xml, true); + if (!$this->manage_domain_xml($matches[1], $xml, true)) { + // Log warning but don't fail - domain is defined in libvirt + error_log("Warning: Failed to save XML to disk for domain: " . $matches[1]); + } } }
2655-2656: Placeholder comment can be removed or resolved.The comment
## Use new function?appears to be a TODO note. The currentlibvirt_domain_update_devicecalls are appropriate for live CD-ROM changes. XML persistence happens throughdomain_definewhen the VM configuration is explicitly saved.
Comment on lines
1636
to
1638
| $ret = libvirt_domain_create_xml($this->conn, $dom); ## Update to use new function | ||
| $this->last_error = libvirt_get_last_error(); | ||
| return $ret; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the exact file and the lines in question
wc -l emhttp/plugins/dynamix.vm.manager/include/libvirt.phpRepository: unraid/webgui
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the code around lines 1636-1638 with more context
sed -n '1600,1650p' emhttp/plugins/dynamix.vm.manager/include/libvirt.php | cat -nRepository: unraid/webgui
Length of output: 2068
🏁 Script executed:
#!/bin/bash
# Search for the domain_start function definition
rg -n "function domain_start" emhttp/plugins/dynamix.vm.manager/include/libvirt.php -A 30Repository: unraid/webgui
Length of output: 1318
🏁 Script executed:
#!/bin/bash
# Search for calls to domain_start to understand how it's used
rg -n "domain_start\s*\(" --type=php -C 2Repository: unraid/webgui
Length of output: 4258
🏁 Script executed:
#!/bin/bash
# Check libvirt function documentation or usage patterns in the codebase
# to understand what libvirt_domain_create_xml expects
rg -n "libvirt_domain_create_xml" --type=php -B 2 -A 2Repository: unraid/webgui
Length of output: 1471
🏁 Script executed:
#!/bin/bash
# Look at the get_domain_object function to understand what it returns
rg -n "function get_domain_object" emhttp/plugins/dynamix.vm.manager/include/libvirt.php -A 15Repository: unraid/webgui
Length of output: 692
Broken fallback code path with incorrect parameter type.
When get_domain_object fails to find a domain (returning false), the fallback at line 1636 attempts to call libvirt_domain_create_xml with a falsy value as the second argument. The function expects an XML string, but receives false/null because the domain lookup failed. This will not work as intended.
The original parameter (domain name) is overwritten at line 1630, so it's unavailable in the fallback. Either this fallback should be removed if unreachable, or the logic needs to be redesigned to handle the case where a domain cannot be looked up by name.
🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php` around lines 1636 -
1638, The fallback calls libvirt_domain_create_xml($this->conn, $dom) with $dom
being false because get_domain_object() overwrote the original domain name; fix
by preserving the original domain identifier before calling get_domain_object()
(e.g., save $origDomain = $domain), then when get_domain_object(...) returns
false either remove the fallback or construct/obtain a valid XML string for
libvirt_domain_create_xml and pass that ($origDomain or built XML) instead of
the boolean; also ensure $this->last_error = libvirt_get_last_error() only runs
after a real libvirt call and return an error/false if no valid XML can be
provided.
Comment on lines
1641
to
1694
| function manage_domain_xml($domain, $xml = null, $save = true) { | ||
| // Save or delete XML in VM directory based on $save flag | ||
| // $domain is the domain name (already validated by caller) | ||
| $xml_dir = null; | ||
| $storage = "default"; | ||
|
|
||
| // Extract storage location from VM metadata if available | ||
| if ($xml && preg_match('/<vmtemplate[^>]*storage="([^"]*)"/', $xml, $matches)) { | ||
| $storage = $matches[1]; | ||
| } | ||
|
|
||
| // Determine storage path | ||
| if ($storage === "default") { | ||
| // Read default storage location from domains.cfg | ||
| $domain_cfg = parse_ini_file('/boot/config/domain.cfg', true); | ||
| if (isset($domain_cfg['DOMAINDIR'])) { | ||
| $storage_path = rtrim($domain_cfg['DOMAINDIR'], '/'); | ||
| } else { | ||
| // Fallback to standard location | ||
| $storage_path = "/mnt/user/domains"; | ||
| } | ||
| } else { | ||
| // Storage is a pool name - construct pool path | ||
| $storage_path = "/mnt/$storage"; | ||
| } | ||
|
|
||
| // Build full VM directory path | ||
| $xml_dir = "$storage_path/$domain"; | ||
|
|
||
| // Verify directory exists | ||
| if (!is_dir($xml_dir)) { | ||
| return false; | ||
| } | ||
|
|
||
| $xml_file = $xml_dir . '/' . $domain . '.xml'; | ||
|
|
||
| if ($save === false) { | ||
| if (is_file($xml_file)) { | ||
| $backup_file = $xml_file . '.prev'; | ||
| @copy($xml_file, $backup_file); | ||
| return unlink($xml_file); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Backup existing XML before writing new content | ||
| if (is_file($xml_file)) { | ||
| $backup_file = $xml_file . '.prev'; | ||
| @copy($xml_file, $backup_file); | ||
| } | ||
|
|
||
| // Write XML to file | ||
| return file_put_contents($xml_file, $xml) !== false; | ||
| } |
Contributor
There was a problem hiding this comment.
Multiple issues in manage_domain_xml function.
-
Line 1476 calls
manage_domain_xml()as a global function, but it's defined as a class method$this->manage_domain_xml()- this will cause a fatal error. -
Line 1655:
parse_ini_filereturnsfalseon failure. Accessing$domain_cfg['DOMAINDIR']onfalsetriggers a warning. -
Line 1671-1673: Returning
falsewhen directory doesn't exist will break new VM creation where the directory hasn't been created yet. -
Inconsistent indentation (lines 1647-1673) suggests copy-paste issues.
-
Line 1664: Pool path construction
/mnt/$storageassumes all pools are mounted under/mnt/, which may not hold for all configurations.
🐛 Proposed fix
function manage_domain_xml($domain, $xml = null, $save = true) {
- // Save or delete XML in VM directory based on $save flag
- // $domain is the domain name (already validated by caller)
$xml_dir = null;
$storage = "default";
-
- // Extract storage location from VM metadata if available
- if ($xml && preg_match('/<vmtemplate[^>]*storage="([^"]*)"/', $xml, $matches)) {
- $storage = $matches[1];
- }
-
- // Determine storage path
- if ($storage === "default") {
- // Read default storage location from domains.cfg
- $domain_cfg = parse_ini_file('/boot/config/domain.cfg', true);
- if (isset($domain_cfg['DOMAINDIR'])) {
- $storage_path = rtrim($domain_cfg['DOMAINDIR'], '/');
- } else {
- // Fallback to standard location
- $storage_path = "/mnt/user/domains";
- }
- } else {
- // Storage is a pool name - construct pool path
- $storage_path = "/mnt/$storage";
- }
-
- // Build full VM directory path
- $xml_dir = "$storage_path/$domain";
-
- // Verify directory exists
- if (!is_dir($xml_dir)) {
- return false;
- }
+
+ // Extract storage location from VM metadata if available
+ if ($xml && preg_match('/<vmtemplate[^>]*storage="([^"]*)"/', $xml, $matches)) {
+ $storage = $matches[1];
+ }
+
+ // Determine storage path
+ if ($storage === "default") {
+ $domain_cfg = `@parse_ini_file`('/boot/config/domain.cfg', true);
+ if ($domain_cfg && isset($domain_cfg['DOMAINDIR'])) {
+ $storage_path = rtrim($domain_cfg['DOMAINDIR'], '/');
+ } else {
+ $storage_path = "/mnt/user/domains";
+ }
+ } else {
+ $storage_path = "/mnt/$storage";
+ }
+
+ $xml_dir = "$storage_path/$domain";
+
+ // For delete operations, directory must exist
+ if ($save === false && !is_dir($xml_dir)) {
+ return true; // Nothing to delete
+ }
$xml_file = $xml_dir . '/' . $domain . '.xml';🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php` around lines 1641 -
1694, The manage_domain_xml function is currently a global function but callers
use $this->manage_domain_xml(); convert it into a class method (e.g., public
function manage_domain_xml(...)) so it can be called via $this; guard the
parse_ini_file call by checking $domain_cfg !== false before accessing
$domain_cfg['DOMAINDIR']; avoid assuming every pool is under /mnt by checking
the constructed $storage_path exists and if not falling back to a sensible
default (e.g., the domains fallback used previously) or resolving pool mounts
via existing pool lookup helper if available; change the behavior when $xml_dir
doesn't exist so that on save=true you attempt to create the directory (mkdir -p
with appropriate permissions and error handling) and on save=false return true
if nothing exists to delete; tidy indentation in the manage_domain_xml method to
match surrounding class style.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php`:
- Around line 1476-1479: The domain_define calls are using the wrong signature
(passing $this->conn as first arg); update both calls to use domain_define($xml,
$autostart=false) style so the XML string is the first parameter and optional
autostart flag second—i.e., call $this->domain_define($xml) for the new define
and $this->domain_define($old_xml) for the rollback, keeping the existing
$this->last_error = libvirt_get_last_error() and return false path intact.
♻️ Duplicate comments (3)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)
1636-1638: Fallback passes a non-XML value intolibvirt_domain_create_xml.When
get_domain_object()fails,$domis falsy, but Line 1636 passes it intolibvirt_domain_create_xml, which expects an XML string. This path cannot succeed as-is; either preserve the original name/XML or return an error.
1641-1694: Hardenmanage_domain_xmlerror handling and directory creation.The current flow still returns
falsewhen the VM directory doesn't exist (Line 1671), andparse_ini_file()failures can cause warnings before$domain_cfg['DOMAINDIR']is accessed. This risks silently skipping XML persistence for new VMs.
1825-1829: Ensuremanage_domain_xmlreceives a domain name, not a resource.
domain_undefine()may be called with a libvirt domain resource (e.g., fromdomain_delete()), butmanage_domain_xml()expects a name string to build paths. This can produce invalid paths and leave XML behind.🐛 Proposed fix
# PR1722 Remove XML from VM directory using storage metadata $xml = libvirt_domain_get_xml_desc($dom, 0); if ($xml) { - $this->manage_domain_xml($domain, $xml, false); + $domain_name = is_resource($domain) ? libvirt_domain_get_name($dom) : $domain; + if ($domain_name) { + $this->manage_domain_xml($domain_name, $xml, false); + } }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into Remove-Vm-Image
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy`:
- Around line 108-116: The current str_replace assumes $default_domain_dir
begins with "/mnt/user/" which can be false; change the assignment of $path_root
to only rewrite the pool prefix when the path actually starts with
"/mnt/<pool>/": e.g. use a regex replacement like
preg_replace('#^/mnt/[^/]+/#','/mnt/'.$metadata_storage.'/',
$default_domain_dir) so a non-/mnt/user/ DOMAINDIR (e.g. /mnt/cache/domains) is
correctly mapped to the target pool, and keep $storage_name = $metadata_storage
unchanged; ensure you fall back to the original $default_domain_dir if the regex
doesn't match.
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate`:
- Around line 103-109: The XML-parse failure branch unconditionally removes
$dest_file; change it to only unlink when we actually created/copyed the
destination by checking the $copied flag (and still respect $dry_run). Update
the block that returns on $xml === false to perform: if (!$dry_run && $copied)
`@unlink`($dest_file); then return the same failure array (referencing $xml,
$dry_run, $copied, $dest_file, and $xml_old_path).
♻️ Duplicate comments (5)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy (1)
1-2: Trailing space in shebang line.Line 1 has a trailing space after
/usr/bin/phpwhich could cause issues on some systems.Proposed fix
-#!/usr/bin/php +#!/usr/bin/phpemhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore (1)
1-2: Trailing space in shebang line.Same issue as in libvirtcopy - trailing space after
/usr/bin/php.Proposed fix
-#!/usr/bin/php +#!/usr/bin/phpemhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
1653-1673: Missing error handling forparse_ini_fileand overly strict directory check.
Line 1655:
parse_ini_filereturnsfalseon failure. Accessing$domain_cfg['DOMAINDIR']onfalsetriggers a warning.Lines 1671-1673: Returning
falsewhen the directory doesn't exist will break new VM creation where the directory hasn't been created yet.Proposed fix
// Determine storage path if ($storage === "default") { // Read default storage location from domains.cfg - $domain_cfg = parse_ini_file('/boot/config/domain.cfg', true); - if (isset($domain_cfg['DOMAINDIR'])) { + $domain_cfg = `@parse_ini_file`('/boot/config/domain.cfg', true); + if ($domain_cfg && isset($domain_cfg['DOMAINDIR'])) { $storage_path = rtrim($domain_cfg['DOMAINDIR'], '/'); } else { // Fallback to standard location $storage_path = "/mnt/user/domains"; } } else { // Storage is a pool name - construct pool path $storage_path = "/mnt/$storage"; } // Build full VM directory path $xml_dir = "$storage_path/$domain"; - // Verify directory exists - if (!is_dir($xml_dir)) { - return false; + // For save operations, create directory if needed; for delete, skip if missing + if (!is_dir($xml_dir)) { + if ($save === false) { + return true; // Nothing to delete + } + // Attempt to create directory for save operation + if (!@mkdir($xml_dir, 0755, true)) { + return false; + } }emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init (1)
53-59: Missing error handling for the second mount/umount block.The first mount block (lines 42-52) now has proper error handling, but this block for mounting
IMAGE_FILElacks the same safeguards. If mount fails, rsync will write to an empty mount point.Proposed fix
elif [[ "$IMAGE_FILE" == *.img ]]; then log "Mounting $IMAGE_FILE to $TMP_MNT" mkdir -p "$TMP_MNT" - mount "$IMAGE_FILE" "$TMP_MNT" + if ! mount "$IMAGE_FILE" "$TMP_MNT"; then + log "ERROR: Failed to mount $IMAGE_FILE" + rm -rf "$TMP_MNT" + exit 1 + fi log "Copying full contents from directory $OLD_IMAGE_FILE to image" - rsync -a --exclude="$IMG_FILE_NAME" --exclude='*.bak-*.img' "$OLD_IMAGE_FILE/" "$TMP_MNT/" - umount "$TMP_MNT" + if ! rsync -a --exclude="$IMG_FILE_NAME" --exclude='*.bak-*.img' "$OLD_IMAGE_FILE/" "$TMP_MNT/"; then + log "WARNING: rsync encountered errors" + fi + umount "$TMP_MNT" || log "WARNING: Failed to unmount $TMP_MNT" + rmdir "$TMP_MNT" 2>/dev/null elseemhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate (1)
116-124: Rollback on line 119 also happens unconditionally.Same issue as line 104 - the rollback should only occur if we actually copied the file.
Proposed fix
// Write updated XML to new location $xml_formatted = $xml->asXML(); if (!$dry_run && !@file_put_contents($xml_new_path, $xml_formatted)) { - `@unlink`($dest_file); // Rollback + if ($copied) `@unlink`($dest_file); // Rollback only if we copied return [ 'success' => false, 'error' => "Failed to write updated XML to: $xml_new_path" ]; }
🧹 Nitpick comments (5)
emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate (1)
314-326: Consider consistent error output format.The script outputs JSON results but uses
die()with plain text for fatal errors. This inconsistency could confuse callers expecting JSON.Suggested improvement
$lv = libvirt_connect('qemu:///system', false); if (!$lv) { - die("ERROR: Failed to connect to libvirt\n"); + echo json_encode(['error' => 'Failed to connect to libvirt'], JSON_PRETTY_PRINT); + exit(1); } // Get all valid VM UUIDs $domains = libvirt_list_domains($lv); if ($domains === false) { - die("ERROR: Failed to list domains\n"); + echo json_encode(['error' => 'Failed to list domains'], JSON_PRETTY_PRINT); + exit(1); }emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init (1)
105-108: Consider adding error logging for libvirtrestore invocation.If
libvirtrestorefails or returns a non-zero exit code, it currently goes unnoticed. Adding basic error logging would help with troubleshooting.Suggested improvement
# Copy XML from VM Directories to QEMU directory/ -/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore +if ! /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore; then + log "WARNING: libvirtrestore encountered errors" +fi #emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy (1)
161-175: Inconsistent brace style affects readability.Line 174 places the
elseand its statement on the same line without braces, while theifblock uses braces. This inconsistency reduces readability.Proposed fix
if ($vmdetail['exists']) { $res = copy_if_different($from_file, $to_file, false); $msg = "$vm from:$from_file to:$to_file"; if (!empty($res['error'])) { $msg .= " ERROR:" . $res['error']; } elseif (!empty($res['copied'])) { $msg .= " COPIED"; } elseif (!empty($res['would_copy'])) { $msg .= " WOULD_COPY"; } else { $msg .= " SKIPPED_IDENTICAL"; } file_put_contents("/tmp/Stopcopy", $msg . "\n", FILE_APPEND); - } else file_put_contents("/tmp/Stopcopy","Nocpy $vm from:$from_file to:$to_file\n",FILE_APPEND); `#echo` " from:$from_file to:$to_file"; + } else { + file_put_contents("/tmp/Stopcopy", "Nocpy $vm from:$from_file to:$to_file\n", FILE_APPEND); + } }emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore (1)
30-33: JSON validation could be more robust.The current check allows
nullto pass if there's no JSON error (valid JSON literalnull), but the subsequentforeach($vms as ...)would fail. Consider also checking that$vmsis an array.Proposed fix
$vms = json_decode($vmsjson, true); -if ($vms === null && json_last_error() !== JSON_ERROR_NONE) { +if (!is_array($vms)) { die("Invalid JSON in configuration file: " . json_last_error_msg() . "\n"); }emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
1641-1650: Inconsistent indentation inmanage_domain_xml.The function has mixed indentation - some blocks use extra tabs that don't align with the class style.
Suggested fix
function manage_domain_xml($domain, $xml = null, $save = true) { // Save or delete XML in VM directory based on $save flag // $domain is the domain name (already validated by caller) $xml_dir = null; $storage = "default"; - - // Extract storage location from VM metadata if available - if ($xml && preg_match('/<vmtemplate[^>]*storage="([^"]*)"/', $xml, $matches)) { - $storage = $matches[1]; - } + + // Extract storage location from VM metadata if available + if ($xml && preg_match('/<vmtemplate[^>]*storage="([^"]*)"/', $xml, $matches)) { + $storage = $matches[1]; + }
Comment on lines
+108
to
+116
| if ($metadata_storage === null || $metadata_storage === '' || strtolower($metadata_storage) === 'default') { | ||
| /* TRUE default storage */ | ||
| $path_root = $default_domain_dir; // e.g. /mnt/user/domains2 | ||
| $storage_name = 'default'; | ||
| } else { | ||
| /* Explicit Unraid pool */ | ||
| $path_root = str_replace("/mnt/user/","/mnt/$metadata_storage/",$default_domain_dir); | ||
| $storage_name = $metadata_storage; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the libvirtcopy file
find . -name "libvirtcopy" -type fRepository: unraid/webgui
Length of output: 112
🏁 Script executed:
# Check where $default_domain_dir is initialized in libvirtcopy
rg -n "default_domain_dir" emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy -B5 -A2Repository: unraid/webgui
Length of output: 1248
🏁 Script executed:
# Search for DOMAINDIR variable initialization and usage
rg -n "DOMAINDIR" emhttp/plugins/dynamix.vm.manager/ -B3 -A3Repository: unraid/webgui
Length of output: 15216
🏁 Script executed:
# Check if there's any configuration or validation for domain directories
rg -n "/mnt/user/domains" emhttp/plugins/dynamix.vm.manager/ -B2 -A2Repository: unraid/webgui
Length of output: 3698
Path resolution may fail when DOMAINDIR is not under /mnt/user/.
The str_replace("/mnt/user/","/mnt/$metadata_storage/",$default_domain_dir) assumes the configured domain directory starts with /mnt/user/. However, the DOMAINDIR setting in VMSettings allows users to configure any path under /mnt, and the UI explicitly checks whether the path starts with /mnt/user/ (e.g., lines 879, 1047 in Custom.form.php), indicating non-user paths are valid configurations. If DOMAINDIR is configured to a different location (e.g., /mnt/cache/domains), the string replacement will fail silently and return the original path unchanged, leading to incorrect storage pool resolution.
🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy` around lines 108 -
116, The current str_replace assumes $default_domain_dir begins with
"/mnt/user/" which can be false; change the assignment of $path_root to only
rewrite the pool prefix when the path actually starts with "/mnt/<pool>/": e.g.
use a regex replacement like
preg_replace('#^/mnt/[^/]+/#','/mnt/'.$metadata_storage.'/',
$default_domain_dir) so a non-/mnt/user/ DOMAINDIR (e.g. /mnt/cache/domains) is
correctly mapped to the target pool, and keep $storage_name = $metadata_storage
unchanged; ensure you fall back to the original $default_domain_dir if the regex
doesn't match.
Comment on lines
+103
to
+109
| if ($xml === false) { | ||
| if (!$dry_run) @unlink($dest_file); // Rollback | ||
| return [ | ||
| 'success' => false, | ||
| 'error' => "Failed to parse XML: $xml_old_path" | ||
| ]; | ||
| } |
Contributor
There was a problem hiding this comment.
Rollback on line 104 still happens unconditionally.
The fix at line 93 correctly checks $copied before rollback, but this block still unconditionally deletes $dest_file on XML parse failure. If the destination file pre-existed (wasn't copied), this incorrectly removes it.
Proposed fix
if ($xml === false) {
- if (!$dry_run) `@unlink`($dest_file); // Rollback
+ if ($copied) `@unlink`($dest_file); // Rollback only if we copied
return [
'success' => false,
'error' => "Failed to parse XML: $xml_old_path"
];
}📝 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.
Suggested change
| if ($xml === false) { | |
| if (!$dry_run) @unlink($dest_file); // Rollback | |
| return [ | |
| 'success' => false, | |
| 'error' => "Failed to parse XML: $xml_old_path" | |
| ]; | |
| } | |
| if ($xml === false) { | |
| if ($copied) `@unlink`($dest_file); // Rollback only if we copied | |
| return [ | |
| 'success' => false, | |
| 'error' => "Failed to parse XML: $xml_old_path" | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate` around lines 103 -
109, The XML-parse failure branch unconditionally removes $dest_file; change it
to only unlink when we actually created/copyed the destination by checking the
$copied flag (and still respect $dry_run). Update the block that returns on $xml
=== false to perform: if (!$dry_run && $copied) `@unlink`($dest_file); then return
the same failure array (referencing $xml, $dry_run, $copied, $dest_file, and
$xml_old_path).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.