Copilot commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3947534807
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -53,6 +57,40 @@ public Answer execute(DeleteDiskOnlyVmSnapshotCommand
command, LibvirtComputingR
return new Answer(command, e);
}
}
+
+ deleteNvramSnapshotIfNeeded(command, resource, storagePoolMgr,
snapshotsToDelete);
return new Answer(command, true, null);
}
+
+ protected void deleteNvramSnapshotIfNeeded(DeleteDiskOnlyVmSnapshotCommand
command, LibvirtComputingResource resource, KVMStoragePoolManager
storagePoolMgr,
+ List<DataTO> snapshotsToDelete) {
+ if (StringUtils.isBlank(command.getNvramSnapshotPath())) {
+ return;
+ }
+
+ try {
+ KVMStoragePool storagePool;
+ if (command.getPrimaryDataStore() != null) {
+ PrimaryDataStoreTO dataStore = command.getPrimaryDataStore();
+ storagePool =
storagePoolMgr.getStoragePool(dataStore.getPoolType(), dataStore.getUuid());
+ } else {
+ SnapshotObjectTO rootVolumeSnapshot =
snapshotsToDelete.stream()
+ .map(SnapshotObjectTO.class::cast)
+ .filter(snapshotObjectTO ->
Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType()))
+ .findFirst()
+ .orElse(null);
+
+ if (rootVolumeSnapshot == null) {
+ logger.warn("Unable to locate the root volume snapshot
while deleting NVRAM snapshot [{}].", command.getNvramSnapshotPath());
+ return;
+ }
+
+ storagePool =
resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot,
storagePoolMgr);
+ }
+
+
Files.deleteIfExists(Path.of(storagePool.getLocalPathFor(command.getNvramSnapshotPath())));
Review Comment:
NVRAM sidecar deletion uses
storagePool.getLocalPathFor(command.getNvramSnapshotPath()) without validating
that the provided path is a safe relative path. Because getLocalPathFor() is a
simple string concatenation, a crafted value containing ".." or an absolute
path could lead to deleting files outside the pool mount.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -108,4 +116,88 @@ public Answer execute(RevertDiskOnlyVmSnapshotCommand cmd,
LibvirtComputingResou
return new RevertDiskOnlyVmSnapshotAnswer(cmd, volumeObjectTos);
}
+
+ protected SnapshotObjectTO getRootVolumeSnapshot(List<SnapshotObjectTO>
snapshotObjectTos) {
+ return snapshotObjectTos.stream()
+ .filter(snapshotObjectTO ->
Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType()))
+ .findFirst()
+ .orElse(null);
+ }
+
+ protected void validateNvramRevertState(RevertDiskOnlyVmSnapshotCommand
cmd, LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot,
+ KVMStoragePoolManager storagePoolMgr) throws IOException,
LibvirtException {
+ String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid());
+ if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) {
+ if (cmd.isUefiEnabled()) {
+ throw new IOException(String.format("Cannot safely revert
disk-only VM snapshot for UEFI VM [%s] because the snapshot does not contain
NVRAM state.",
+ cmd.getVmName()));
+ }
+ return;
+ }
+
+ if (StringUtils.isBlank(activeNvramPath)) {
+ throw new IOException(String.format("Unable to determine the
active UEFI NVRAM path for VM [%s].", cmd.getVmName()));
+ }
+
+ Path snapshotNvramPath =
getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot,
resource, storagePoolMgr);
+ if (!Files.exists(snapshotNvramPath)) {
+ throw new IOException(String.format("Unable to find the UEFI NVRAM
snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName()));
+ }
+ }
+
+ protected void restoreNvramIfNeeded(RevertDiskOnlyVmSnapshotCommand cmd,
LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot,
+ KVMStoragePoolManager storagePoolMgr) throws IOException,
LibvirtException {
+ if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) {
+ return;
+ }
+
+ String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid());
+ if (StringUtils.isBlank(activeNvramPath)) {
+ throw new IOException(String.format("Unable to determine the
active UEFI NVRAM path for VM [%s].", cmd.getVmName()));
+ }
+
+ Path snapshotNvramPath =
getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot,
resource, storagePoolMgr);
+ if (!Files.exists(snapshotNvramPath)) {
+ throw new IOException(String.format("Unable to find the UEFI NVRAM
snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName()));
+ }
+
+ replaceNvramAtomically(snapshotNvramPath, Path.of(activeNvramPath));
+ }
+
+ protected void replaceNvramAtomically(Path snapshotNvramPath, Path
activeNvramPath) throws IOException {
+ Path targetDirectory = activeNvramPath.getParent();
+ if (targetDirectory != null) {
+ Files.createDirectories(targetDirectory);
+ }
+
+ Path temporaryNvramPath = Files.createTempFile(targetDirectory,
activeNvramPath.getFileName().toString(), ".tmp");
+ try {
+ copyNvramSnapshotToTemporaryPath(snapshotNvramPath,
temporaryNvramPath);
+ moveTemporaryNvramIntoPlace(temporaryNvramPath, activeNvramPath);
+ } finally {
+ Files.deleteIfExists(temporaryNvramPath);
+ }
+ }
+
+ protected void copyNvramSnapshotToTemporaryPath(Path snapshotNvramPath,
Path temporaryNvramPath) throws IOException {
+ Files.copy(snapshotNvramPath, temporaryNvramPath,
StandardCopyOption.REPLACE_EXISTING);
+ }
+
+ protected void moveTemporaryNvramIntoPlace(Path temporaryNvramPath, Path
activeNvramPath) throws IOException {
+ try {
+ Files.move(temporaryNvramPath, activeNvramPath,
StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+ } catch (AtomicMoveNotSupportedException e) {
+ Files.move(temporaryNvramPath, activeNvramPath,
StandardCopyOption.REPLACE_EXISTING);
+ }
+ }
+
+ protected Path getNvramSnapshotAbsolutePath(String nvramSnapshotPath,
SnapshotObjectTO rootVolumeSnapshot, LibvirtComputingResource resource,
+ KVMStoragePoolManager storagePoolMgr) throws IOException,
LibvirtException {
+ if (rootVolumeSnapshot == null) {
+ throw new IOException("Unable to locate the root volume snapshot
while handling the UEFI NVRAM state.");
+ }
+
+ KVMStoragePool storagePool =
resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot,
storagePoolMgr);
+ return Path.of(storagePool.getLocalPathFor(nvramSnapshotPath));
+ }
Review Comment:
getNvramSnapshotAbsolutePath() passes nvramSnapshotPath directly into
storagePool.getLocalPathFor(). Since getLocalPathFor() does not sanitize
traversal (it concatenates strings), an unexpected nvramSnapshotPath containing
".." or an absolute path could escape the pool mount and read/overwrite
arbitrary files when restoring NVRAM.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -88,44 +103,71 @@ protected Answer
takeDiskOnlyVmSnapshotOfRunningVm(CreateDiskOnlyVmSnapshotComma
dm = resource.getDomain(conn, vmName);
if (dm == null) {
- return new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
String.format("Creation of disk-only VM Snapshot failed as we could not find
the VM [%s].", vmName), null);
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
+ String.format("Creation of disk-only VM Snapshot
failed as we could not find the VM [%s].", vmName), null, null);
+ return answer;
}
VMSnapshotTO target = cmd.getTarget();
Pair<String, Map<String, Pair<Long, String>>>
snapshotXmlAndVolumeToNewPathMap =
createSnapshotXmlAndNewVolumePathMap(volumeObjectTOS, disks, target, resource);
+ if (shouldFreezeVmFilesystemsForSnapshot(cmd)) {
+ // The guest-agent freeze flushes guest filesystems; suspend
below prevents concurrent UEFI NVRAM writes.
+ freezeVmFilesystems(dm, vmName);
+ filesystemsFrozenByThisWrapper = true;
+ verifyVmFilesystemsFrozen(dm, vmName);
+ }
+ if (shouldSuspendVmForSnapshot(cmd)) {
+ suspendedByThisWrapper = suspendVmIfNeeded(dm);
+ }
+ nvramSnapshotPath = backupNvramIfNeeded(cmd, resource);
- dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(),
getFlagsToUseForRunningVmSnapshotCreation(target));
+ dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(),
getFlagsToUseForRunningVmSnapshotCreation(target,
filesystemsFrozenByThisWrapper));
- return new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null,
snapshotXmlAndVolumeToNewPathMap.second());
- } catch (LibvirtException e) {
+ postSnapshotCleanupIssue = recoverVmAfterSnapshot(dm, vmName,
suspendedByThisWrapper, filesystemsFrozenByThisWrapper,
postSnapshotCleanupIssue);
+ filesystemsFrozenByThisWrapper = false;
+ suspendedByThisWrapper = false;
+
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null,
snapshotXmlAndVolumeToNewPathMap.second(), nvramSnapshotPath);
+ } catch (LibvirtException | IOException e) {
String errorMsg = String.format("Creation of disk-only VM snapshot
for VM [%s] failed due to %s.", vmName, e.getMessage());
logger.error(errorMsg, e);
- if (e.getMessage().contains("QEMU guest agent is not connected")) {
+ cleanupNvramSnapshotIfNeeded(cmd, resource, nvramSnapshotPath);
+ if (StringUtils.contains(e.getMessage(), "QEMU guest agent is not
connected")) {
errorMsg = "QEMU guest agent is not connected. If the VM has
been recently started, it might connect soon. Otherwise the VM does not have
the" +
" guest agent installed; thus the QuiesceVM parameter
is not supported.";
- return new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
errorMsg, null);
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
errorMsg, null, null);
+ } else {
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
e.getMessage(), null, null);
Review Comment:
In the non-guest-agent error path, the failure Answer uses e.getMessage()
directly. This can be null and also drops the VM context that was already
assembled into errorMsg, making failures harder to diagnose from the management
server/UI.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -6334,6 +6336,15 @@ public String getSnapshotTemporaryPath(String diskPath,
String snapshotName) {
return String.join(File.separator, diskPathSplitted);
}
+ public String getUefiNvramPath(String vmUuid) {
+ String nvramDirectory =
uefiProperties.getProperty(LibvirtVMDef.GuestDef.GUEST_NVRAM_PATH);
+ if (StringUtils.isBlank(nvramDirectory) ||
StringUtils.isBlank(vmUuid)) {
+ return null;
+ }
+
+ return nvramDirectory + vmUuid + ".fd";
Review Comment:
getUefiNvramPath() builds the NVRAM file path via string concatenation
(nvramDirectory + vmUuid + ".fd"), which silently breaks if the configured
directory does not end with a path separator. Using Path joining avoids relying
on trailing slashes and keeps path handling consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
