- Devel - libvirt List Archives

[PATCH 0/2] virsh: Two simple long version output fixes
by Michal Privoznik 24 Nov '22

24 Nov '22

*** BLURB HERE *** Michal Prívozník (2): virsh: Format -V output properly virsh: Report iSCSI-direct backend in long version tools/virsh.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.37.4

2 3

Add option to skip cpu feature and model verification in libvirt and rely on qemu 'enforce' option
by manish.mishra 24 Nov '22

24 Nov '22

Hi Everyone, We had a requirement to skip all CPU feature validations from libvirt and totally rely on Qemu for following reasons. * As libvirt always lies behind in term of cpu feature management compare to qemu, so we may not be able to use all of the latest support provided by qemu till it is updated in libvirt too. For example like if new cpu features are added in qemu but not yet updated in libvirt. * Libvirt does not support cpu model versioning as Qemu. * Libvirt checks if a features is supported or not based on cpuid, a feature supported by host may not be supported by qemu or there can be some features which are emulated by Qemu so libvirt level checking does not make sense in those cases. * Qemu already provide an option 'enforce' to validate if features with which vm is started is exactly same as one provided and nothing is silently dropped. Basically we want another check option like 'qemu-enforce' which will skip all feature and cpu model verfications in libvirt and pass 'enforce' option to qemu. It will work similar to check 'none' i mean no check to verify if provide features are supported by host, also on top of that skip things like virCPUValidateFeatures where ever required. I understand this is specific to just Qemu and libvirt has to take care for many other emulator, so wanted to check if something like that makes sense as it will be very useful for Qemu atleast. If so we can send a patch. Thanks Manish Mishra

4 31

TX/RX statistics need to be swapped
by Oleg Vasilev 24 Nov '22

24 Nov '22

Hi, virDomainNetTypeSharesHostView[1] contains the logic where for some interface types it swaps TX and RX values, because what TX from the inside is RX on the outside and vice versa. We observe a configuration where <interface type='ethernet' /> shows swapped values in statistics, because, apparently, the values are swapped once inside the kernel when going from the inside to the outside, but not swapped second time because of the mentioned logic. Is this a bug? Why in the virDomainNetTypeSharesHostView[1] ethernet is marked as 'non-swapped'? Maybe we need some more fine-grained way of determining whether to swap value a second time? Thanks, Oleg [1]: https://gitlab.com/libvirt/libvirt/-/blob/master/src/conf/domain_conf.c#L29…

2 2

[libvirt PATCH] spec: libvirt-daemon: Add optional dependency on *-client
by Jiri Denemark 24 Nov '22

24 Nov '22

The libvirt-daemon subpackage contains libvirt-guests.sh script (used by libvirt-guests service), which requires virsh to actually work. But since dynamic libraries were separated from libvirt-client to libvirt-libs more than 6 years ago, libvirt-daemon no longer requires virsh to be installed. So unless libvirt-client is explicitly installed (either manually or by installing the libvirt meta package), libvirt-guests will not work. Just adding libvirt-client as a dependency of libvirt-daemon would go against the original idea behind splitting libvirt-client: users may not want to install or use any client binaries on the host where the daemon runs (either they just use various language bindings or access the daemon remotely). To solve this we could possibly turn libvirt-daemon into an empty package and separate the daemons and libvirt-guests into subpackages to make sure we support both use cases, but marking libvirt-client as Recommended for libvirt-daemon does the same job in a much simpler way. https://bugzilla.redhat.com/show_bug.cgi?id=2136591 Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- libvirt.spec.in | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libvirt.spec.in b/libvirt.spec.in index 0bbcdb8956..450f50f5b5 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -441,6 +441,12 @@ Summary: Server side daemon and supporting files for libvirt library # The client side, i.e. shared libs are in a subpackage Requires: %{name}-libs = %{version}-%{release} +# The libvirt-guests.sh script requires virsh from libvirt-client subpackage, +# but not every deployment wants to use libvirt-guests service. Using +# Recommends here will install libvirt-client by default (if available), but +# RPM won't complain if the package is unavailable, masked, or removed later. +Recommends: %{name}-client = %{version}-%{release} + # netcat is needed on the server side so that clients that have # libvirt < 6.9.0 can connect, but newer versions will prefer # virt-ssh-helper. Making this a Recommends means that it gets -- 2.38.1

4 6

questions regarding modular daemons
by Jim Fehlig 24 Nov '22

24 Nov '22

Hi All, I've procrastinated long enough and finally decided to switch to modular daemons in openSUSE Factory libvirt package. For the most part, the Factory spec file mimics the upstream one. I enabled with_modular_daemons and would like to confirm the results of my testing. When upgrading an existing installation running the monolithic daemon, the monolithic daemon is still enabled after the upgrade. I suppose this behavior is expected, and fine IMO. However, when installing the "modularized" packages on a system with no prior installation, the monolithic daemon is still installed and potentially enabled in posttrans. Having both the monolithic and modular daemons installed, along with all associated systemd socket and service files, is a bit confusing to users IMO. E.g. should one enable libvirtd sockets, virtqemud sockets, both? Is the intention, over time, to remove the monolithic daemon? Perhaps we could start by isolating the monolithic daemon in the libvirt-daemon subpackage and moving the other daemons (virtproxyd, virtlogd, virtlockd, etc) to a new subpackage? The modular daemons could then drop the libvirt-daemon dependency, allowing installation without the monolithic daemon. TIA for your comments. Regards, Jim

2 2

[PATCH] rpc: Mark close callback (un-)register as high priority
by Michal Privoznik 23 Nov '22

23 Nov '22

Our RPC calls can be divided into two groups: regular and high priority. The latter can be then processed by so called high priority worker threads. This is our way of defeating a 'deadlock' and allowing some RPCs to be processed even when all (regular) worker threads are stuck. For instance: if all regular worker threads get stuck when talking to QEMU on monitor, the virDomainDestroy() can be processed by a high priority worker thread(s) and thus unstuck those threads. Now, this is all fine, except if users want to use virsh non interactively: virsh destroy $dom This does a bit more - it needs to open a connection. And that consists of multiple RPC calls: AUTH_LIST, CONNECT_SUPPORTS_FEATURE, CONNECT_OPEN, and finally CONNECT_REGISTER_CLOSE_CALLBACK. All of them are marked as high priority except the last one. Therefore, virsh just sits there with a partially open connection. There's one requirement for high priority calls though: they can not get stuck. Hopefully, the reason is obvious by now. And looking into the server side implementation the CONNECT_REGISTER_CLOSE_CALLBACK processing can't ever get stuck. The only driver that implements the callback for public API is Parallels (vz). And that can't block really. And for virConnectUnregisterCloseCallback() it's the same story. Therefore, both can be marked as high priority. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143840 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/remote/remote_protocol.x | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 79ffc63f03..7dfb4548f4 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -6416,12 +6416,14 @@ enum remote_procedure { /** * @generate: none + * @priority: high * @acl: connect:getattr */ REMOTE_PROC_CONNECT_REGISTER_CLOSE_CALLBACK = 360, /** * @generate: none + * @priority: high * @acl: connect:getattr */ REMOTE_PROC_CONNECT_UNREGISTER_CLOSE_CALLBACK = 361, -- 2.37.4

2 1

[libvirt PATCH v2 1/1] qemu: fix vcpu clearing when multiple vcpu hotunplugs timeout
by Shaleen Bathla 22 Nov '22

22 Nov '22

Problem: libvirt has a 5 second timeout (generally) for hotplug/unplug operations which can time out due to heavy load in guest. vcpu hotunplug occurs one vcpu at a time. But, if we perform hotplug-unplug repeatedly, Case 1: qemu sends multiple timedout vcpu unplug notification before libvirt processed first one. Case 2: when attempting a hotplug, qemu finishes unplug of another cpu libvirt waits for an async event notification from qemu regarding successful vcpu delete. qemu deletes its vcpu, vcpuinfo and sends notification to libvirt. libvirt handles vcpu delete notification, and updates vcpuinfo received from qemu in qemuDomainRefreshVcpuInfo(). qemu's vcpuinfo during refresh will not contain other deleted, sent vcpu qemu's vcpuinfo will overwrite libvirt's vcpuinfo of the other pending timedout vcpu(s) which qemu has deleted and notified libvirt. The overwrite resets other timedout vcpu's alias to NULL and tid to 0. The error is then seen when validating tid of vcpus. Example error log: "internal error: qemu didn't report thread id for vcpu 'XX'" Solution: Clear vcpu alias of only the vcpu that hotplug API calls. Other vcpu-alias won't get reset when vcpuinfo refresh occurs. Validation is also done for corresponding vcpus only, not all. Co-authored-by: Partha Satapathy <partha.satapathy(a)oracle.com> Signed-off-by: Shaleen Bathla <shaleen.bathla(a)oracle.com> --- src/qemu/qemu_domain.c | 6 ++++-- src/qemu/qemu_hotplug.c | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3435da5bdc4c..6ae842d0e32f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9773,8 +9773,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 da92ced2f444..f11b90a220ac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6090,6 +6090,8 @@ qemuDomainRemoveVcpu(virDomainObj *vm, unsigned int nvcpus = vcpupriv->vcpus; virErrorPtr save_error = NULL; size_t i; + bool hasVcpuPids = qemuDomainHasVcpuPids(vm); + bool rollback = false; if (qemuDomainRefreshVcpuInfo(vm, VIR_ASYNC_JOB_NONE, false) < 0) return -1; @@ -6097,14 +6099,26 @@ qemuDomainRemoveVcpu(virDomainObj *vm, /* 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 = false; + VIR_FREE(vcpupriv->alias); /* clear vcpu alias of only this vcpu */ + + if (vcpupriv->tid != 0) { + if (hasVcpuPids) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu reported thread id for inactive vcpu '%zu'"), i); + + rollback = true; + break; + } } - if (qemuDomainValidateVcpuInfo(vm) < 0) { + if (rollback) { /* rollback vcpu count if the setting has failed */ virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", false); - for (i = vcpu; i < vcpu + nvcpus; i++) { + for (; i >= vcpu; i--) { vcpuinfo = virDomainDefGetVcpu(vm->def, i); vcpuinfo->online = true; } @@ -6141,6 +6155,9 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm, return; } } + + VIR_DEBUG("%s not found in vcpulist of domain %s ", + alias, vm->def->name); } @@ -6209,6 +6226,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver, int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); size_t i; + bool hasVcpuPids = qemuDomainHasVcpuPids(vm); if (!qemuDomainSupportsNewVcpuHotplug(vm)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -6245,14 +6263,19 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver, 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 { + if (hasVcpuPids) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu didn't report thread id for vcpu '%zu'"), i); + return -1; + } + } } - if (qemuDomainValidateVcpuInfo(vm) < 0) - return -1; - qemuDomainVcpuPersistOrder(vm->def); if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) -- 2.31.1 V1 Patch : https://listman.redhat.com/archives/libvir-list/2022-November/235470.html V1 Patch Review comments : https://listman.redhat.com/archives/libvir-list/2022-November/235534.html Changes in V2 since V1: - Patch addresses review comments from Peter Krempa. - Validation is done for per cpu hotplug entity instead of all

2 2

[PATCH 0/5] Avoid 'unsigned long flags'
by Peter Krempa 22 Nov '22

22 Nov '22

Peter Krempa (5): internal: Refuse values exceeding range of 'unsigned int' in virCheckFlags virsh: vol-create-as: Use 'unsigned int' for flags qemu: processGuestPanicEvent: Use 'unsigned int' for flags qemu: migration: Use 'unsigned int' for flags libxl: migration: Use 'unsigned int' for flags src/internal.h | 22 +++++++-- src/libxl/libxl_migration.c | 4 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_migration.c | 82 ++++++++++++++++---------------- src/qemu/qemu_migration.h | 10 ++-- src/qemu/qemu_migration_params.c | 6 +-- src/qemu/qemu_migration_params.h | 4 +- tools/virsh-volume.c | 2 +- 8 files changed, 73 insertions(+), 59 deletions(-) -- 2.37.3

2 6

[PATCH] qemu_tpm: Check for qemuTPMSetupEncryption() errors
by Michal Privoznik 22 Nov '22

22 Nov '22

Inside of qemuTPMEmulatorBuildCommand() there are two calls to qemuTPMSetupEncryption() which simply ignore returned error. This is suboptimal because then we rely on swtpm binary reporting a generic error (something among invalid arguments) while an error reported by qemuTPMSetupEncryption() is more specific. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_tpm.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 15ee7db757..502a77b5b3 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -635,7 +635,12 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, } pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd); + if (pwdfile_fd < 0) + goto error; + migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd); + if (migpwdfile_fd < 0) + goto error; virCommandAddArg(cmd, "--key"); virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", pwdfile_fd); -- 2.37.4

2 1

[PATCH] apparmor: allow getattr on usb devices
by christian.ehrhardt＠canonical.com 22 Nov '22

22 Nov '22

From: Christian Ehrhardt <christian.ehrhardt(a)canonical.com> For the handling of usb we already allow plenty of read access, but so far /sys/bus/usb/devices only needed read access to the directory to enumerate the symlinks in there that point to the actual entries via relative links to ../../../devices/. But in more recent systemd with updated libraries a program might do getattr calls on those symlinks. And while symlinks in apparmor usually do not matter, as it is the effective target of an access that has to be allowed, here the getattr calls are on the links themselves. On USB hostdev usage that causes a set of denials like: apparmor="DENIED" operation="getattr" class="file" name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86" requested_mask="r" denied_mask="r" ... It is safe to read the links, therefore add a rule to allow it to the block of rules that covers the usb related access. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 02ee273e7e..d0289b8943 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -42,6 +42,7 @@ # For hostdev access. The actual devices will be added dynamically /sys/bus/usb/devices/ r, + /sys/bus/usb/devices/* r, /sys/devices/**/usb[0-9]*/** r, # libusb needs udev data about usb devices (~equal to content of lsusb -v) /run/udev/data/+usb* r, -- 2.38.1

3 4