[virt-tools-list] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'

Thu Sep 18 12:28:01 UTC 2014

On 09/18/2014 08:00 AM, Laszlo Ersek wrote:
> On 09/18/14 00:55, Cole Robinson wrote:
>> We expose a simple combobox with two entries: BIOS, and UEFI. The
>> UEFI option is only selectable if 1) libvirt supports the necessary
>> domcapabilities bits, 2) it detects that qemu supports the necessary
>> command line options, and 3) libvirt detects a UEFI binary on the
>> host that maps to a known template via qemu.conf
>>
>> If those conditions aren't met, we disable the UEFI option, and show
>> a small warning icon with an explanatory tooltip.
>>
>> The option can only be changed via New VM->Customize Before Install.
>> For existing x86 VMs, it's a readonly label.
>> ---
>>  ui/details.ui          | 84 ++++++++++++++++++++++++++++++++++++++++++++++----
>>  virtManager/details.py | 72 ++++++++++++++++++++++++++++++++++++++++++-
>>  virtManager/domain.py  | 32 ++++++++++++++++++-
>>  virtinst/support.py    |  3 ++
>>  4 files changed, 183 insertions(+), 8 deletions(-)
> 
> (a) I picked / ported the following upstream libvirt patches to
> libvirt-1.2.8-2.el7:
> 
>    1  68bf13d conf: Extend <loader/> and introduce <nvram/>
>    2  5428991 qemu: Implement extended loader and nvram
>    3  742b08e qemu: Automatically create NVRAM store
>    4  37d8c75 nvram: Fix permissions
>    5  3c07693 libvirt.spec: Fix permission even for libvirt-driver-qemu
>    6  273b658 virDomainUndefineFlags: Allow NVRAM unlinking
>    7  dcf7d04 formatdomain: Update <loader/> example to match the rest
>    8  4f76621 domaincaps: Expose UEFI capability
>    9  2b2e4a7 qemu_capabilities: Change virQEMUCapsFillDomainCaps
>               signature
>   10  f05b6a9 domaincaps: Expose UEFI binary path, if it exists
>   11  b3f42da domaincapstest: Run cleanly on systems missing OVMF
>               firmware
> 
> (b) I picked / ported the following upstream virt-manager patches to
> virt-manager-1.1.0-2.el7:
> 
>   1  1341928 test: Fix tests with latest libvirt
>   2  4a83ea3 test: skip unit tests affected by loader extention before
>              libvirt 1.2.9
>   3  30c3434 test: update compare_check flags for auto-clone cases
>   4  d2fffa5 virt-install: add support for OVMF
>   5  17a37ea virt-install: add tests for OVMF
>   6  e5d2059 virt-manager: delete nvram file on VM deletion
>   7  052220c virtinst: Add DomainCapabilities parser
>   8  ead9d3f domain: If VM has nvram, ask libvirt to remove it on
>              undefine
> 
> (c) I then applied this patch with git-am.
> 
> Results:
> - UI looks good
> - works as expected (chooses UEFI binary from the first nvram list
>   element)
> - When I delete the VM, using virt-manager, the VM-specific varstore
>   file is still leaked (ie. it is left under
>   /var/lib/libvirt/qemu/nvram); but that's probably not the scope of
>   this patch (*).
> 
> So, for this patch:
> 
> Tested-by: Laszlo Ersek <lersek at redhat.com>
> 
> ----------------o----------------
> 
> (*) Regarding the varstore file leak: as I said before, the leak occurs
> only when the domain XML does not contain an <nvram>PATHNAME</nvram>
> element. (This is valid BTW because in this case libvirt will (try to)
> figure out everything -- it will resolve the pathname of the varstore
> template from the nvram stanza in qemu.conf, then it will figure out the
> pathname of the vm-specific varstore instance, and then instantiate the
> latter from the former if the latter is missing.)
> 
> So, virt-manager patches e5d2059 and ead9d3f are:
> (i) redundant (apparently), because we either display the nvram file
>     ourselves and delete it too (--> e5d2059), or we ask libvirt to do
>     it (--> ead9d3f),
> 
> (ii) and ineffective, both, because they both use
>      get_xmlobj().os.nvram, which does not exist if the <nvram> element
>      is missing (which is valid, again).
> 
> My suggestion:
> - revert e5d2059,
> - fix up ead9d3f so that setting VIR_DOMAIN_UNDEFINE_NVRAM occur *iff*:
> 
>   get_xmlobj().loader_ro && get_xmlobj().loader_type == "pflash"
> 
> Because that's the condition that libvirt uses, at creation / startup
> time, to decide whether the VM-specific varstore file should *exists*
> (regardless of how its name and its contents are deduced).
> 
> Namely, see the qemuPrepareNVRAM() function: the above condition
> triggers libvirt to ensure the non-nullity of "loader->nvram". Then, in
> qemuDomainUndefineFlags(), the same "vm->def->os.loader->nvram" is
> checked for non-nullity, to see if VIR_DOMAIN_UNDEFINE_NVRAM is required.
> 

Thanks for the analysis, indeed we should try and match libvirt's logic here.

> ... Honestly, it would be simplest for virt-manager to *always* set
> VIR_DOMAIN_UNDEFINE_NVRAM. As far as I can see, it will be silently
> ignored when it is not used.
> 
> IOW: please consider reverting e5d2059, and de-conditionalizing ead9d3f
> -- which would BTW precisely match how
> VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA and
> VIR_DOMAIN_UNDEFINE_MANAGED_SAVE are set too!
> 

There's problems with doing it unconditionally though.

- Say you are connecting from a new libvirt UNDEFINE_NVRAM support (say Fedora
21 GA), to an old libvirt without it, like F20 or RHEL7.0. If we specify the
flag unconditionally, the undefineFlags call will fail, which also means that
we lose the benefit of the other UNDEFINE flags. So if the VM had managedsave
data, the undefine will fail saying we need to remove it first.

- Not all drivers support the UNDEFINE_NVRAM flag, like the 'test' driver
which we use heavily for virt-manager development. If we pass the flag
unconditionally, then the current code loses the benefit of all UNDEFINE
flags, and trying to delete a VM with managedsave data will fail.

The existing code has similar problems as well though, since we lump all the
UNDEFINE flags together, so it certainly isn't perfect. However my hack of
checking for nvram in the XML avoids some of the common issues, so I'll extend
it with the logic you pointed out.

Trying to figure out if an API or flag is supported with an arbitrary libvirt
connection is a pain: it's a factor of host libvirt version, host
libvirt-python version, remote libvirt version, libvirt hv driver, hv version.
Some readonly APIs you can get away with just testing first, but undefine
isn't one of them :) Maybe we can get a libvirt API exposing the driver
function table at least?

- Cole