Updates for libvirt image removal and use directory #1722
Conversation
WalkthroughAdds OLD_IMAGE_FILE UI field and domain.cfg-driven image sync/migration; introduces libvirt path constants and helpers, per‑VM vms.json discovery, pre‑stop XML capture/restore tools, fs helpers, NVRAM migration tooling, rc.libvirt migration hook, and UI/helptext/gating updates. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant UI as VMSettings.page
participant CFG as /boot/config/domain.cfg
participant Init as libvirt_init
participant FS as Storage
participant Lib as libvirtd
participant RC as rc.libvirt
participant Copy as libvirtcopy
participant VMS as /boot/config/plugins/dynamix.vm.manager/vms.json
participant FSHELP as fs_helpers.php
participant Restore as libvirtrestore
Admin->>UI: set IMAGE_FILE and OLD_IMAGE_FILE
UI->>CFG: persist IMAGE_FILE / OLD_IMAGE_FILE
Init->>CFG: read IMAGE_FILE, OLD_IMAGE_FILE
alt both set and differ
Init->>FS: mount/backup/rsync between OLD and IMAGE (image ↔ dir)
Init->>CFG: update OLD_IMAGE_FILE
else skip sync
Init->>CFG: log skipped
end
RC->>Copy: on stop invoke libvirtcopy
Copy->>Lib: query domains -> build vms.json
Copy->>FS: copy planned XMLs -> per‑VM paths (uses FSHELP)
Init->>Restore: invoke libvirtrestore to restore XMLs into LIBVIRT_QEMU_DIR
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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🧪 Generate unit tests (beta)
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
emhttp/plugins/dynamix.vm.manager/javascript/vmmanager.js (1)
239-265:⚠️ Potential issue | 🟡 MinorClarify confirmation text to match disk‑removal scope.
Both dialogs currently say “Completely REMOVE … disk image and definition,” which doesn’t distinguish first‑disk‑only vs all‑disks and can mislead users. Consider making the confirm text explicit per option.
🩹 Suggested text tweak
- text:_("Completely REMOVE")+" "+name+" "+_("disk image and definition"), + text:_("Completely REMOVE")+" "+name+" "+_("definition and FIRST disk only"), ... - text:_("Completely REMOVE")+" "+name+" "+_("disk image and definition"), + text:_("Completely REMOVE")+" "+name+" "+_("definition and ALL disks"),emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
2966-3029:⚠️ Potential issue | 🟡 MinorFallback to rsync exists; however, defaulting to
alwaysfor non-ZFS filesystems is inefficient.Good news:
execCommand_nchan_clone()does properly fall back to rsync when the reflink command fails (see VMClone.php:34-47). Clones won't break on non-reflink filesystems.However, there are two issues:
Inefficiency for unknown filesystems: The function returns early with
reflink_mode='always'for all non-ZFS filesystems (ext4, btrfs, etc.), causingcp --reflink=alwaysto fail unnecessarily before falling back to rsync. Consider defaulting to'auto'or'never'for unknown filesystems to skip the failed attempt.Misleading comment: Line 2970's comment states
'always'is the "safest fallback if unsupported," but this is contradictory—'always'requires reflink support and will fail on unsupported systems. The actual safe default is'auto'(tries reflink, skips if unavailable) or'never'(no reflink attempt).
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/VMajax.php`:
- Around line 283-286: The domain-delete branch currently reads $firstdisk via
unscript(_var($_REQUEST,'firstdisk')) which treats the string "false" as truthy
and an absent parameter as '', flipping the intended default; update the parsing
so $firstdisk is a real boolean with default true: check if the request key
exists and if so normalize its value (accept "false", "0", 0, false as false;
everything else as true) before passing to $lv->domain_delete($domName,
$firstdisk), keep the requireLibvirt() and domain_delete call unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate`:
- Around line 60-89: The logic currently leaves the source NVRAM ($src_file)
undeleted when the destination ($dest_file) already exists and matches because
$copied remains false; update the compare branch so when files are identical
(the $same path) you set $copied = true (or otherwise mark as handled) so
downstream cleanup will remove the source after a successful redefine; apply the
same change to the other duplicate check around lines handling the second copy
(the block referenced 152-155) so both code paths treat identical destination
files as "copied" for deletion purposes.
- Around line 115-150: The migrate_nvram_file flow is always updating
$xml->os->nvram and running "virsh define" which can repoint the domain to a
snapshot VARS file; change migrate_nvram_file so it detects snapshot NVRAMs (use
the same logic/flag from validate_nvram_uuids or add an $is_snapshot param) and
for snapshot files only perform the file move/copy (handle $copied rollback) but
DO NOT modify $xml->os->nvram, write $xml_new_path, or run exec("virsh define
..."); keep the existing behavior for base NVRAMs (update $xml_formatted, write
$xml_new_path, and run virsh define) and preserve rollback on failures using
$dest_file and $copied.
🧹 Nitpick comments (3)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
2978-3041: Change default reflink mode from'always'to'auto'for unknown filesystems.For non-ZFS filesystems,
'always'causescp --reflink=alwaysto fail when reflinks aren't supported, even though a fallback mechanism exists. The'auto'mode gracefully attempts reflinks only when available, avoiding unnecessary failures.♻️ Proposed tweak
$result = [ 'filesystem' => 'unknown', - 'reflink_mode' => 'always' // safest fallback if unsupported + 'reflink_mode' => 'auto' // gracefully attempts reflinks when available ];emhttp/plugins/dynamix.vm.manager/include/libvirt.php (2)
284-286: Unused variable$vms_entry_json.The variable
$vms_entry_jsonis computed on line 286 but never used withinconfig_to_xml(). Either remove it or utilize it as intended.♻️ Proposed fix
$nvram_dir = libvirt_get_nvram_dir($vm_path, $name); - $vms_json = libvirt_get_vms_json(); - $vms_entry = $vms_json[$name] ?? null; - $vms_entry_json = $vms_entry !== null ? json_encode($vms_entry) : 'null';
1029-1045: Validate$vm_namebefore building VM entry.If
$config['domain']['name']is null or empty,libvirt_build_vm_entry($vm_name, ...)may produce invalid paths or entries. Consider adding validation.🛡️ Proposed fix
function build_vm_paths($config) { if (!file_exists('/boot/config/plugins/dynamix.vm.manager/vm_newmodel')) { return $config; } if (empty($config['domain']['uuid'])) { $config['domain']['uuid'] = $this->domain_generate_uuid(); } $vm_name = $config['domain']['name'] ?? null; + if (empty($vm_name)) { + return $config; + } $storage = $config['template']['storage'] ?? 'default';
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate`:
- Around line 15-18: Guard against missing or empty DOCUMENT_ROOT by replacing
the current $docroot initialization that uses $_SERVER['DOCUMENT_ROOT']
directly; ensure you first coalesce to an empty string then use a truthy
fallback so CLI runs don't emit notices—update the $docroot assignment (the line
setting $docroot and any use of $_SERVER['DOCUMENT_ROOT'] in this script) to use
a pattern like coalesce-to-empty then ?: '/usr/local/emhttp' so empty strings
are handled as well.
- Around line 1-14: Remove the premature closing PHP tag '?>' and the subsequent
short open tag '<?' so the file stays in a single '<?php' block; specifically
delete the closing '?>' at the end of the initial PHP block and remove the later
'<?' token, leaving only the original '<?php' opener and all PHP code under it
(do not add any additional tags and ensure no short_open_tag usage remains).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php`:
- Around line 1887-1892: domain_delete() may receive a domain resource, but
libvirt_get_vm_path() and libvirt_remove_vms_json_entry() expect a string name;
derive the string name first by calling get_domain_object($domain) (or otherwise
extracting the name) and store it in a new variable (e.g., $domain_name), then
pass $domain_name into libvirt_get_vm_path() and libvirt_remove_vms_json_entry()
and anywhere else that expects the domain name rather than the original $domain
resource; keep using the original $domain resource for libvirt API calls that
require it.
In `@emhttp/plugins/dynamix/include/Helpers.php`:
- Around line 520-521: The code currently treats symlinked directories as real
directories because it uses is_dir($path) and then calls
delete_dir_contents($path) and rmdir($path), which can delete targets outside
the intended tree; update the logic in Helpers.php to first detect and skip or
safely handle symlinks (use is_link($path) or lstat to detect symlinks) before
recursing: if is_link($path) either unlink the symlink only or skip it,
otherwise proceed with is_dir($path) -> delete_dir_contents($path) &&
rmdir($path) && $success; ensure delete_dir_contents() itself does not follow
symlinks when iterating.
- Line 516: scandir($dir) can return false on error, but the code assigns it to
$items and later iterates it with foreach which causes warnings and incorrect
success; update the logic around scandir($dir) to check for a strict === false
result before any foreach or processing of $items (e.g., in the same function in
Helpers.php where $items is assigned) and handle the failure path explicitly —
either return/throw/log an error or set $items to an empty array and skip
processing — so that a failed scandir does not lead to iterating a non-array and
falsely reporting success.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
3016-3021: Remove or gate the debugerror_log()call ingetFilesystemAndReflinkMode.
This can flood logs during clone operations.♻️ Minimal cleanup
- error_log("Getting real disks\n");
This is for files outside of the normal location.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
3016-3079:⚠️ Potential issue | 🟠 MajorChange default
reflink_modefromalwaystoautofor unknown filesystems.
cp --reflink=alwaysfails with non-zero exit on filesystems without reflink support, breaking clone operations.--reflink=autogracefully falls back to standard copy when reflink is unavailable—the safe default for unknown filesystems. The current comment claimingalwaysis "safest" is incorrect per GNU coreutils documentation.🔧 Suggested fix
$result = [ 'filesystem' => 'unknown', - 'reflink_mode' => 'always' // safest fallback if unsupported + 'reflink_mode' => 'auto' // safe fallback: attempt reflink, gracefully fall back to standard copy ];
This will backup the image, copy xml files to the VM directory. Migrate snapshot database and NVRAM files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init`:
- Around line 106-117: The backup step in the libvirt_init block uses TIMESTAMP
and assumes IMAGE_FILE is a .img file, which can be unset or a directory;
initialize TIMESTAMP locally (e.g., TIMESTAMP="$(date +%s)" or similar) before
building BACKUP_PATH, and guard the backup by checking that IMAGE_FILE is a
regular file and matches *.img (or otherwise skip/handle directories) before
running cp -p; keep the rest of the migration flow (libvirtcopy and
libvirtmigrate) unchanged and only create BACKUP_PATH when those checks pass to
avoid cp failures and empty timestamps.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php`:
- Around line 1711-1717: The code unlinks $xml_file when $save === false but
doesn't guard for an empty $vm_path, which can produce "/$domain.xml" and delete
root-level files; update the conditional in the block that checks ($save ===
false) to first validate $vm_path (e.g., if empty($vm_path) or not
is_string($vm_path) or it equals '/' then return false), only build $xml_file
and call is_file/unlink when $vm_path passes validation; reference the variables
$save, $vm_path, $domain and the $xml_file unlink branch to locate where to add
this guard.
- Around line 1887-1899: The code is deriving $domain_name incorrectly when
callers may pass $dom (UUID or alias) and then uses
libvirt_get_vm_path($domain_name) without guarding an empty $vm_path; update the
logic to derive the VM name from the libvirt domain handle if $dom is a resource
(use $this->domain_get_name($dom) or similar) and only call
libvirt_remove_vms_json_entry and inspect $vm_path subdirs after verifying
libvirt_get_vm_path returned a non-empty, valid path; specifically adjust the
sequence around domain_get_name / libvirt_get_vm_path /
libvirt_remove_vms_json_entry to compute $domain_name from $dom safely and add a
guard like if (!empty($vm_path) && is_dir($vm_path)) before touching
$vm_path.'/nvram' and $vm_path.'/snapshotdb'.
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init`:
- Around line 53-59: The IMG-file branch handling when [[ "$IMAGE_FILE" == *.img
]] lacks error checking: ensure each step (mount "$IMAGE_FILE" "$TMP_MNT", rsync
-a ..., and umount "$TMP_MNT") is validated and failures logged and cause early
exit/cleanup. After mkdir -p "$TMP_MNT", check the mount command's exit status
and log a descriptive error (including $IMAGE_FILE and $TMP_MNT) and abort if
mount fails; only run rsync when mount succeeded and check rsync's exit status
and log/abort on failure; finally check umount's exit status and retry or log
error before proceeding. Use the same style/messages and return behavior as the
other branch to keep behavior consistent with the existing mount/rsync/umount
handling (referencing IMAGE_FILE, TMP_MNT, OLD_IMAGE_FILE, IMG_FILE_NAME, mount,
rsync, umount).
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init (1)
124-126: Trailing comment marker on line 126.The standalone
#on line 126 appears to be a leftover artifact. Consider removing it for cleanliness.🧹 Minor cleanup
# Copy XML from VM Directories to QEMU directory/ /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtrestore -#emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
2777-2780: Resolve or remove the “Use new function?” placeholder.
It’s ambiguous in a release path—either wire it to the new flow or drop the comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
2014-2074:⚠️ Potential issue | 🟠 MajorAdd vm_name fallback to nvram_rename and snapshot helpers to avoid missing per-VM files.
The
nvram_rename,nvram_create_snapshot,nvram_revert_snapshot, andnvram_delete_snapshotfunctions lack the uuid-to-vm_name resolution thatnvram_backupandnvram_restorealready implement. When these helpers are called without an explicit$vm_nameparameter, they fall back to the global NVRAM directory, causing them to miss per-VM NVRAM files on new-model configurations.The
nvram_renamecall inCustom.form.php:283already omits$vm_name, making this a real bug during VM UUID changes.Apply to nvram_rename, nvram_create_snapshot, nvram_revert_snapshot, and nvram_delete_snapshot
function nvram_rename($uuid, $newuuid, $vm_name = null) { + if (empty($vm_name) && !empty($uuid)) { + $vm_name = $this->domain_get_name_by_uuid($uuid) ?: null; + } $vm_path = $vm_name ? libvirt_get_vm_path($vm_name) : null; $nvram_dir = libvirt_get_nvram_dir($vm_path, $vm_name);
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
1047-1084:⚠️ Potential issue | 🟠 MajorRollback vms.json entry on create/define failure.
build_vm_paths()writes vms.json before disk creation/define; if those steps fail, the entry can become stale and later path lookups will be wrong.🛠️ Suggested fix
function domain_new($config) { # Build paths for VM and store in the vms.json $config = $this->build_vm_paths($config); + $vm_name = $config['domain']['name'] ?? null; # Set storage for disks. foreach ($config['disk'] as $i => $disk) { $config['disk'][$i]['storage'] = $config['template']['storage'];} @@ if (!empty($disk['error'])) { $this->last_error = $disk['error']; + if ($vm_name) libvirt_remove_vms_json_entry($vm_name); return false; } @@ if ($config['domain']['persistent']) { #$tmp = libvirt_domain_define_xml($this->conn, $strXML); ## Update to use new function $tmp = $this->domain_define($strXML); - if (!$tmp) return $this->_set_last_error(); + if (!$tmp) { + if ($vm_name) libvirt_remove_vms_json_entry($vm_name); + return $this->_set_last_error(); + } $this->domain_set_autostart($tmp, $config['domain']['autostart'] == 1); return $tmp;
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
2787-2793: Consider removing or tracking the lingering “Use new function?” note.
If this is a TODO, capture it explicitly; otherwise, it can ship as dead commentary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init`:
- Around line 65-67: The sed replacement that updates OLD_IMAGE_FILE uses
unescaped $IMAGE_FILE which will break if the value contains &, |, or
backslashes; before running sed, create an escaped version of IMAGE_FILE (escape
backslashes, escape & as \&, and escape or avoid the sed delimiter like |) and
then use that escaped variable in the sed invocation that updates DOMAIN_CFG so
the substitution writes the literal filename safely.
| # Update OLD_IMAGE_FILE in domain.cfg | ||
| log "Updating OLD_IMAGE_FILE in $DOMAIN_CFG" | ||
| sed -i "s|^OLD_IMAGE_FILE=.*|OLD_IMAGE_FILE=\"$IMAGE_FILE\"|" "$DOMAIN_CFG" |
There was a problem hiding this comment.
Escape IMAGE_FILE before the sed replacement to avoid corrupting domain.cfg.
If IMAGE_FILE contains &, |, or backslashes, sed will interpret them and write a wrong value. Escape before substitution.
🛠️ Suggested fix
- sed -i "s|^OLD_IMAGE_FILE=.*|OLD_IMAGE_FILE=\"$IMAGE_FILE\"|" "$DOMAIN_CFG"
+ SAFE_IMAGE_FILE=$(printf '%s' "$IMAGE_FILE" | sed 's/[\\&|]/\\&/g')
+ sed -i "s|^OLD_IMAGE_FILE=.*|OLD_IMAGE_FILE=\"$SAFE_IMAGE_FILE\"|" "$DOMAIN_CFG"🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/scripts/libvirt_init` around lines 65 - 67,
The sed replacement that updates OLD_IMAGE_FILE uses unescaped $IMAGE_FILE which
will break if the value contains &, |, or backslashes; before running sed,
create an escaped version of IMAGE_FILE (escape backslashes, escape & as \&, and
escape or avoid the sed delimiter like |) and then use that escaped variable in
the sed invocation that updates DOMAIN_CFG so the substitution writes the
literal filename safely.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@etc/rc.d/rc.libvirt`:
- Around line 346-355: The libvirt_migrate function currently always marks
migration complete and restarts libvirt even if
/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy or
libvirtmigrate fail or are missing; update libvirt_migrate to verify both
scripts exist and are executable (test -x), run them capturing their exit
status, and only proceed to touch
/boot/config/plugins/dynamix.vm.manager/vm_newmodel and call libvirtd_stop /
libvirtd_start when both commands succeed; if either is missing or returns
non‑zero, log an error and abort the function (return non‑zero) so the sentinel
is not created and restart is not attempted.
- Around line 37-43: Replace hardcoded "/etc/libvirt/qemu" with the configurable
variable LIBVIRT_QEMU_DIR everywhere so rc.libvirt.conf overrides work: update
the XML scan loop to use for XML in $LIBVIRT_QEMU_DIR/*.xml; do, change the
swtpm/tpm-states mkdir call to mkdir -p $LIBVIRT_QEMU_DIR/swtpm/tpm-states,
change the snapshot mkdir to mkdir -p $LIBVIRT_QEMU_DIR/snapshot, and update the
symlink creation to ln -sf $LIBVIRT_QEMU_DIR/snapshot
/var/lib/libvirt/qemu/snapshot; ensure all references use the LIBVIRT_QEMU_DIR
variable and preserve existing quoting/word-splitting behavior.
| # libvirt path configuration | ||
| LIBVIRT_QEMU_DIR="/etc/libvirt/qemu" | ||
| LIBVIRT_SNAPSHOTDB_DIR="$LIBVIRT_QEMU_DIR/snapshotdb" | ||
| if [[ -r /etc/rc.d/rc.libvirt.conf ]]; then | ||
| # shellcheck disable=SC1091 | ||
| . /etc/rc.d/rc.libvirt.conf | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get an overview of the file size and check for all /etc/libvirt/qemu references
wc -l etc/rc.d/rc.libvirtRepository: unraid/webgui
Length of output: 80
🏁 Script executed:
# Search for all /etc/libvirt/qemu hardcoded references in the file
rg '/etc/libvirt/qemu' etc/rc.d/rc.libvirt -nRepository: unraid/webgui
Length of output: 304
🏁 Script executed:
# Also search for LIBVIRT_QEMU_DIR usage to see where the variable is actually being used
rg 'LIBVIRT_QEMU_DIR' etc/rc.d/rc.libvirt -nRepository: unraid/webgui
Length of output: 153
🏁 Script executed:
# Read the specific changed segments mentioned in the review
sed -n '37,43p' etc/rc.d/rc.libvirt
echo "---"
sed -n '244,246p' etc/rc.d/rc.libvirt
echo "---"
sed -n '258,260p' etc/rc.d/rc.libvirt
echo "---"
sed -n '263,270p' etc/rc.d/rc.libvirt
echo "---"
sed -n '346,355p' etc/rc.d/rc.libvirtRepository: unraid/webgui
Length of output: 1225
🏁 Script executed:
# Check if rc.libvirt.conf exists and what variables it may define
if [[ -f etc/rc.d/rc.libvirt.conf ]]; then
cat etc/rc.d/rc.libvirt.conf
else
echo "rc.libvirt.conf not found in etc/rc.d/"
fiRepository: unraid/webgui
Length of output: 216
🏁 Script executed:
# Let's examine line 197 context to understand the XML scan usage
sed -n '190,205p' etc/rc.d/rc.libvirtRepository: unraid/webgui
Length of output: 807
Use $LIBVIRT_QEMU_DIR consistently instead of hardcoding /etc/libvirt/qemu.
rc.libvirt.conf allows overriding LIBVIRT_QEMU_DIR, but the variable is only used once (line 39). The hardcoded path /etc/libvirt/qemu appears at lines 197 (XML scan), 241, 243, and 248 (tpm-states, snapshot directories, symlink), making the override mechanism non-functional. Replace all hardcoded instances with $LIBVIRT_QEMU_DIR:
- Line 197:
for XML in $LIBVIRT_QEMU_DIR/*.xml; do - Line 241:
mkdir -p $LIBVIRT_QEMU_DIR/swtpm/tpm-states - Line 243:
mkdir -p $LIBVIRT_QEMU_DIR/snapshot - Line 248:
ln -sf $LIBVIRT_QEMU_DIR/snapshot /var/lib/libvirt/qemu/snapshot
🤖 Prompt for AI Agents
In `@etc/rc.d/rc.libvirt` around lines 37 - 43, Replace hardcoded
"/etc/libvirt/qemu" with the configurable variable LIBVIRT_QEMU_DIR everywhere
so rc.libvirt.conf overrides work: update the XML scan loop to use for XML in
$LIBVIRT_QEMU_DIR/*.xml; do, change the swtpm/tpm-states mkdir call to mkdir -p
$LIBVIRT_QEMU_DIR/swtpm/tpm-states, change the snapshot mkdir to mkdir -p
$LIBVIRT_QEMU_DIR/snapshot, and update the symlink creation to ln -sf
$LIBVIRT_QEMU_DIR/snapshot /var/lib/libvirt/qemu/snapshot; ensure all references
use the LIBVIRT_QEMU_DIR variable and preserve existing quoting/word-splitting
behavior.
| libvirt_migrate(){ | ||
| log "Copying VM data to VM directories" | ||
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy | ||
| log "Migrating VM configs to new model" | ||
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate --migrate --confirm > /var/log/libvirt-migrate 2>&1 | ||
| touch /boot/config/plugins/dynamix.vm.manager/vm_newmodel | ||
| rmdir /run/libvirt/qemu | ||
| libvirtd_stop | ||
| libvirtd_start | ||
| } |
There was a problem hiding this comment.
Don’t mark migration complete or restart if scripts fail.
libvirt_migrate proceeds to touch the sentinel and restarts even if libvirtcopy/libvirtmigrate fails or is missing. That can leave the system in a partially migrated state and prevent re‑attempts.
🛠️ Minimal failure guards before marking migration complete
libvirt_migrate(){
log "Copying VM data to VM directories"
- /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy
+ local LIBVIRTCOPY="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy"
+ local LIBVIRTMIGRATE="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate"
+ if [[ ! -x "$LIBVIRTCOPY" ]]; then
+ log "Error: libvirtcopy not found or not executable"
+ return 1
+ fi
+ if ! "$LIBVIRTCOPY"; then
+ log "Error: libvirtcopy failed"
+ return 1
+ fi
log "Migrating VM configs to new model"
- /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate --migrate --confirm > /var/log/libvirt-migrate 2>&1
- touch /boot/config/plugins/dynamix.vm.manager/vm_newmodel
+ if [[ ! -x "$LIBVIRTMIGRATE" ]]; then
+ log "Error: libvirtmigrate not found or not executable"
+ return 1
+ fi
+ if ! "$LIBVIRTMIGRATE" --migrate --confirm > /var/log/libvirt-migrate 2>&1; then
+ log "Error: libvirtmigrate failed; see /var/log/libvirt-migrate"
+ return 1
+ fi
+ touch /boot/config/plugins/dynamix.vm.manager/vm_newmodel
rmdir /run/libvirt/qemu
libvirtd_stop
libvirtd_start
}📝 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.
| libvirt_migrate(){ | |
| log "Copying VM data to VM directories" | |
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy | |
| log "Migrating VM configs to new model" | |
| /usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate --migrate --confirm > /var/log/libvirt-migrate 2>&1 | |
| touch /boot/config/plugins/dynamix.vm.manager/vm_newmodel | |
| rmdir /run/libvirt/qemu | |
| libvirtd_stop | |
| libvirtd_start | |
| } | |
| libvirt_migrate(){ | |
| log "Copying VM data to VM directories" | |
| local LIBVIRTCOPY="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy" | |
| local LIBVIRTMIGRATE="/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtmigrate" | |
| if [[ ! -x "$LIBVIRTCOPY" ]]; then | |
| log "Error: libvirtcopy not found or not executable" | |
| return 1 | |
| fi | |
| if ! "$LIBVIRTCOPY"; then | |
| log "Error: libvirtcopy failed" | |
| return 1 | |
| fi | |
| log "Migrating VM configs to new model" | |
| if [[ ! -x "$LIBVIRTMIGRATE" ]]; then | |
| log "Error: libvirtmigrate not found or not executable" | |
| return 1 | |
| fi | |
| if ! "$LIBVIRTMIGRATE" --migrate --confirm > /var/log/libvirt-migrate 2>&1; then | |
| log "Error: libvirtmigrate failed; see /var/log/libvirt-migrate" | |
| return 1 | |
| fi | |
| touch /boot/config/plugins/dynamix.vm.manager/vm_newmodel | |
| rmdir /run/libvirt/qemu | |
| libvirtd_stop | |
| libvirtd_start | |
| } |
🤖 Prompt for AI Agents
In `@etc/rc.d/rc.libvirt` around lines 346 - 355, The libvirt_migrate function
currently always marks migration complete and restarts libvirt even if
/usr/local/emhttp/plugins/dynamix.vm.manager/scripts/libvirtcopy or
libvirtmigrate fail or are missing; update libvirt_migrate to verify both
scripts exist and are executable (test -x), run them capturing their exit
status, and only proceed to touch
/boot/config/plugins/dynamix.vm.manager/vm_newmodel and call libvirtd_stop /
libvirtd_start when both commands succeed; if either is missing or returns
non‑zero, log an error and abort the function (return non‑zero) so the sentinel
is not created and restart is not attempted.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
Chores