Conversation
It is not allowed from the frontend but it spits and error just in case
There was a problem hiding this comment.
Pull request overview
This PR refactors the KVM ISO/CDROM UEFI testing and updates KVM UEFI XML generation to support 32-bit (i686) UEFI firmware selection.
Changes:
- Move CDROM add/remove/base lifecycle ISO coverage from
t/vm/d20_disks.tinto a dedicatedt/kvm/uefi_iso.ttest. - Update UEFI loader filename expectations in
t/kvm/uefi.t. - Extend KVM UEFI XML generation to handle i686 and adjust related test fixtures.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| t/vm/d20_disks.t | Removes the embedded test_cdrom test and its invocation (test moved elsewhere). |
| t/request/31_hw_boot_order.t | Updates the test XML “machine” attribute dynamically via _find_machine_type. |
| t/kvm/uefi_iso.t | Adds a new dedicated KVM UEFI ISO/CDROM regression test covering CDROM add/remove and base ops. |
| t/kvm/uefi.t | Relaxes the OVMF loader filename match for q35+UEFI test cases. |
| t/kvm/pcie.t | Adjusts the sample libvirt XML seclabel model in the fixture. |
| lib/Ravada/VM/KVM.pm | Updates _xml_add_uefi to introduce i686 handling and change x86_64 behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| my $machine_type = $domain->_vm->_find_machine_type('i686',qr'pc-i440fx'); | ||
| my ($os_type) = $doc->findnodes('/domain/os/type'); | ||
| $os_type->setAttribute('machine' => $machine_type); |
There was a problem hiding this comment.
_fix_domain_config() sets the domain machine attribute using an i686 machine type, but the embedded XML being reloaded is explicitly arch='x86_64'. This arch/machine mismatch can make libvirt reject the domain or behave unexpectedly when the test later starts the VM. Use the arch from the XML (or from the existing domain XML) when calling _find_machine_type(), or update the arch attribute consistently if the intent is to force i686.
| my $machine_type = $domain->_vm->_find_machine_type('i686',qr'pc-i440fx'); | |
| my ($os_type) = $doc->findnodes('/domain/os/type'); | |
| $os_type->setAttribute('machine' => $machine_type); | |
| my ($os_type) = $doc->findnodes('/domain/os/type'); | |
| if ($os_type) { | |
| my $arch = $os_type->getAttribute('arch') // 'i686'; | |
| my $machine_type = $domain->_vm->_find_machine_type($arch, qr'pc-i440fx'); | |
| $os_type->setAttribute('machine' => $machine_type); | |
| } |
| } elsif ($arch eq 'i686') { | ||
| if (!$self->file_exists($ovmf)) { | ||
| $ovmf = '/usr/share/OVMF/OVMF32_CODE_4M.fd'; | ||
| } | ||
| die "Unsupported UEFI in this architecture ".$type->toString() | ||
| ." missing file $ovmf" | ||
| unless $self->file_exists($ovmf); |
There was a problem hiding this comment.
In _xml_add_uefi(), the i686 branch keeps the default '/usr/share/OVMF/OVMF_CODE.fd' whenever that file exists, and only switches to the 32-bit OVMF path when OVMF_CODE.fd is missing. On hosts that have both files, i686 guests will never use OVMF32_CODE_4M.fd, which undermines the goal of enabling 32-bit UEFI. Prefer selecting the i686-specific OVMF file first (and only falling back to a generic one if the 32-bit file is missing), with an explicit existence check for the chosen path.
| if ( $arch eq 'x86_64' ) { | ||
| $ovmf = '/usr/share/OVMF/OVMF_CODE_4M.fd'; |
There was a problem hiding this comment.
_xml_add_uefi() no longer ensures that x86_64 UEFI domains use a q35 machine type (the previous logic upgraded non-q35 machines to a pc-q35-* variant). Since the default domain XML templates in etc/xml include pc-i440fx-* machines, requesting UEFI without explicitly setting machine can now produce UEFI+i440fx configs that the rest of the test suite avoids (e.g., tests only exercise UEFI on q35). Consider restoring the q35 enforcement for x86_64 UEFI or handling invalid machine/UEFI combinations earlier with a clear error.
| $found_q35++; | ||
| my ($loader) = $doc->findnodes('/domain/os/loader/text()'); | ||
| like($loader,qr/OVMF_CODE_4M.fd$/) or die $domain->name; | ||
| like($loader,qr/OVMF_CODE.4M.fd$/i) or die $domain->name; |
There was a problem hiding this comment.
The regex qr/OVMF_CODE.4M.fd$/i uses . which matches any character, so it will also match unintended loader filenames. If the goal is to accept both underscore and dash variants, use a more specific pattern (e.g., a character class) so the test only matches the expected OVMF 4M firmware name.
| like($loader,qr/OVMF_CODE.4M.fd$/i) or die $domain->name; | |
| like($loader,qr/OVMF_CODE[_-]4M\.fd$/i) or die $domain->name; |
Allows UEFI in 32 bits Virtual machines