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

[Laszlo Ersek]

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.

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

Cheers,
Laszlo