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

[Michal Privoznik]

On 21.09.2014 16:58, Laszlo Ersek wrote:
> Hi Cole and Michal,
>
> I'm attaching three patches:
>
> <snip/>
 >
> (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").)


The original problem is, that once the path is generated, the config XML 
under /etc/libvirt/qemu/ is not updated as it need to. Having said that, 
this patch is then not needed anymore.

>
>
> (3) I just realized that a domain's name can change during its lifetime.

Well, the only moment that we support that is migration. Otherwise the 
name can't be changed (it's not only varstore path that's derived from 
domain's name, consider snapshots, socket names, ...). Having said that, 
the current code is not safe, because the domain name may contain 
characters not supported by the FS mounted on 
/var/lib/libvirt/qemu/nvram. But if that's the case users can just 
provide the acceptable nvram path. And that's the case in migration too 
- users can pass not only new domain name but new domain XML too. And 
the XML can contain changed nvram path to match domain name. Therefore 
I'm undecided if this patch is needed. I mean, seeing bare UUIDs under 
/var/.../nvram directory doesn't make the life easier for admins. BTW 
that's the reason why we still use /etc/libvirt/qemu/$dom_name.xml.

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


Michal