[PATCH v3] qemu: Don't report spurious errors from vCPU tid validation on hotunplug timeout
by Peter Krempa
From: Shaleen Bathla <shaleen.bathla(a)oracle.com>
Use of qemuDomainValidateVcpuInfo in the helpers for hotplug and unplug
of vCPUs can lead to spurious errors reported such as:
internal error: qemu didn't report thread id for vcpu 'XX'"
The reason for this is that qemuDomainValidateVcpuInfo validates the
state of all vCPUs against the expected state of vCPUs. If an unplug
operation completed before libvirt was unable to process it yet the
expected state could not reflect the current state.
To avoid spurious errors the qemuDomainHotplugAddVcpu and
qemuDomainRemoveVcpu functions are modified to do localized validation
only for the vCPUs they actually modify.
We also now ensure that the cgroups are modified before bailing out on
error for any vCPUs which passed validation.
Additionally in order for qemuDomainRemoveVcpuAlias to be able to find
the unplugged vCPU we must ensure that qemuDomainRefreshVcpuInfo does
not clear out the alias in case when the vCPU is no longer reported by
qemu.
Co-authored-by: Partha Satapathy <partha.satapathy(a)oracle.com>
Signed-off-by: Shaleen Bathla <shaleen.bathla(a)oracle.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
v3 addresses my review feedback of the original patch, as well as
rewrites the commit message for more clarity.
src/qemu/qemu_domain.c | 6 +++--
src/qemu/qemu_hotplug.c | 53 ++++++++++++++++++++++++-----------------
2 files changed, 35 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ef1a9c8c74..64ebec626c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9795,8 +9795,10 @@ qemuDomainRefreshVcpuInfo(virDomainObj *vm,
vcpupriv->vcpus = info[i].vcpus;
VIR_FREE(vcpupriv->type);
vcpupriv->type = g_steal_pointer(&info[i].type);
- VIR_FREE(vcpupriv->alias);
- vcpupriv->alias = g_steal_pointer(&info[i].alias);
+ if (info[i].alias) {
+ VIR_FREE(vcpupriv->alias);
+ vcpupriv->alias = g_steal_pointer(&info[i].alias);
+ }
virJSONValueFree(vcpupriv->props);
vcpupriv->props = g_steal_pointer(&info[i].props);
vcpupriv->enable_id = info[i].id;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index da92ced2f4..6e300f547c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6088,38 +6088,37 @@ qemuDomainRemoveVcpu(virDomainObj *vm,
qemuDomainVcpuPrivate *vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
int oldvcpus = virDomainDefGetVcpus(vm->def);
unsigned int nvcpus = vcpupriv->vcpus;
- virErrorPtr save_error = NULL;
size_t i;
+ ssize_t offlineVcpuWithTid = -1;
if (qemuDomainRefreshVcpuInfo(vm, VIR_ASYNC_JOB_NONE, false) < 0)
return -1;
- /* validation requires us to set the expected state prior to calling it */
for (i = vcpu; i < vcpu + nvcpus; i++) {
vcpuinfo = virDomainDefGetVcpu(vm->def, i);
- vcpuinfo->online = false;
+ vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
+
+ if (vcpupriv->tid == 0) {
+ vcpuinfo->online = false;
+ /* Clear the alias as VCPU is now unplugged */
+ VIR_FREE(vcpupriv->alias);
+ ignore_value(virCgroupDelThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i));
+ } else {
+ if (offlineVcpuWithTid == -1)
+ offlineVcpuWithTid = i;
+ }
}
- if (qemuDomainValidateVcpuInfo(vm) < 0) {
- /* rollback vcpu count if the setting has failed */
+ if (offlineVcpuWithTid != -1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("qemu reported thread id for inactive vcpu '%zu'"),
+ offlineVcpuWithTid);
virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", false);
-
- for (i = vcpu; i < vcpu + nvcpus; i++) {
- vcpuinfo = virDomainDefGetVcpu(vm->def, i);
- vcpuinfo->online = true;
- }
return -1;
}
virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", true);
- virErrorPreserveLast(&save_error);
-
- for (i = vcpu; i < vcpu + nvcpus; i++)
- ignore_value(virCgroupDelThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i));
-
- virErrorRestore(&save_error);
-
return 0;
}
@@ -6141,6 +6140,9 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm,
return;
}
}
+
+ VIR_DEBUG("vcpu '%s' not found in vcpulist of domain '%s'",
+ alias, vm->def->name);
}
@@ -6209,6 +6211,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
int rc;
int oldvcpus = virDomainDefGetVcpus(vm->def);
size_t i;
+ bool vcpuTidMissing = false;
if (!qemuDomainSupportsNewVcpuHotplug(vm)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -6238,20 +6241,26 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
if (qemuDomainRefreshVcpuInfo(vm, VIR_ASYNC_JOB_NONE, false) < 0)
return -1;
- /* validation requires us to set the expected state prior to calling it */
for (i = vcpu; i < vcpu + nvcpus; i++) {
vcpuinfo = virDomainDefGetVcpu(vm->def, i);
vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
vcpuinfo->online = true;
- if (vcpupriv->tid > 0 &&
- qemuProcessSetupVcpu(vm, i, true) < 0)
- return -1;
+ if (vcpupriv->tid > 0) {
+ if (qemuProcessSetupVcpu(vm, i, true) < 0) {
+ return -1;
+ }
+ } else {
+ vcpuTidMissing = true;
+ }
}
- if (qemuDomainValidateVcpuInfo(vm) < 0)
+ if (vcpuTidMissing && qemuDomainHasVcpuPids(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("qemu didn't report thread id for vcpu '%zu'"), i);
return -1;
+ }
qemuDomainVcpuPersistOrder(vm->def);
--
2.37.3
2 years, 3 months
[PATCH 0/9] Couple of virCommand related improvements
by Michal Privoznik
Found these on a abandoned branch, but they are still worth merging.
Polished them a bit though.
Michal Prívozník (9):
vircommand: Document virCommandSetSendBuffer() behaviour wrt daemonize
virCommandDoAsyncIO: Drop misleading statement about main event loop
commandtest: Use unsigned char in test27()
virCommandSetSendBuffer: Take double pointer of @buffer
commandtest: Test virCommandSetSendBuffer() with virCommandDoAsyncIO()
commandtest: Use virTestCompareToFile() in checkoutput()
virbuftest: Cleanup code around virTestDifference()
tests: Don't wrap virTestDifference() arguments in NULLSTR()
tests: Use virTestCompareToString() more
src/qemu/qemu_tpm.c | 2 +-
src/util/vircommand.c | 11 ++-
src/util/vircommand.h | 2 +-
tests/commanddata/test29.log | 20 ++++
tests/commandtest.c | 152 +++++++++++++++++++++++--------
tests/esxutilstest.c | 10 +-
tests/nwfilterebiptablestest.c | 21 ++---
tests/openvzutilstest.c | 6 +-
tests/sockettest.c | 3 +-
tests/utiltest.c | 3 +-
tests/vboxsnapshotxmltest.c | 3 +-
tests/virbuftest.c | 25 ++---
tests/virfirewalltest.c | 30 ++----
tests/virjsontest.c | 6 +-
tests/virkmodtest.c | 3 +-
tests/virnetdevbandwidthtest.c | 5 +-
tests/virnetdevopenvswitchtest.c | 10 +-
tests/virnetsockettest.c | 3 +-
tests/virshtest.c | 3 +-
19 files changed, 182 insertions(+), 136 deletions(-)
create mode 100644 tests/commanddata/test29.log
--
2.37.4
2 years, 4 months
[libvirt PATCH] apparmor: Allow running /usr/libexec/qemu-kvm
by Andrea Bolognani
Distros that use AppArmor, such as Debian and Ubuntu, install
QEMU under /usr/bin/qemu-system-*, and our AppArmor profile is
written with that assumption in mind.
If you try to run the RHEL or CentOS version of libvirt and
QEMU inside a privileged container on such distros, however,
that will result in an error, because the path
/usr/libexec/qemu-kvm is used instead.
In particular, this prevents upstream KubeVirt releases (which
are based on CentOS) from running on Debian/Ubuntu nodes. See
https://github.com/kubevirt/kubevirt/pull/8692
and the issues referenced therein for additional details.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/security/apparmor/usr.sbin.libvirtd.in | 4 ++++
src/security/apparmor/usr.sbin.virtqemud.in | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
index 886f1ad518..2994de5ec9 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -99,6 +99,10 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
# read and run an ebtables script.
/var/lib/libvirt/virtd* ixr,
+ # Needed when running the RHEL/CentOS version of libvirt and QEMU
+ # inside a privileged container on a Debian/Ubuntu host
+ /usr/libexec/qemu-kvm PUx,
+
# force the use of virt-aa-helper
audit deny /{usr/,}sbin/apparmor_parser rwxl,
audit deny /etc/apparmor.d/libvirt/** wxl,
diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in
index 3de03d49fc..b3f33b9471 100644
--- a/src/security/apparmor/usr.sbin.virtqemud.in
+++ b/src/security/apparmor/usr.sbin.virtqemud.in
@@ -94,6 +94,10 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
# read and run an ebtables script.
/var/lib/libvirt/virtd* ixr,
+ # Needed when running the RHEL/CentOS version of libvirt and QEMU
+ # inside a privileged container on a Debian/Ubuntu host
+ /usr/libexec/qemu-kvm PUx,
+
# force the use of virt-aa-helper
audit deny /{usr/,}sbin/apparmor_parser rwxl,
audit deny /etc/apparmor.d/libvirt/** wxl,
--
2.38.1
2 years, 4 months
[PATCH v4 0/7] qemu: tpm: Add support for migration across shared storage
by Stefan Berger
This series of patches adds support for migrating vTPMs across hosts whose
storage has been set up to share the directory structure holding the state
of the TPM (swtpm). The existence of share storage influences the
management of the directory structure holding the TPM state, which for
example is only removed when a domain is undefined and not when a VM is
removed on the migration source host. Further, when shared storage is used
then security labeling on the destination side is skipped assuming that the
labeling was already done on the source side.
I have tested this with an NFS setup where I had to turn SELinux off on
the hosts since the SELinux MLS range labeling is not supported by NFS.
For shared storage support to work properly both sides of the migration
need to be clients of the shared storage setup, meaning that they both have
to have /var/lib/libvirt/swtpm mounted as shared storage, because other-
wise the virFileIsSharedFS() may not detect shared storage and in the
worst case may cause the TPM emulator (swtpm) to malfunction if for
example the source side removed the TPM state directory structure.
Shared storage migration requires (upcoming) swtpm v0.8.
Stefan
v4:
- Fixed long-standing bug regarding offline migration that now blocks if
no shared storage is set up
- Addressed Michal's concerns
v3:
- Relying entirely on virFileIsSharedFS() on migration source and
destination sides to detect whether shared storage is set up between
hosts; no more hint about shared storage from user via flag
- Added support for virDomainTPMPrivate structure to store and persist
TPM-related private data
Stefan Berger (7):
util: Add parsing support for swtpm's cmdarg-migration capability
qemu: tpm: Allow offline migration with TPM_EMULATOR only with shared
storage
qemu: tpm: Conditionally create storage on incoming migration
qemu: tpm: Add support for storing private TPM-related data
qemu: tpm: Pass --migration option to swtpm if supported and needed
qemu: tpm: Avoid security labels on incoming migration with shared
storage
qemu: tpm: Never remove state on outgoing migration and shared storage
src/conf/domain_conf.c | 63 ++++++++++++++++++--
src/conf/domain_conf.h | 9 +++
src/qemu/qemu_domain.c | 85 ++++++++++++++++++++++++--
src/qemu/qemu_domain.h | 17 +++++-
src/qemu/qemu_driver.c | 20 +++----
src/qemu/qemu_extdevice.c | 10 ++--
src/qemu/qemu_extdevice.h | 6 +-
src/qemu/qemu_migration.c | 28 +++++++--
src/qemu/qemu_process.c | 9 ++-
src/qemu/qemu_snapshot.c | 4 +-
src/qemu/qemu_tpm.c | 122 ++++++++++++++++++++++++++++++++++----
src/qemu/qemu_tpm.h | 14 ++++-
src/util/virtpm.c | 1 +
src/util/virtpm.h | 1 +
14 files changed, 339 insertions(+), 50 deletions(-)
--
2.37.3
2 years, 4 months
[libvirt PATCH 0/3] qemu: A few qemuMigrationCookieParse cleanups
by Jiri Denemark
Jiri Denemark (3):
qemu: Reorder qemuMigrationCookieParse arguments
qemu: Replace priv with qemuCaps in qemuMigrationCookieParse
qemu: Reindent qemuMigrationCookieParse prototype arguments
src/qemu/qemu_migration.c | 35 ++++++++++++++++--------------
src/qemu/qemu_migration_cookie.c | 12 ++++------
src/qemu/qemu_migration_cookie.h | 14 ++++++------
tests/qemumigrationcookiexmltest.c | 6 ++---
4 files changed, 33 insertions(+), 34 deletions(-)
--
2.38.1
2 years, 4 months
[PATCH] node_device_conf: Avoid memleak in virNodeDeviceGetPCIVPDDynamicCap()
by Michal Privoznik
The virNodeDeviceGetPCIVPDDynamicCap() function is called from
virNodeDeviceGetPCIDynamicCaps() and therefore has to be a wee
bit more clever about adding VPD capability. Namely, it has to
remove the old one before adding a new one. This is how other
functions called from virNodeDeviceGetPCIDynamicCaps() behave
as well.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143235
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/node_device_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 83e66b85e3..af331baaf3 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -3068,6 +3068,9 @@ virNodeDeviceGetPCIVPDDynamicCap(virNodeDevCapPCIDev *devCapPCIDev)
virPCIDeviceAddress devAddr = { 0 };
g_autoptr(virPCIVPDResource) res = NULL;
+ g_clear_pointer(&devCapPCIDev->vpd, virPCIVPDResourceFree);
+ devCapPCIDev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VPD;
+
devAddr.domain = devCapPCIDev->domain;
devAddr.bus = devCapPCIDev->bus;
devAddr.slot = devCapPCIDev->slot;
@@ -3081,8 +3084,6 @@ virNodeDeviceGetPCIVPDDynamicCap(virNodeDevCapPCIDev *devCapPCIDev)
if ((res = virPCIDeviceGetVPD(pciDev))) {
devCapPCIDev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VPD;
devCapPCIDev->vpd = g_steal_pointer(&res);
- } else {
- virPCIVPDResourceFree(g_steal_pointer(&devCapPCIDev->vpd));
}
}
return 0;
--
2.37.4
2 years, 4 months
[libvirt PATCH] qemu: Pass vm to qemuMigrationCookieParse if it exists
by Jiri Denemark
The vm object is used inside qemuMigrationCookieParse based on the flags
passed to qemuMigrationCookieParse and the content of the cookie. The
callers should not just blindly guess and pass NULL if they
(incorrectly) think the vm object is not needed. We should always pass
the vm object unless it does not exist yet.
This fixes a bug when statistics of a completed migration reported
"Unknown" operation instead of "Incoming migration" on the destination
host.
https://bugzilla.redhat.com/show_bug.cgi?id=2137298
Fixes: v8.7.0-79-g0150f7a8c1
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index bef06f4caf..6d3810c79c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4775,7 +4775,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
QEMU_MIGRATION_COOKIE_GRAPHICS |
QEMU_MIGRATION_COOKIE_CAPS |
QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS,
- NULL);
+ vm);
if (!mig)
goto error;
@@ -6459,7 +6459,7 @@ qemuMigrationDstFinishOffline(virQEMUDriver *driver,
g_autoptr(qemuMigrationCookie) mig = NULL;
if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv,
- cookiein, cookieinlen, cookie_flags, NULL)))
+ cookiein, cookieinlen, cookie_flags, vm)))
return NULL;
if (qemuMigrationDstPersist(driver, vm, mig, false) < 0)
@@ -6655,7 +6655,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
vm, flags, retcode);
if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv,
- cookiein, cookieinlen, cookie_flags, NULL)))
+ cookiein, cookieinlen, cookie_flags, vm)))
goto error;
if (retcode != 0) {
--
2.38.1
2 years, 4 months
[PATCH] Fix couple of comment typos
by Martin Kletzander
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Pushed as trivial.
examples/c/admin/list_clients.c | 2 +-
examples/c/admin/logging.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/examples/c/admin/list_clients.c b/examples/c/admin/list_clients.c
index 70907b4fba2f..169c9a7ec814 100644
--- a/examples/c/admin/list_clients.c
+++ b/examples/c/admin/list_clients.c
@@ -53,7 +53,7 @@ int main(int argc, char **argv)
{
int ret = -1;
virAdmConnectPtr conn = NULL;
- virAdmServerPtr srv = NULL; /* which server list the clients from */
+ virAdmServerPtr srv = NULL; /* which server to list the clients from */
virAdmClientPtr *clients = NULL; /* where to store the servers */
ssize_t i = 0;
int count = 0;
diff --git a/examples/c/admin/logging.c b/examples/c/admin/logging.c
index 730ae40d9d53..575d15a3a67c 100644
--- a/examples/c/admin/logging.c
+++ b/examples/c/admin/logging.c
@@ -77,7 +77,7 @@ int main(int argc, char **argv)
goto cleanup;
}
- /* now, try to change the redefine the current log output and filters */
+ /* now, try to change the current log output and filters */
if (virAdmConnectSetLoggingOutputs(conn, set_outputs, 0) < 0)
goto cleanup;
--
2.38.1
2 years, 4 months
[PATCH] selinux: Reflect context_str() type change
by Michal Privoznik
As of [1]. libselinux changed the type of context_str() - it now
returns a const string. Follow this change in our code base.
1: https://github.com/SELinuxProject/selinux/commit/dd98fa322766760c4e1f029c...
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_selinux.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 1f72f61cac..92e85c92e0 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -526,7 +526,7 @@ static char *
virSecuritySELinuxContextAddRange(char *src,
char *dst)
{
- char *str = NULL;
+ const char *str = NULL;
char *ret = NULL;
context_t srccon = NULL;
context_t dstcon = NULL;
@@ -567,7 +567,7 @@ virSecuritySELinuxGenNewContext(const char *basecontext,
{
context_t context = NULL;
char *ret = NULL;
- char *str;
+ const char *str;
char *ourSecContext = NULL;
context_t ourContext = NULL;
--
2.37.4
2 years, 4 months
