diff --git a/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx b/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx index 6e91c8a7b..8214c3aac 100644 --- a/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx +++ b/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx @@ -147,22 +147,18 @@ export class CreateSnapshotModal extends React.Component { onCreate() { const Dialogs = this.context; - const { vm, isExternal, storagePools } = this.props; + const { vm, isExternal } = this.props; const { name, description, memoryPath } = this.state; - const disks = Object.values(vm.disks); const validationError = this.onValidate(); if (!Object.keys(validationError).length) { this.setState({ inProgress: true }); snapshotCreate({ - connectionName: vm.connectionName, - vmId: vm.id, + vm, name, description, - memoryPath: vm.state === "running" && memoryPath, - disks, isExternal, - storagePools + memoryPath: isExternal && vm.state === "running" && memoryPath, }) .then(() => { // VM Snapshots do not trigger any events so we have to refresh them manually diff --git a/src/libvirt-xml-create.js b/src/libvirt-xml-create.js index 417c7d3cc..5d59093e3 100644 --- a/src/libvirt-xml-create.js +++ b/src/libvirt-xml-create.js @@ -236,7 +236,7 @@ export function getPoolXML({ name, type, source, target }) { } // see https://libvirt.org/formatsnapshot.html -export function getSnapshotXML(name, description, disks, memoryPath, isExternal, storagePools, connectionName) { +export function getSnapshotXML(name, description, memoryPath) { const doc = document.implementation.createDocument('', '', null); const snapElem = doc.createElement('domainsnapshot'); @@ -253,36 +253,11 @@ export function getSnapshotXML(name, description, disks, memoryPath, isExternal, snapElem.appendChild(descriptionElem); } - if (isExternal) { - if (memoryPath) { - const memoryElem = doc.createElement('memory'); - memoryElem.setAttribute('snapshot', 'external'); - memoryElem.setAttribute('file', memoryPath); - snapElem.appendChild(memoryElem); - } - - const disksElem = doc.createElement('disks'); - disks.forEach(disk => { - // Disk can have attribute "snapshot" set to "no", which - // means no snapshot should be created of the said disk - // This cannot be configured through cockpit, but we - // should uphold it nevertheless. - // - // See "snapshot" attribute of element at - // https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms - if (disk.snapshot == "no") - return; - - // Skip disks without source, such as empty media drives. - if (isExternal && disk.type == "file" && !disk.source.file) - return; - - const diskElem = doc.createElement('disk'); - diskElem.setAttribute('name', disk.target); - diskElem.setAttribute('snapshot', 'external'); - disksElem.appendChild(diskElem); - }); - snapElem.appendChild(disksElem); + if (memoryPath) { + const memoryElem = doc.createElement('memory'); + memoryElem.setAttribute('snapshot', 'external'); + memoryElem.setAttribute('file', memoryPath); + snapElem.appendChild(memoryElem); } doc.appendChild(snapElem); diff --git a/src/libvirtApi/snapshot.js b/src/libvirtApi/snapshot.js index 671ec607d..e19222b13 100644 --- a/src/libvirtApi/snapshot.js +++ b/src/libvirtApi/snapshot.js @@ -29,11 +29,37 @@ import { parseDomainSnapshotDumpxml } from '../libvirt-xml-parse.js'; import { call, Enum, timeout } from './helpers.js'; import { logDebug } from '../helpers.js'; -export function snapshotCreate({ connectionName, vmId, name, description, memoryPath, disks, isExternal, storagePools }) { - // that flag ought to be implicit for non-running VMs, see https://issues.redhat.com/browse/RHEL-22797 +export async function snapshotCreate({ vm, name, description, isExternal, memoryPath }) { + // The "disk only" flag ought to be implicit for non-running VMs, see https://issues.redhat.com/browse/RHEL-22797 + + // However, "disk only" can be used to request external snapshots + // for a stopped machine. The alternative is to list all disks + // with a snapshot type of "external" in the XML, but then we need + // to worry about which disks to include and which to skip. + + // The behavior is as follows: + // + // VM state | disk only | memory path => resulting snapshot type + // -------------------------------------------------------------------- + // shutoff | false | null => internal + // shutoff | true | null => external + // running | false | null => internal full system + // running | false | non-null => external full system + // running | true | null => external disk only + // + // Other cases are errors. + const flags = (!isExternal || memoryPath) ? 0 : Enum.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; - const xmlDesc = getSnapshotXML(name, description, disks, memoryPath, isExternal, storagePools, connectionName); - return call(connectionName, vmId, 'org.libvirt.Domain', 'SnapshotCreateXML', [xmlDesc, flags], { timeout, type: 'su' }); + const xmlDesc = getSnapshotXML(name, description, memoryPath); + + // We really don't want to make disk only snapshots of running + // machines by accident. + + if (vm.state === "running" && (flags & Enum.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)) + throw new Error("Cowardly refusing to make a disk-only snapshot of a running machine"); + + return await call(vm.connectionName, vm.id, 'org.libvirt.Domain', 'SnapshotCreateXML', [xmlDesc, flags], + { timeout, type: 'su' }); } export function snapshotCurrent({ connectionName, objPath }) {