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@xxxxxxxxxx>
----------------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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
