November 2022 - Devel - libvirt List Archives

[PATCH v3] qemu: Don't report spurious errors from vCPU tid validation on hotunplug timeout
by Peter Krempa 05 Dec '22

05 Dec '22

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

3 3

[PATCH 0/9] Couple of virCommand related improvements
by Michal Privoznik 30 Nov '22

30 Nov '22

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 10

[libvirt PATCH] apparmor: Allow running /usr/libexec/qemu-kvm
by Andrea Bolognani 30 Nov '22

30 Nov '22

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

4 12

[PATCH v4 0/7] qemu: tpm: Add support for migration across shared storage
by Stefan Berger 30 Nov '22

30 Nov '22

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 18

[libvirt PATCH 0/3] qemu: A few qemuMigrationCookieParse cleanups
by Jiri Denemark 30 Nov '22

30 Nov '22

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 4

[PATCH] node_device_conf: Avoid memleak in virNodeDeviceGetPCIVPDDynamicCap()
by Michal Privoznik 30 Nov '22

30 Nov '22

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 1

[libvirt PATCH] qemu: Pass vm to qemuMigrationCookieParse if it exists
by Jiri Denemark 30 Nov '22

30 Nov '22

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 1

[PATCH 0/2] logging: add log cleanup for obsolete domains
by Oleg Vasilev 30 Nov '22

30 Nov '22

Motivation comes from the case when libguestfs has repeatedly created transient VMs, which in turn created lots of logs without any mechanism to delete it afterwards. More motivation in [1]. Previous attempt in [2]. [1]: https://listman.redhat.com/archives/libvir-list/2022-February/msg00425.html [2]: https://listman.redhat.com/archives/libvir-list/2022-February/msg00865.html Oleg Vasilev (2): logging: refactor to store config inside log handler logging: add log cleanup for obsolete domains src/logging/log_daemon.c | 6 +- src/logging/log_daemon_config.c | 9 ++ src/logging/log_daemon_config.h | 3 + src/logging/log_handler.c | 137 +++++++++++++++++++++++++++---- src/logging/log_handler.h | 17 ++-- src/logging/test_virtlogd.aug.in | 2 + src/logging/virtlogd.aug | 2 + src/logging/virtlogd.conf | 7 ++ 8 files changed, 153 insertions(+), 30 deletions(-) -- 2.38.1

2 5

[PATCH] Fix couple of comment typos
by Martin Kletzander 30 Nov '22

30 Nov '22

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

1 0

[PATCH] selinux: Reflect context_str() type change
by Michal Privoznik 29 Nov '22

29 Nov '22

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/dd98fa322766760c4e1f029cf1… 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 1