Updates for libvirt image removal and use directory #1722
Conversation
WalkthroughAdds UI tracking for IMAGE_FILE changes and implements synchronization/migration between old and new VM image paths; records VM locations at libvirt stop and prepares restoration of VM XMLs on libvirt start, plus supporting filesystem utility and migration scripts. 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 Libvirt as libvirtd
participant RC as rc.libvirt (stop hook)
participant Copy as libvirtcopy
participant VMSJSON as /boot/.../vms.json
participant Save as savehook.php
participant Restore as libvirtrestore
participant FS as Storage
Admin->>UI: update IMAGE_FILE
UI->>CFG: write IMAGE_FILE and OLD_IMAGE_FILE
Activate Init
Init->>CFG: read IMAGE_FILE, OLD_IMAGE_FILE
alt both set and differ
Init->>FS: mount/backup old .img (if .img)
Init->>FS: rsync from old -> new (cases: .img->dir, dir->dir, dir->.img)
Init->>CFG: update OLD_IMAGE_FILE
else skip sync
Init->>CFG: log skipped
end
Deactivate Init
Admin->>Libvirt: stop
Libvirt->>RC: invoke pre-stop hooks
RC->>Copy: run libvirtcopy
Copy->>Libvirt: query domains
Copy->>VMSJSON: write VM metadata and planned copy actions
loop for each VM stopped
Save->>VMSJSON: read metadata
Save->>FS: copy /etc/libvirt/qemu/{vm}.xml -> vm backup path
end
Libvirt->>Libvirt: stop services
Libvirt start->>Restore: run libvirtrestore
Restore->>VMSJSON: read metadata
Restore->>FS: copy_if_different VM XMLs back to /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}
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.
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
|
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.
| <?endif;?> | ||
|
|
||
| <?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs'):?> | ||
| <?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs' && strpos($domain_cfg['IMAGE_FILE'],".img")) :?> |
There was a problem hiding this comment.
Fix imprecise string matching for .img check.
The condition strpos($domain_cfg['IMAGE_FILE'],".img") has potential issues:
- It will return false when ".img" is at position 0, though this is unlikely for a path
- More importantly, it will match ".img" appearing anywhere in the path, including in directory names (e.g.,
/mnt/user/my.images/libvirt/or/path/.img-backup/libvirt/)
Since the intent is to show this section only for image files (not directories), consider using a more precise check:
-<?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs' && strpos($domain_cfg['IMAGE_FILE'],".img")) :?>
+<?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs' && substr($domain_cfg['IMAGE_FILE'], -4) === '.img') :?>This checks if the path ends with .img rather than containing it anywhere.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs' && strpos($domain_cfg['IMAGE_FILE'],".img")) :?> | |
| <?if ($libvirt_up && trim(shell_exec('stat -c %T -f /etc/libvirt'))=='btrfs' && substr($domain_cfg['IMAGE_FILE'], -4) === '.img') :?> |
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/VMSettings.page around line 303, the
current check strpos($domain_cfg['IMAGE_FILE'],".img") is imprecise (matches
anywhere and fails when at position 0); replace it with an end-of-string match
to ensure the path is an image file — for example, use a case-insensitive regex
or a suffix check such as preg_match('/\.img$/i',
trim($domain_cfg['IMAGE_FILE'])) or
strtolower(substr(trim($domain_cfg['IMAGE_FILE']), -4)) === '.img' so only paths
that actually end with ".img" trigger the condition.
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.
| :vms_libvirt_secondary_volume_help: | ||
| This is a location for storing previous versions of xml and nvram at change. | ||
| :end |
There was a problem hiding this comment.
Fix grammar: "at change" is awkward.
The phrase "at change" should be replaced with clearer alternatives. This issue was previously flagged and remains unresolved.
📝 Suggested fix
:vms_libvirt_secondary_volume_help:
- This is a location for storing previous versions of xml and nvram at change.
+ This is a location for storing previous versions of XML and NVRAM when changes are made.Or alternatively:
- This is a location for storing previous versions of xml and nvram at change.
+ This is a location for storing previous versions of XML and NVRAM on change.📝 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.
| :vms_libvirt_secondary_volume_help: | |
| This is a location for storing previous versions of xml and nvram at change. | |
| :end | |
| :vms_libvirt_secondary_volume_help: | |
| This is a location for storing previous versions of XML and NVRAM when changes are made. | |
| :end |
🤖 Prompt for AI Agents
In @emhttp/languages/en_US/helptext.txt around lines 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.
| :vms_libvirt_secondary_location_help: | ||
| This is a directory for storing previous versions of xml and nvram at change. Does not need to be specified. | ||
| :end |
There was a problem hiding this comment.
Fix grammar: "at change" is awkward.
The phrase "at change" should be replaced with clearer alternatives. This issue was previously flagged and remains unresolved. Additionally, "XML" and "NVRAM" should be capitalized for consistency with the rest of the documentation.
📝 Suggested fix
:vms_libvirt_secondary_location_help:
- This is a directory for storing previous versions of xml and nvram at change. Does not need to be specified.
+ This is a directory for storing previous versions of XML and NVRAM when changes are made. Does not need to be specified.Or alternatively:
- This is a directory for storing previous versions of xml and nvram at change. Does not need to be specified.
+ This is a directory for storing previous versions of XML and NVRAM on change; it does not need to be specified.📝 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.
| :vms_libvirt_secondary_location_help: | |
| This is a directory for storing previous versions of xml and nvram at change. Does not need to be specified. | |
| :end | |
| :vms_libvirt_secondary_location_help: | |
| This is a directory for storing previous versions of XML and NVRAM when changes are made. Does not need to be specified. | |
| :end |
🤖 Prompt for AI Agents
In @emhttp/languages/en_US/helptext.txt around lines 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.").
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.
| /* Shell-safe path (VM name quoted) */ | ||
| $path = $path_root | ||
| ? $path_root . '/"' . $vm_name . '"' | ||
| : null; |
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.
| /* 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.
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.
| * --------------------------------------------------------- */ | ||
| #print_r($vms); | ||
|
|
||
| file_put_contents("/boot/config/plugins/dynamix.vm.manager/vms.json",json_encode($vms,JSON_PRETTY_PRINT)); |
There was a problem hiding this comment.
Add directory creation before writing vms.json.
As flagged in a previous review, the directory /boot/config/plugins/dynamix.vm.manager/ may not exist when this script runs. The file_put_contents() call will fail silently if the parent directory is missing.
📁 Proposed fix to ensure directory exists
+$json_path = "/boot/config/plugins/dynamix.vm.manager/vms.json";
+$json_dir = dirname($json_path);
+if (!is_dir($json_dir)) {
+ if (!@mkdir($json_dir, 0755, true)) {
+ die("Failed to create directory: $json_dir\n");
+ }
+}
-file_put_contents("/boot/config/plugins/dynamix.vm.manager/vms.json",json_encode($vms,JSON_PRETTY_PRINT));
+if (file_put_contents($json_path, json_encode($vms, JSON_PRETTY_PRINT)) === false) {
+ die("Failed to write vms.json\n");
+}📝 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.
| file_put_contents("/boot/config/plugins/dynamix.vm.manager/vms.json",json_encode($vms,JSON_PRETTY_PRINT)); | |
| $json_path = "/boot/config/plugins/dynamix.vm.manager/vms.json"; | |
| $json_dir = dirname($json_path); | |
| if (!is_dir($json_dir)) { | |
| if (!@mkdir($json_dir, 0755, true)) { | |
| die("Failed to create directory: $json_dir\n"); | |
| } | |
| } | |
| if (file_put_contents($json_path, json_encode($vms, JSON_PRETTY_PRINT)) === false) { | |
| die("Failed to write vms.json\n"); | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy at 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.
| $vmsjson = file_get_contents("/boot/config/plugins/dynamix.vm.manager/vms.json"); | ||
| $vms = json_decode($vmsjson,true); |
There was a problem hiding this comment.
Add error handling for file operations.
The script lacks error handling for reading and parsing vms.json. If the file is missing or contains invalid JSON, the script will fail with warnings or fatal errors.
🛡️ Proposed fix with error handling
-$vmsjson = file_get_contents("/boot/config/plugins/dynamix.vm.manager/vms.json");
-$vms = json_decode($vmsjson,true);
+$json_path = "/boot/config/plugins/dynamix.vm.manager/vms.json";
+
+if (!file_exists($json_path)) {
+ die("Configuration file not found: $json_path\n");
+}
+
+$vmsjson = file_get_contents($json_path);
+if ($vmsjson === false) {
+ die("Failed to read configuration file: $json_path\n");
+}
+
+$vms = json_decode($vmsjson, true);
+if ($vms === null && json_last_error() !== JSON_ERROR_NONE) {
+ die("Invalid JSON in configuration file: " . json_last_error_msg() . "\n");
+}📝 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.
| $vmsjson = file_get_contents("/boot/config/plugins/dynamix.vm.manager/vms.json"); | |
| $vms = json_decode($vmsjson,true); | |
| $json_path = "/boot/config/plugins/dynamix.vm.manager/vms.json"; | |
| if (!file_exists($json_path)) { | |
| die("Configuration file not found: $json_path\n"); | |
| } | |
| $vmsjson = file_get_contents($json_path); | |
| if ($vmsjson === false) { | |
| die("Failed to read configuration file: $json_path\n"); | |
| } | |
| $vms = json_decode($vmsjson, true); | |
| if ($vms === null && json_last_error() !== JSON_ERROR_NONE) { | |
| die("Invalid JSON in configuration file: " . json_last_error_msg() . "\n"); | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore around lines 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.
| $cfg = "/boot/config/plugins/dynamix.vm.manager/vms.json"; | ||
| $vms = json_decode(file_get_contents($cfg),true); |
There was a problem hiding this comment.
Add error handling for file operations.
The script lacks error handling for file_get_contents() and json_decode(). If the vms.json file is missing, unreadable, or contains invalid JSON, the script will fail with warnings or fatal errors.
🛡️ Proposed fix with error handling
-$cfg = "/boot/config/plugins/dynamix.vm.manager/vms.json";
-$vms = json_decode(file_get_contents($cfg),true);
+$cfg = "/boot/config/plugins/dynamix.vm.manager/vms.json";
+
+if (!file_exists($cfg)) {
+ error_log("savehook: Configuration file not found: $cfg");
+ exit(1);
+}
+
+$json = file_get_contents($cfg);
+if ($json === false) {
+ error_log("savehook: Failed to read configuration file: $cfg");
+ exit(1);
+}
+
+$vms = json_decode($json, true);
+if ($vms === null && json_last_error() !== JSON_ERROR_NONE) {
+ error_log("savehook: Invalid JSON in configuration file: " . json_last_error_msg());
+ exit(1);
+}📝 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.
| $cfg = "/boot/config/plugins/dynamix.vm.manager/vms.json"; | |
| $vms = json_decode(file_get_contents($cfg),true); | |
| $cfg = "/boot/config/plugins/dynamix.vm.manager/vms.json"; | |
| if (!file_exists($cfg)) { | |
| error_log("savehook: Configuration file not found: $cfg"); | |
| exit(1); | |
| } | |
| $json = file_get_contents($cfg); | |
| if ($json === false) { | |
| error_log("savehook: Failed to read configuration file: $cfg"); | |
| exit(1); | |
| } | |
| $vms = json_decode($json, true); | |
| if ($vms === null && json_last_error() !== JSON_ERROR_NONE) { | |
| error_log("savehook: Invalid JSON in configuration file: " . json_last_error_msg()); | |
| exit(1); | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/savehook.php around lines 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.
| 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); | ||
| } |
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);
+ }
+}| # Save VM locations | ||
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy | ||
| # |
There was a problem hiding this comment.
Add error handling for the libvirtcopy invocation.
The pre-stop hook invokes libvirtcopy without checking if the script exists, is executable, or executes successfully. If the script fails, the error is silently ignored and libvirtd stop continues.
🛡️ Proposed fix with error handling
libvirtd_stop(){
# Save VM locations
- /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy
- #
+ LIBVIRTCOPY="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy"
+ if [[ -x "$LIBVIRTCOPY" ]]; then
+ "$LIBVIRTCOPY" || log "Warning: Failed to save VM locations"
+ else
+ log "Warning: libvirtcopy script not found or not executable"
+ fi
+
log "Stopping $DAEMON..."
if [[ ! -f $LIBVIRTD_PIDFILE ]]; then📝 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.
| # Save VM locations | |
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy | |
| # | |
| # Save VM locations | |
| LIBVIRTCOPY="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy" | |
| if [[ -x "$LIBVIRTCOPY" ]]; then | |
| "$LIBVIRTCOPY" || log "Warning: Failed to save VM locations" | |
| else | |
| log "Warning: libvirtcopy script not found or not executable" | |
| fi | |
🤖 Prompt for AI Agents
In @etc/rc.d/rc.libvirt around lines 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.
SimonFair
changed the title
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.
| function dir_remove($dir) { | ||
| if (!is_dir($dir)) return false; | ||
| $items = scandir($dir); | ||
| foreach ($items as $item) { | ||
| if ($item === '.' || $item === '..') continue; | ||
| $path = $dir . DIRECTORY_SEPARATOR . $item; | ||
| if (is_dir($path)) { | ||
| dir_remove($path); | ||
| } else { | ||
| @unlink($path); | ||
| } | ||
| } | ||
| return @rmdir($dir); | ||
| } |
There was a problem hiding this comment.
Recursive removal doesn't propagate child failure status.
When dir_remove($path) fails on a nested directory (line 93), the return value is ignored and iteration continues. This could leave a partially deleted directory while returning true from parent calls if only the final rmdir succeeds.
Suggested fix to propagate failures
function dir_remove($dir) {
if (!is_dir($dir)) return false;
$items = scandir($dir);
+ if ($items === false) return false;
foreach ($items as $item) {
if ($item === '.' || $item === '..') continue;
$path = $dir . DIRECTORY_SEPARATOR . $item;
if (is_dir($path)) {
- dir_remove($path);
+ if (!dir_remove($path)) return false;
} else {
- @unlink($path);
+ if (!@unlink($path)) return false;
}
}
return @rmdir($dir);
}📝 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 dir_remove($dir) { | |
| if (!is_dir($dir)) return false; | |
| $items = scandir($dir); | |
| foreach ($items as $item) { | |
| if ($item === '.' || $item === '..') continue; | |
| $path = $dir . DIRECTORY_SEPARATOR . $item; | |
| if (is_dir($path)) { | |
| dir_remove($path); | |
| } else { | |
| @unlink($path); | |
| } | |
| } | |
| return @rmdir($dir); | |
| } | |
| function dir_remove($dir) { | |
| if (!is_dir($dir)) return false; | |
| $items = scandir($dir); | |
| if ($items === false) return false; | |
| foreach ($items as $item) { | |
| if ($item === '.' || $item === '..') continue; | |
| $path = $dir . DIRECTORY_SEPARATOR . $item; | |
| if (is_dir($path)) { | |
| if (!dir_remove($path)) return false; | |
| } else { | |
| if (!@unlink($path)) return false; | |
| } | |
| } | |
| return @rmdir($dir); | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/include/fs_helpers.php around lines 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.
| # Read values from domain.cfg | ||
| eval $(grep -E '^(IMAGE_FILE|OLD_IMAGE_FILE)=' "$DOMAIN_CFG") | ||
|
|
||
| # Remove quotes | ||
| IMAGE_FILE="${IMAGE_FILE%\"}" | ||
| IMAGE_FILE="${IMAGE_FILE#\"}" | ||
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE%\"}" | ||
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE#\"}" |
There was a problem hiding this comment.
Potential command injection via eval on config file.
Using eval to parse config values is risky if domain.cfg is writable by untrusted processes or contains malformed content. A safer approach uses direct parsing without shell interpretation.
Safer alternative using grep/sed
-# Read values from domain.cfg
-eval $(grep -E '^(IMAGE_FILE|OLD_IMAGE_FILE)=' "$DOMAIN_CFG")
+# Read values from domain.cfg safely (no eval)
+IMAGE_FILE=$(grep -E '^IMAGE_FILE=' "$DOMAIN_CFG" | head -1 | cut -d= -f2-)
+OLD_IMAGE_FILE=$(grep -E '^OLD_IMAGE_FILE=' "$DOMAIN_CFG" | head -1 | cut -d= -f2-)📝 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.
| # Read values from domain.cfg | |
| eval $(grep -E '^(IMAGE_FILE|OLD_IMAGE_FILE)=' "$DOMAIN_CFG") | |
| # Remove quotes | |
| IMAGE_FILE="${IMAGE_FILE%\"}" | |
| IMAGE_FILE="${IMAGE_FILE#\"}" | |
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE%\"}" | |
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE#\"}" | |
| # Read values from domain.cfg safely (no eval) | |
| IMAGE_FILE=$(grep -E '^IMAGE_FILE=' "$DOMAIN_CFG" | head -1 | cut -d= -f2-) | |
| OLD_IMAGE_FILE=$(grep -E '^OLD_IMAGE_FILE=' "$DOMAIN_CFG" | head -1 | cut -d= -f2-) | |
| # Remove quotes | |
| IMAGE_FILE="${IMAGE_FILE%\"}" | |
| IMAGE_FILE="${IMAGE_FILE#\"}" | |
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE%\"}" | |
| OLD_IMAGE_FILE="${OLD_IMAGE_FILE#\"}" |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init around lines 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.
| cp -p "$OLD_IMAGE_FILE" "$BACKUP_PATH" | ||
|
|
||
| log "Mounting $OLD_IMAGE_FILE to $TMP_MNT" | ||
| mkdir -p "$TMP_MNT" | ||
| mount "$OLD_IMAGE_FILE" "$TMP_MNT" | ||
| log "Copying full contents from image to directory $IMAGE_FILE" | ||
| rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/" | ||
| umount "$TMP_MNT" |
There was a problem hiding this comment.
Mount operations lack error checking; failure leaves stale mount point.
If mount fails on line 41, the script continues to rsync from an empty mount point. Similarly, if the mount point already has something mounted, this could cause data corruption.
Suggested hardening with error checks
log "Mounting $OLD_IMAGE_FILE to $TMP_MNT"
mkdir -p "$TMP_MNT"
- mount "$OLD_IMAGE_FILE" "$TMP_MNT"
+ if ! mount "$OLD_IMAGE_FILE" "$TMP_MNT"; then
+ log "ERROR: Failed to mount $OLD_IMAGE_FILE"
+ 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/"
- umount "$TMP_MNT"
+ if ! rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/"; then
+ log "WARNING: rsync encountered errors"
+ fi
+ umount "$TMP_MNT" || log "WARNING: Failed to unmount $TMP_MNT"
+ rmdir "$TMP_MNT" 2>/dev/nullApply similar error handling to the other mount/umount blocks (lines 46-51).
📝 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.
| cp -p "$OLD_IMAGE_FILE" "$BACKUP_PATH" | |
| log "Mounting $OLD_IMAGE_FILE to $TMP_MNT" | |
| mkdir -p "$TMP_MNT" | |
| mount "$OLD_IMAGE_FILE" "$TMP_MNT" | |
| log "Copying full contents from image to directory $IMAGE_FILE" | |
| rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/" | |
| umount "$TMP_MNT" | |
| cp -p "$OLD_IMAGE_FILE" "$BACKUP_PATH" | |
| log "Mounting $OLD_IMAGE_FILE to $TMP_MNT" | |
| mkdir -p "$TMP_MNT" | |
| if ! mount "$OLD_IMAGE_FILE" "$TMP_MNT"; then | |
| log "ERROR: Failed to mount $OLD_IMAGE_FILE" | |
| rm -rf "$TMP_MNT" | |
| exit 1 | |
| fi | |
| log "Copying full contents from image to directory $IMAGE_FILE" | |
| if ! rsync -a --exclude="$OLD_IMG_FILE_NAME" "$TMP_MNT/" "$IMAGE_FILE/"; then | |
| log "WARNING: rsync encountered errors" | |
| fi | |
| umount "$TMP_MNT" || log "WARNING: Failed to unmount $TMP_MNT" | |
| rmdir "$TMP_MNT" 2>/dev/null |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init around lines 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.
| #!/usr/bin/php | ||
| <?PHP |
There was a problem hiding this comment.
Trailing space in shebang line.
Line 1 has a trailing space after /usr/bin/php which could cause issues on some systems.
-#!/usr/bin/php
+#!/usr/bin/php📝 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.
| #!/usr/bin/php | |
| <?PHP | |
| #!/usr/bin/php | |
| <?PHP |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate around lines 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.
| // Read old XML | ||
| if (!file_exists($xml_old_path)) { | ||
| @unlink($dest_file); // Rollback | ||
| return [ | ||
| 'success' => false, | ||
| 'error' => "XML file not found: $xml_old_path" | ||
| ]; | ||
| } |
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).
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.