Re: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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