On 9/1/2026 4:36 PM, Matthew Rosato wrote:
On 8/26/26 10:57 AM, Joshua Daley wrote:Split test_s390x_secure_ipl() into two subtests. Each tests with a different boot device: virtio-blk-ccw or virtio-blk-pci. Use class-level variables and a temporary shared workdir such that the time-consuming setup is not run multiple times.Signed-off-by: Joshua Daley <[email protected]>Thanks, I verified that the setup is not run twice (and indeed, you can tell that while the first test takes quite a while, the second test finishes much faster) One other thought below... [...]@@ -109,39 +125,42 @@ def setup_s390x_secure_ipl(self): temp_vm.set_machine('s390-ccw-virtio')asset_path = self.ASSET_F40_QCOW2.fetch()- self.qcow2_path = self.scratch_file('f40.qcow2') + qcow2_path = os.path.join(S390xSecureIpl._shared_workdir, 'f40.qcow2') qemu_img = get_qemu_img(self) check_call([qemu_img, 'create', '-f', 'qcow2', '-b', asset_path, - '-F', 'qcow2', self.qcow2_path], stdout=DEVNULL, stderr=DEVNULL) + '-F', 'qcow2', qcow2_path], stdout=DEVNULL, stderr=DEVNULL) + S390xSecureIpl._qcow2_path = qcow2_pathtemp_vm.set_console()temp_vm.add_args('-nographic', '-accel', 'kvm', '-m', '1024', '-drive', - f'id=drive0,if=none,format=qcow2,file={self.qcow2_path}', + f'id=drive0,if=none,format=qcow2,file={qcow2_path}', '-device', 'virtio-blk-ccw,drive=drive0,bootindex=1') temp_vm.launch()self._require_host_secure_ipl_support(temp_vm)If we find this fails the first time, then we are going to fail setup and therefore the 2nd test will also enter here, create the qcow2 again, and then fail again for the same reason. Couldn't we also save the time spent creating the qcow2 and launching a VM the second time if we know it didn't work the first time? It's checking the same host facilities. Basically I'm wondering if we should just create another flag in S390xSecureIpl that defaults to false and gets set to true in _require_host_secure_ipl_support() if the support is missing. And check that flag in each test before checking _setup_done and skip right away if it's true (with the same message as the one in _require_host_secure_ipl_support().
I think that's a good idea. I'll add it. Thanks for reviewing.
