Updates for libvirt image removal and use diretory #1722
Conversation
WalkthroughThis change implements a VM image file synchronization and backup system for the Unraid libvirt plugin. It adds UI controls to track IMAGE_FILE changes, syncs data between old and new image paths, collects VM metadata during shutdown, and prepares restoration of VM configurations on service restart. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as VMSettings.page
participant libvirt_init as libvirt_init (script)
participant Config as domain.cfg
participant LibvirtSvc as Libvirt Service
participant libvirtcopy as libvirtcopy (script)
participant savehook as savehook.php
participant Storage as Storage Backend
User->>UI: Change IMAGE_FILE setting
UI->>Config: Save IMAGE_FILE + track OLD_IMAGE_FILE
activate libvirt_init
libvirt_init->>Config: Read IMAGE_FILE & OLD_IMAGE_FILE
alt Files differ and old path exists
libvirt_init->>Storage: Create timestamped backup of old image
libvirt_init->>Storage: Mount old image (if .img)
libvirt_init->>Storage: rsync contents to new location
libvirt_init->>Config: Update OLD_IMAGE_FILE to current path
else Files match or old path missing
libvirt_init->>Config: Log sync skipped
end
deactivate libvirt_init
User->>LibvirtSvc: Stop libvirt service
activate LibvirtSvc
libvirtcopy->>LibvirtSvc: Query all VM domains
libvirtcopy->>Config: Read VM storage metadata
libvirtcopy->>Storage: Write vms.json with VM locations
loop For each VM
savehook->>Config: Read vms.json
savehook->>Storage: Copy /etc/libvirt/qemu/{vm}.xml to backup path
end
LibvirtSvc->>LibvirtSvc: Stop services
deactivate LibvirtSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
| foreach ($vms as $vm => $vmdetail) { | ||
| file_put_contents("/tmp/Stopcopy",""); | ||
| $from_file = "/etc/libvirt/qemu/$vm.xml"; | ||
| $to_file = $vmdetail['path']."/$vm.xml"; | ||
| #echo " from:$from_file to:$to_file"; | ||
| if ($vmdetail['exists']) { | ||
| file_put_contents("/tmp/Stopcopy","$vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | ||
| #copy($from_file,$to_file); | ||
| } else file_put_contents("/tmp/Stopcopy","Nocpy $vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | ||
| } |
There was a problem hiding this comment.
Fix file overwrite bug in loop.
Line 141 overwrites /tmp/Stopcopy with an empty string on every iteration of the loop. This erases all previous entries, leaving only the last VM's data in the file. Move the initialization outside the loop.
🐛 Proposed fix
+file_put_contents("/tmp/Stopcopy", "");
+
foreach ($vms as $vm => $vmdetail) {
- file_put_contents("/tmp/Stopcopy","");
$from_file = "/etc/libvirt/qemu/$vm.xml";
$to_file = $vmdetail['path']."/$vm.xml";
#echo " from:$from_file to:$to_file";
if ($vmdetail['exists']) {
- file_put_contents("/tmp/Stopcopy","$vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file";
+ file_put_contents("/tmp/Stopcopy", "$vm from:$from_file to:$to_file\n", FILE_APPEND);
#copy($from_file,$to_file);
- } 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);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ($vms as $vm => $vmdetail) { | |
| file_put_contents("/tmp/Stopcopy",""); | |
| $from_file = "/etc/libvirt/qemu/$vm.xml"; | |
| $to_file = $vmdetail['path']."/$vm.xml"; | |
| #echo " from:$from_file to:$to_file"; | |
| if ($vmdetail['exists']) { | |
| file_put_contents("/tmp/Stopcopy","$vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | |
| #copy($from_file,$to_file); | |
| } else file_put_contents("/tmp/Stopcopy","Nocpy $vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | |
| } | |
| file_put_contents("/tmp/Stopcopy", ""); | |
| foreach ($vms as $vm => $vmdetail) { | |
| $from_file = "/etc/libvirt/qemu/$vm.xml"; | |
| $to_file = $vmdetail['path']."/$vm.xml"; | |
| #echo " from:$from_file to:$to_file"; | |
| if ($vmdetail['exists']) { | |
| file_put_contents("/tmp/Stopcopy", "$vm from:$from_file to:$to_file\n", FILE_APPEND); | |
| #copy($from_file,$to_file); | |
| } else { | |
| file_put_contents("/tmp/Stopcopy", "Nocpy $vm from:$from_file to:$to_file\n", FILE_APPEND); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy around lines 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.
| $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.
| foreach ($vms as $vm => $vmdetail) { | ||
| file_put_contents("/tmp/Stopcopy",""); | ||
| $to_file = "/etc/libvirt/qemu/$vm.xml"; | ||
| $from_file = $vmdetail['path']."/$vm.xml"; | ||
| #echo " from:$from_file to:$to_file"; | ||
| if (file_exists($from_file)) { | ||
| file_put_contents("/tmp/libvirtrestore","$vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | ||
| #copy($from_file,$to_file); | ||
| } else file_put_contents("/tmp/libvirtrestore","Nocpy $vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file"; | ||
| } |
There was a problem hiding this comment.
Fix file overwrite bug in loop.
Line 19 overwrites /tmp/Stopcopy with an empty string on every iteration, erasing all previous entries. This is the same bug present in libvirtcopy. Additionally, the script writes to both /tmp/Stopcopy and /tmp/libvirtrestore – clarify if both are needed or if one is a copy-paste error.
🐛 Proposed fix
+file_put_contents("/tmp/libvirtrestore", "");
+
foreach ($vms as $vm => $vmdetail) {
- file_put_contents("/tmp/Stopcopy","");
$to_file = "/etc/libvirt/qemu/$vm.xml";
$from_file = $vmdetail['path']."/$vm.xml";
#echo " from:$from_file to:$to_file";
if (file_exists($from_file)) {
- file_put_contents("/tmp/libvirtrestore","$vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file";
+ file_put_contents("/tmp/libvirtrestore", "$vm from:$from_file to:$to_file\n", FILE_APPEND);
#copy($from_file,$to_file);
- } else file_put_contents("/tmp/libvirtrestore","Nocpy $vm from:$from_file to:$to_file\n",FILE_APPEND); #echo " from:$from_file to:$to_file";
+ } else {
+ file_put_contents("/tmp/libvirtrestore", "Nocpy $vm from:$from_file to:$to_file\n", FILE_APPEND);
+ }
}Note: Remove references to /tmp/Stopcopy if they were unintentional.
🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore around lines 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.
| $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.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.