[PATCH v2] virtmanager: Fix legacy firmware installation error

[Cole Robinson]

On 3/10/21 4:02 PM, Charles Arnold wrote:
> When we choose /usr/share/qemu/bios.bin or /usr/share/qemu/bios-256k.bin
> in Customize configuration before install, show this error message:
> 
> Unable to complete install: 'operation failed: unable to find any master var store for loader: /usr/share/qemu/bios.bin'
> 
> Traceback (most recent call last):
>   File "/usr/share/virt-manager/virtManager/asyncjob.py", line 65, in cb_wrapper
>     callback(asyncjob, *args, **kwargs)
>   File "/usr/share/virt-manager/virtManager/createvm.py", line 2081, in _do_async_install
>     installer.start_install(guest, meter=meter)
>   File "/usr/share/virt-manager/virtinst/install/installer.py", line 721, in start_install
>     domain = self._create_guest(
>   File "/usr/share/virt-manager/virtinst/install/installer.py", line 669, in _create_guest
>     domain = self.conn.createXML(install_xml or final_xml, 0)
>   File "/usr/lib64/python3.8/site-packages/libvirt.py", line 4347, in createXML
>     raise libvirtError('virDomainCreateXML() failed')
> libvirt.libvirtError: operation failed: unable to find any master var store for loader: /usr/share/qemu/bios.bin
> 
> This patch checks the loader for bios.bin or bios-256k.bin and prevents them
> from begin presented as UEFI files (type pflash) but instead as type 'rom'.
> 

Sorry for the late response. From your domcapabilities mail I guess suse
is shipping firmware.json files for bios*.bin? I guess that's acceptable
but it's a little strange for historical usage, no one should really be
manually choosing between bios.bin and bios-256k.bin manually for
example. Anyways...

> Signed-off-by: Charles Arnold <carnold at suse.com>
> Signed-off-by: Yu-Chen Cho <acho at suse.com>
> 
> ---
>  virtManager/object/domain.py | 18 +++++++++---------
>  virtinst/guest.py            | 13 +++++++++++++
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/virtManager/object/domain.py b/virtManager/object/domain.py
> index cc2f506d..21841ada 100644
> --- a/virtManager/object/domain.py
> +++ b/virtManager/object/domain.py
> @@ -671,16 +671,16 @@ class vmmDomain(vmmLibvirtObject):
>  
>          if loader != _SENTINEL:
>              if loader is None:
> -                # Implies seabios, aka the default, so clear everything
> -                guest.os.loader = None
> -                guest.os.loader_ro = None
> -                guest.os.loader_type = None
> -                guest.os.nvram = None
> -                guest.os.nvram_template = None
> +                # Implies the default, so clear everything
> +                guest.set_legacy_path(None)
>              else:
> -                # Implies UEFI
> -                guest.set_uefi_path(loader)
> -                guest.disable_hyperv_for_uefi()
> +                if "bios.bin" in loader or "bios-256k.bin" in loader:
> +                    # Implies Custom: seabios, not UEFI
> +                    guest.set_legacy_path(loader)
> +                else:
> +                    # Implies UEFI
> +                    guest.set_uefi_path(loader)
> +                    guest.disable_hyperv_for_uefi()
>  
>          if nvram != _SENTINEL:
>              guest.os.nvram = nvram
> diff --git a/virtinst/guest.py b/virtinst/guest.py
> index 34da3e8c..52f49252 100644
> --- a/virtinst/guest.py
> +++ b/virtinst/guest.py
> @@ -566,6 +566,19 @@ class Guest(XMLBuilder):
>                          self.os.machine)
>                  self.os.machine = "q35"
>  
> +    def set_legacy_path(self, path):
> +        """
> +        Configure legacy BIOS for QEMU.
> +        """
> +        self.os.loader = path
> +        if (path):
> +            self.os.loader_ro = True
> +        else:
> +            self.os.loader_ro = None
> +        self.os.loader_type = None
> +        self.os.nvram = None
> +        self.os.nvram_template = None
> +

This still has the same problem I pointed out in previous mails, that
`pytest --cov` now regresses. Anything added to virtinst/ needs unit
test coverage.

I think the simpler thing to do here is extend the 'if loader is None'
in details.py to handle bios*.bin too. Then the logic stays self
contained in virt-manager which is the only one that needs this code
path so far, and the change will be smaller.

Thanks,
Cole