Re: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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
On 09/21/2014 10:58 AM, Laszlo Ersek wrote:
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").)
Thanks for the analysis Laszlo. Things worked fine for me originally... but now that I've played with this some more, there is definitely some funkiness here at the libvirt level that explains it. When the VM is defined, its XML looks like this: <os> <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <boot dev='network'/> </os> Which is fine. After the first time the VM starts, the XML looks like: <os> <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/qemu/nvram/ovmf_VARS.fd</nvram> <boot dev='network'/> </os> The generated nvram path is in the XML regardless of VM state from that point forward. Seems fine. However, when libvirtd is restarted, the generated nvram path is gone. So some part of the process is not actually saving the path to disk. That seems problematic. I say that at define time, if there's a template, we should do the copying then and hardcode the nvram path before even initially saving the XML to disk. Then it's safely hardcoded forever. That's similar to the qemu-ga socket path auto generation, where libvirt will generate a path for us in the proper location, it seems to be hardcoded at define time (though it doesn't create the socket). (also, another issue I just noticed... libvirt tries to use /var/lib/libvirt...nvram path if connected to qemu:///session, probably wants whatever the typical path under $HOME is)
(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 for the virt-manager patch, I've applied it now. - Cole
On 09/22/14 18:37, Cole Robinson wrote:
On 09/21/2014 10:58 AM, Laszlo Ersek wrote:
(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").)
Thanks for the analysis Laszlo.
Things worked fine for me originally... but now that I've played with this some more, there is definitely some funkiness here at the libvirt level that explains it.
When the VM is defined, its XML looks like this:
<os> <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <boot dev='network'/> </os>
Which is fine. After the first time the VM starts, the XML looks like:
<os> <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <nvram>/var/lib/libvirt/qemu/nvram/ovmf_VARS.fd</nvram> <boot dev='network'/> </os>
The generated nvram path is in the XML regardless of VM state from that point forward. Seems fine.
However, when libvirtd is restarted, the generated nvram path is gone. So some part of the process is not actually saving the path to disk.
Yes, the pathname is generated and/or the contents are copied in qemuProcessStart() qemuPrepareNVRAM() which I gather are called when you start the VM, not when you define it.
That seems problematic.
I say that at define time, if there's a template, we should do the copying then and hardcode the nvram path before even initially saving the XML to disk. Then it's safely hardcoded forever. That's similar to the qemu-ga socket path auto generation, where libvirt will generate a path for us in the proper location, it seems to be hardcoded at define time (though it doesn't create the socket).
(2) I think that minimally the copying from the varstore template must happen at startup. Otherwise, if you lose your varstore file, you won't be able to start the VM at all, until you recreate the varstore manually from the template (which is exactly what libvirtd should save you from). A particular, valid use case for this functionality is when you want to "reset" your variable store. You just delete the old one (when the VM is shut down) and libvirt will recreate it for you, for the pristine template, at next VM startup. (1) Moving the generation of the varstore pathname to define time is also somewhat brittle, similarly to the above. If you accidentally deleted the nvram element while in "virsh edit", then the proposed change (== don't generate an nvram pathname at startup, if it's missing) would prevent the VM from starting, and you'd have to restore the element manually. BTW I could not find the qemu-ga socket generation code in libvirt. (I grepped for "org.qemu.guest_agent".) I did find something relevant in "virtinst/devicechar.py" though, and CHANNEL_NAME_QEMUGA leads further. Also I think if you accidentally remove the guest agent channel from the domain XML, it won't break the VM completely, and (I think!) you can even re-add it in virt-manager. I think this can be made robust, we "just" need to start thinking about the nvram element's text contents as "procedural" -- probably a getter function should be factored out. If the text() child exists, then that's the returned value (as a human-provided override), if it doesn't, then the name should be auto-generated, based on the VM's UUID. I agree that this is complex, but the domain XML should accommodate all of the following: - simple loader element, or type='rom': usual bios thing - type='pflash' readonly='no': unified OVMF.fd file containing both binary and varstore. Consequences: - no concept of a varstore template - no concept of a varstore - hence no pathname generation and no contents copying - this is a "fringe" case for completeness only - type='pflash' readonly='yes': split OVMF binary and varstore. Consequences: a. loader specifies the binary only b. nvram text() child enables a user-provided, VM-specific varstore pathname, if present c. lack of nvram text() child requests auto-generation based on VM name ^W UUID (see my earlier patch about the UUID) d. in nvram's template attribute, if present, the user provides the pathname of a varstore template file compatible with the loader binary e. if nvram's template attribute is missing, then libvirtd insists on being able to resolve the loader binary to a varstore template using qemu.conf's "nvram" setting. If you restrict (c) or (d)/(e) to define time only, then things will work in the "optimal" case, but as soon as the user deletes or loses the VM-specific varstore, and/or loses the <nvram> element, things will break much less gracefully. In any case I don't insist either way; I'll defer to Michal and you.
(also, another issue I just noticed... libvirt tries to use /var/lib/libvirt...nvram path if connected to qemu:///session, probably wants whatever the typical path under $HOME is)
I don't know what the "typical path under $HOME is", but this case is certainly addressible with (b) -- as long as the tools expose it. :) Virt-install does: --boot loader=/usr/share/OVMF/OVMF_CODE.fd,loader_ro=yes,loader_type=pflash,nvram=$HOME/.../guest-VARS.fd This is (b)+(e). --boot loader=/custom/OVMF_CODE.fd,loader_ro=yes,loader_type=pflash,nvram_template=/custom/OVMF_VARS.fd,nvram=$HOME/.../guest-VARS.fd This is (b)+(d).
(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 for the virt-manager patch, I've applied it now.
Thanks! Laszlo
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
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
On 09/25/2014 10:08 AM, Laszlo Ersek wrote:
On 09/25/14 14:59, Michal Privoznik wrote:
(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.)
Yeah and virt-manager's support is basically a hack, since rename won't happen if there's nvram, or managed save, or snapshots for example. But the virt-manager support predates those bits, when rename wasn't too problematic. We really just need a proper 'rename' API that handles migrating all the hidden stuff that is dependent on VM name. - Cole
participants (3)
-
Cole Robinson -
Laszlo Ersek -
Michal Privoznik