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
>From b8adf86d6ebee7af4aa11e9810e55276c58d7e98 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@xxxxxxxxxx>
Date: Sun, 21 Sep 2014 13:12:48 +0200
Subject: [PATCH] domain: relax the UNDEFINE_NVRAM condition precisely to
libvirtd's one
(1) A separate nvram (ie. variable store) file for the domain exists if
and only if the following condition holds:
(self.get_xmlobj().os.loader_ro is True and
self.get_xmlobj().os.loader_type == "pflash")
(Refer to libvirtd's qemuPrepareNVRAM() function, as of commit 742b08e30.)
(2) The
self.get_xmlobj().os.nvram
condition is sufficient, but not necessary, for the separate varstore file
to exist. That is, if the condition holds, then the separate varstore file
exists for sure, but if the condition doesn't hold, the file may exist
nonetheless. (Because libvirtd can auto-generate the varstore file's
pathname.)
This means that requiring condition (2) for setting UNDEFINE_NVRAM on
domain deletion will miss a subset of the cases, ie. when the necessary
and sufficient condition (1) holds, but the sufficient-only condition (2)
doesn't.
Make sure that the code uses the necessary and sufficient condition (1).
Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
---
virtManager/domain.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/virtManager/domain.py b/virtManager/domain.py
index b28f454..585f64b 100644
--- a/virtManager/domain.py
+++ b/virtManager/domain.py
@@ -1399,8 +1399,7 @@ class vmmDomain(vmmLibvirtObject):
"VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA", 0)
flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_MANAGED_SAVE", 0)
if (self.get_xmlobj().os.loader_ro is True and
- self.get_xmlobj().os.loader_type == "pflash" and
- self.get_xmlobj().os.nvram):
+ self.get_xmlobj().os.loader_type == "pflash"):
flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0)
try:
self._backend.undefineFlags(flags)
--
1.8.3.1
>From 40293f2ba8b6611cc4f57478700f076bd254fc06 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@xxxxxxxxxx>
Date: Sun, 21 Sep 2014 15:07:34 +0200
Subject: [PATCH 1/2] qemuDomainUndefineFlags: auto-generate nvram pathname
when necessary
The condition
(loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
loader->readonly == VIR_TRISTATE_SWITCH_ON)
is equivalent to the domain having a separate varstore (nvram) file. The
pathname of this file can be explicitly set in the domain XML, or
auto-generated by libvirtd. In
qemuProcessStart()
qemuPrepareNVRAM()
the pathname is auto-generated only when starting qemu.
Auto-generate the varstore pathname also when undefining the domain, so
that the varstore file is not leaked when its pathname is not specified
explicitly in the domain XML.
Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
---
src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b888b09..16cd3e3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6415,6 +6415,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
int ret = -1;
int nsnapshots;
virQEMUDriverConfigPtr cfg = NULL;
+ virDomainLoaderDefPtr loader;
+ bool nvramPathnameGenerated = false;
virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE |
VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA |
@@ -6468,19 +6470,31 @@ qemuDomainUndefineFlags(virDomainPtr dom,
}
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;
+ (loader = vm->def->os.loader) &&
+ loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
+ loader->readonly == VIR_TRISTATE_SWITCH_ON) {
+ if (!loader->nvram) {
+ if (virAsprintf(&loader->nvram,
+ "%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
+ LOCALSTATEDIR, vm->def->name) < 0)
+ goto cleanup;
+
+ nvramPathnameGenerated = true;
}
- if (unlink(vm->def->os.loader->nvram) < 0) {
- virReportSystemError(errno,
- _("failed to remove nvram: %s"),
- vm->def->os.loader->nvram);
- goto cleanup;
+ if (virFileExists(loader->nvram)) {
+ if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot delete inactive domain with nvram"));
+ goto cleanup;
+ }
+
+ if (unlink(loader->nvram) < 0) {
+ virReportSystemError(errno,
+ _("failed to remove nvram: %s"),
+ loader->nvram);
+ goto cleanup;
+ }
}
}
@@ -6507,6 +6521,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
ret = 0;
cleanup:
+ if (nvramPathnameGenerated)
+ VIR_FREE(loader->nvram);
VIR_FREE(name);
if (vm)
virObjectUnlock(vm);
--
1.8.3.1
>From 5fd4f9a9430c757aae8681ba7e9f65ec55bb7889 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@xxxxxxxxxx>
Date: Sun, 21 Sep 2014 16:07:22 +0200
Subject: [PATCH 2/2] qemu NVRAM: auto-generated pathnames should contain
domain UUIDs
The name of a domain is unstable in the sense that the domain can be
renamed. Currently a rename will cause the domain to lose connection with
its separate varstore file, if the pathname of the varstore was
automatically generated.
Hence generate such pathnames based on the domain's UUID, because that one
never changes.
Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
---
src/qemu/qemu_driver.c | 7 +++++--
src/qemu/qemu_process.c | 7 +++++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16cd3e3..9ea1b5b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6474,9 +6474,12 @@ qemuDomainUndefineFlags(virDomainPtr dom,
loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
loader->readonly == VIR_TRISTATE_SWITCH_ON) {
if (!loader->nvram) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
if (virAsprintf(&loader->nvram,
- "%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
- LOCALSTATEDIR, vm->def->name) < 0)
+ "%s/lib/libvirt/qemu/nvram/%s-VARS.fd",
+ LOCALSTATEDIR,
+ virUUIDFormat(vm->def->uuid, uuidstr)) < 0)
goto cleanup;
nvramPathnameGenerated = true;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 13614e9..57bde45 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3866,9 +3866,12 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
/* Autogenerate nvram path if needed.*/
if (!loader->nvram) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
if (virAsprintf(&loader->nvram,
- "%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
- LOCALSTATEDIR, def->name) < 0)
+ "%s/lib/libvirt/qemu/nvram/%s-VARS.fd",
+ LOCALSTATEDIR,
+ virUUIDFormat(def->uuid, uuidstr)) < 0)
goto cleanup;
generated = true;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list