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

[Laszlo Ersek]

On 09/18/14 14:28, Cole Robinson wrote:
> 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.

I considered that -- after all, libvirt's qemuDomainUndefineFlags()
function, if it is earlier than 273b6581, will reject
VIR_DOMAIN_UNDEFINE_NVRAM (with value (1 << 2)).

However, the virt-manage code says:

  flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0)

and I took that for

  if libvirt knows about "VIR_DOMAIN_UNDEFINE_NVRAM", then OR it in;
  otherwise, OR in zero

So, if you are connected to an older libvirtd, one that has no knowledge
of VIR_DOMAIN_UNDEFINE_NVRAM, then the above getattr should return 0 anyway.

Is that correct?

Hmm... no, it isn't actually. Sorry. You are right. This getattr() thing
is apparently a python builtin function, and the value it returns
depends on the "libvirt.py" module that virt-manager is "linked"
against, locally (you mention "libvirt-python" below, correctly). That
is, it doesn't depend on the *remote* libvirt daemon.

Fine, in that case we should go with the elaborate condition (loader_ro
&& loader_type==pflash).

> - 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.

Thanks!

> 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?

That's for libvirt developers to answer! ;)

FWIW, in this case, if you determine that the domain has an nvram (uefi
varstore) file, based on the above check, then it won't be possible for
the remote daemon not to know about the flag. I think there won't be a
libvirt release separating the one feature from the other.

Thanks
Laszlo