9:08 a.m.
On 09/25/14 14:59, Michal Privoznik wrote:
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.
As far as I remember, this was Cole's idea too. And -- again if I
remember correctly -- I tried to describe in response why I thought the
current way (ie. not writing the generated nvram pathname back into the
XML file) was superior. (I'd rather not repeat it now, please just read
it in the thread.)
But: if you disagree with that, or think that the benefits of
immediately serializing the generated nvram pathname to the XML file
would outweigh the stuff that I described earlier, then please go ahead
with that solution. I really don't "insist", I'm fine either way. (And
AIUI Cole already agrees with you.)
> (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, ...).
Ah! I didn't know this. It's very interesting because virt-manager in
fact offers you to rename the VM -- the Details | Overview | Name field
is editable, not just a label. (The UUID *is* a label.)
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.
I fully agree with that -- I (obviously) tested both ways, and the
UUID-based filenames are just completely opaque. First thing you do is
grep the XML files under /etc/libvirt/qemu/, to see which domain owns
the varstore in question.
BTW
that's the reason why we still use /etc/libvirt/qemu/$dom_name.xml.
Understood.
> 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?
Summary:
- feel free to drop the 3rd patch,
- if you just consider the 2nd patch and come up with *any* (matching,
or different) solution to the nvram-leaked-on-undefine problem, I'll
be thankful.
Cheers!
Laszlo