From 4deef0d22cc21494d1cfc4a2df59c1582e67d64d Mon Sep 17 00:00:00 2001
From: Martin Pitt <mpitt@redhat.com>
Date: Fri, 24 Nov 2023 14:05:42 +0100
Subject: [PATCH] Disk add dialog: Change format together with the storage pool

Otherwise we create a race condition: The ModalBody already gets
re-rendered with the new pool name, but with possibly with a format that
is invalid for the new pool type. That triggers our new assertion.

This is very redundant with the effect which fine-tunes the
format and disk type according to the other data. This effect is evil
and should be dropped, but doing so requires more intrusive code
cleanup. Introduce a helper function to avoid repeated code.
---
 src/components/vm/disks/diskAdd.jsx | 35 ++++++++++++++++++-----------
 test/check-machines-disks           |  6 -----
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/components/vm/disks/diskAdd.jsx b/src/components/vm/disks/diskAdd.jsx
index 85e647df1..4b450f1f1 100644
--- a/src/components/vm/disks/diskAdd.jsx
+++ b/src/components/vm/disks/diskAdd.jsx
@@ -395,6 +395,19 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
     }), [defaultPool, mode, vm]);
     const storagePoolName = diskParams.storagePool?.name;
 
+    const getPoolFormatAndDevice = (pool, volName) => {
+        const params = { format: getDefaultVolumeFormat(pool), device: "disk" };
+        if (['dir', 'fs', 'netfs', 'gluster', 'vstorage'].indexOf(pool.type) > -1) {
+            const volume = pool.volumes.find(vol => vol.name === volName);
+            if (volume?.format) {
+                params.format = volume.format;
+                if (volume.format === "iso")
+                    params.device = "cdrom";
+            }
+        }
+        return params;
+    };
+
     useEffect(() => {
         // Refresh storage volume list before displaying the dialog.
         // There are recently no Libvirt events for storage volumes and polling is ugly.
@@ -423,20 +436,9 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
             return;
         }
 
-        let format = getDefaultVolumeFormat(diskParams.storagePool);
-        let deviceType = "disk";
-        if (['dir', 'fs', 'netfs', 'gluster', 'vstorage'].indexOf(diskParams.storagePool.type) > -1) {
-            const volume = diskParams.storagePool.volumes.find(vol => vol.name === diskParams.existingVolumeName);
-            if (volume && volume.format) {
-                format = volume.format;
-                if (volume.format === "iso")
-                    deviceType = "cdrom";
-            }
-        }
         setDiskParams(diskParams => ({
             ...diskParams,
-            format,
-            device: deviceType,
+            ...getPoolFormatAndDevice(diskParams.storagePool, diskParams.existingVolumeName),
         }));
     }, [storagePools, diskParams.storagePool, diskParams.existingVolumeName]);
 
@@ -513,7 +515,14 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
         switch (key) {
         case 'storagePoolName': {
             const currentPool = storagePools.find(pool => pool.name === value && pool.connectionName === vm.connectionName);
-            setDiskParams(diskParams => ({ ...diskParams, storagePool: currentPool, existingVolumeName: getDefaultVolumeName(currentPool, vm) }));
+            const existingVolumeName = getDefaultVolumeName(currentPool, vm);
+
+            setDiskParams(diskParams => ({
+                ...diskParams,
+                storagePool: currentPool,
+                existingVolumeName,
+                ...getPoolFormatAndDevice(currentPool, existingVolumeName),
+            }));
             break;
         }
         case 'mode': {
diff --git a/test/check-machines-disks b/test/check-machines-disks
index 79318ddd9..7b4e6609f 100755
--- a/test/check-machines-disks
+++ b/test/check-machines-disks
@@ -953,12 +953,6 @@ class TestMachinesDisks(VirtualMachinesCase):
         if m.image in ["debian-testing"]:
             self.allow_journal_messages(f'.* type=1400 .* apparmor="DENIED" operation="open" profile="libvirt.* name="{self.vm_tmpdir}.*')
 
-        # HACK: Switching the pool relies on useEffect() to update "Format:" to the default format of the new pool
-        # type; there is an intermediade rendering with the new pool and the old format. This is unfortunately
-        # hard to fix due to the useEffect() maze in the "Add disk" dialog. Until we clean this up, ignore this
-        # particular instance
-        self.allow_browser_errors("VolumeDetails internal error: format qcow2 is not valid for storage pool type disk")
-
     @timeout(900)
     def testAddDiskDirPool(self):
         b = self.browser