snapshots: Don't list disks in XML

We can control whether or not an external snapshot is made with the
"disk only" flag and the memory path, and libvirt will provide
defaults for all the disks.

Cherry-picked from main commit ebcf07b

Author: mvollmer

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

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 <disk> 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);

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 }) {