Updates for libvirt image removal and use diretory #1722
Conversation
WalkthroughAdds tracking and sync between current and previous libvirt IMAGE_FILE paths. A hidden OLD_IMAGE_FILE field was added to the VM settings UI, libvirt_init now synchronizes domain data between old/new paths (handling Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as VMSettings.page
participant Config as libvirtconfig
participant Init as libvirt_init
User->>UI: Open/modify VM Settings
UI->>Config: Load/Save IMAGE_FILE and OLD_IMAGE_FILE
Note over Init,Config: On libvirt_init run
Init->>Config: Read IMAGE_FILE and OLD_IMAGE_FILE
alt both set and differ
Init->>Init: Mount/.img or access dirs
Init->>Init: Rsync data old → new (respecting excludes)
Init->>Config: Update OLD_IMAGE_FILE to current IMAGE_FILE
else
Init->>Init: Log and skip syncing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
| :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 in help text.
The phrase "at change" is awkward. Consider one of these alternatives:
-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 changed.or
-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 changed. | |
| :end |
🤖 Prompt for AI Agents
In emhttp/languages/en_US/helptext.txt around lines 1717 to 1719, the help text
"This is a location for storing previous versions of xml and nvram at change."
is grammatically awkward; replace it with a clearer phrase such as "This is a
location for storing previous versions of XML and NVRAM when changes are made."
Ensure XML and NVRAM are capitalized and the new sentence reads fluently.
| :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 in help text.
Same grammar issue as line 1718. The phrase "at change" should be improved:
-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 changed. Does not need to be specified.or
-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. 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 changed. Does not need to be specified. | |
| :end |
| :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 on change. Does not need to be specified. | |
| :end |
🤖 Prompt for AI Agents
In emhttp/languages/en_US/helptext.txt around lines 1730 to 1732, the help text
uses the awkward phrase "at change"; update the sentence for correct grammar and
clarity by replacing "This is a directory for storing previous versions of xml
and nvram at change. Does not need to be specified." with a single clear
sentence such as "This directory stores previous versions of XML and NVRAM when
they change and does not need to be specified." or alternatively "This directory
stores previous versions of XML and NVRAM on change; it does not need to be
specified."
| <?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.
2 participants
Summary by CodeRabbit
New Features
Enhancements
Documentation