9:58 a.m.
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
11:37 a.m.
New subject: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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
1:05 p.m.
New subject: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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
7:59 a.m.
New subject: [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
9:08 a.m.
New subject: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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
9:11 a.m.
New subject: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'
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
3711
days inactive
3715
days old
5 comments
3 participants
participants (3)
-
Cole Robinson -
Laszlo Ersek -
Michal Privoznik