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

Sun Sep 21 14:58:59 UTC 2014

Hi Cole and Michal,

I'm attaching three patches:

On 09/20/14 02:37, Cole Robinson wrote:
> On 09/17/2014 06:55 PM, 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.
>
> I've pushed this now, with follow up patches to handle a couple points
> we discussed:
>
> - Remove the nvram deletion from delete.py, since it won't work in the
> non-root case, and all uses of nvram should also be with a libvirt +
> driver that provides UNDEFINE_NVRAM
>
> - Before using UNDEFINE_NVRAM, match the same condition that libvirt
> uses: loader_ro is True and loader_type == "pflash" and bool(nvram)

(1) unfortunately virt-manager commit 3feedb76 ("domain: Match
UNDEFINE_NVRAM conditions with libvirt") is not correct (it doesn't
work), and not what I had in mind:

> diff --git a/virtManager/domain.py b/virtManager/domain.py
> index 29f3861..abf3146 100644
> --- a/virtManager/domain.py
> +++ b/virtManager/domain.py
> @@ -1403,7 +1403,9 @@ class vmmDomain(vmmLibvirtObject):
>              flags |= getattr(libvirt,
>                               "VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA", 0)
>              flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_MANAGED_SAVE", 0)
> -            if self.get_xmlobj().os.nvram:
> +            if (self.get_xmlobj().os.loader_ro is True and
> +                self.get_xmlobj().os.loader_type == "pflash" and
> +                self.get_xmlobj().os.nvram):
>                  flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0)
>          try:
>              self._backend.undefineFlags(flags)

Before virt-manager commit 3feedb76, the condition on which to set the
flag was:

  self.get_xmlobj().os.nvram

and it didn't work for all possible cases.

After the patch, what you have is

  self.get_xmlobj().os.nvram *and* blah

The commit only constrains (further tightens, further restricts) the
original condition, which had been too restrictive to begin with. Again,
the nvram element (or its text() child) can be completely absent from
the domain XML -- libvirtd will generate the pathname of the varstore
file on the fly, and it need not be part of the serialized XML file. So
in the XML all you'll have is

  <os>
    <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
    <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
    <boot dev='cdrom'/>
  </os>

or

  <os>
    <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
    <loader readonly='yes' type='pflash'>/custom/OVMF_CODE.fd</loader>
    <nvram template='/custom/OVMF_VARS.fd'/>
    <boot dev='cdrom'/>
  </os>

My suggestion was to *replace* the original condition with the
(loader_ro && loader_type=="pflash") one. If you check my earlier
message, I wrote *iff*, meaning "if and only if".

Cole, can you please apply the first attached patch?

(2) Unfortunately, even libvirtd needs to be modified, in addition.

My patch for (1) *does* pass VIR_DOMAIN_UNDEFINE_NVRAM to libvirtd (I
verified that with gdb), but libvirtd doesn't act upon it correctly.
Namely, in the libvirtd source code, in qemuDomainUndefineFlags(),
commit 273b6581 added:

    if (!virDomainObjIsActive(vm) &&
        vm->def->os.loader && vm->def->os.loader->nvram &&
        virFileExists(vm->def->os.loader->nvram)) {
        if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                           _("cannot delete inactive domain with nvram"));
            goto cleanup;
        }

        if (unlink(vm->def->os.loader->nvram) < 0) {
            virReportSystemError(errno,
                                 _("failed to remove nvram: %s"),
                                 vm->def->os.loader->nvram);
            goto cleanup;
        }
    }

Here "vm->def->os.loader->nvram" evaluates to NULL:

  6468        if (!virDomainObjIsActive(vm) &&
  6469            vm->def->os.loader && vm->def->os.loader->nvram &&
  6470            virFileExists(vm->def->os.loader->nvram)) {
  6471            if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
  6472                virReportError(VIR_ERR_OPERATION_INVALID, "%s",

  (gdb) print vm->def->os.loader
  $1 = (virDomainLoaderDefPtr) 0x7ff59c00bf20

  (gdb) print vm->def->os.loader->nvram
  $2 = 0x0

I thought that the auto-generation of the nvram pathname (internal to
libvirtd, ie. not visible in the XML file) would occur on the undefine
path as well. Apparently this is not the case.

Indeed, the only call to qemuPrepareNVRAM() is from qemuProcessStart().

Michal, would it be possible generate the *pathname* of the separate
varstore in qemuDomainUndefineFlags() too?

Please see the second attached patch; it works for me. (This patch is
best looked at with "git diff -b" (or "git show -b").)

(3) I just realized that a domain's name can change during its lifetime.
Renaming a domain becomes a problem when the varstore's pathname is
deduced from the domain's name. For this reason, the auto-generation
should use the domain's UUID (which never changes). Michal, what do you
think of the 3rd attached patch?

I can resubmit these as standalone patches / series as well (the first
to virt-tools-list, and the last two to libvir-list), if that's
necessary.

Thanks,
Laszlo

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-domain-relax-the-UNDEFINE_NVRAM-condition-precisely-.patch
Type: text/x-patch
Size: 2047 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140921/bd0a6ef4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qemuDomainUndefineFlags-auto-generate-nvram-pathname.patch
Type: text/x-patch
Size: 3422 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140921/bd0a6ef4/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-qemu-NVRAM-auto-generated-pathnames-should-contain-d.patch
Type: text/x-patch
Size: 2336 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140921/bd0a6ef4/attachment-0002.bin>