March 2020 - Devel - libvirt List Archives

[PATCH 0/5] virCommand: fix approach to pidfiles
by Michal Privoznik 13 Mar '20

13 Mar '20

This was inspired by reviewing Marc-Andre's patchset [2]. I was wondering why the dbus-daemon is not being killed even though the corresponding function was called. The problem is, the dbus-daemon doesn't have the pidfile locked and therefore virPidFileForceCleanupPath() can't be used. This got me thinking, what is the pidfile good for if the daemon doesn't own it. Sure, we have virPidFileReadPathIfAlive() but that won't work if the daemon binary gets updated meanwhile. Michal Prívozník (5): virCommand: Actually acquire pidfile instead of just writing it qemuProcessStartManagedPRDaemon: Don't pass -f pidfile to the daemon qemuSlirpStop: Simplify helper kill qemuVirtioFSStop: Simplify daemon kill bridge_driver: Replace and drop networkKillDaemon docs/internals/command.html.in | 4 +- src/network/bridge_driver.c | 107 ++++++--------------------------- src/qemu/qemu_process.c | 9 --- src/qemu/qemu_slirp.c | 16 ++--- src/qemu/qemu_virtiofs.c | 21 +++---- src/util/vircommand.c | 56 ++++++++++++++--- tests/commanddata/test4.log | 1 + 7 files changed, 81 insertions(+), 133 deletions(-) -- 2.24.1

1 5

[libvirt] [PATCH] virt-host-validate: warn if kvm_hv is not loaded for POWER hosts
by Daniel Henrique Barboza 13 Mar '20

13 Mar '20

POWER hosts does not implement CPU virtualization extensions like x86 or s390x. Instead, all bare-metal POWER hosts are considered to be virtualization ready. For POWER, the validation is done by checking the virtualization kernel modules, kvm_hv and kvm_pr, to see if they are either not installed or not loaded in the host. If the KVM modules aren't present, we should not just warn but fail to validate. This patch implements this support. If kvm_hv is not installed, which can be determined by 'modinfo' returning not-zero return code, fail the verification. If kvm_hv is installed but not loaded, show a warning. The exception are POWER8 hosts, which can work with kvm_pr. In its case, ACK the use of kvm_pr if kvm_hv is not loaded/present. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com> --- tools/virt-host-validate-common.c | 136 ++++++++++++++++++++++++++++++ tools/virt-host-validate-common.h | 2 + tools/virt-host-validate-qemu.c | 6 ++ 3 files changed, 144 insertions(+) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index bce0f14917..e6d7986758 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -411,3 +411,139 @@ int virHostValidateIOMMU(const char *hvname, virHostMsgPass(); return 0; } + + +static bool virHostCPUIsPower8(void) +{ + FILE *fp; + bool ret = false; + + if (!(fp = fopen("/proc/cpuinfo", "r"))) + return false; + + do { + char line[1024]; + + if (!fgets(line, sizeof(line), fp)) + break; + + /* Looks for the 'model name' line. This is more common for + * Intel /proc/cpuinfo formats, but let's account for it + * too. */ + if (STRPREFIX(line, "model name")) { + if (strstr(line, "POWER8")) + ret = true; + break; + } + + /* Looks for the 'cpu:' line which is more commonly present + * in /proc/cpuinfo Power systems. To ensure this is not + * 'cpu id' or any other cpu attribute, peek at the next char + * after the first whitespace. A tab, whitespace or ':' + * indicates we're on the right line */ + if (STRPREFIX(line, "cpu") && + (line[3] == '\t' || line[3] == ':' || line[3] == ' ')) { + if (strstr(line, "POWER8")) + ret = true; + break; + } + + } while (1); + + VIR_FORCE_FCLOSE(fp); + + return ret; +} + + +static bool virHostKernelModuleExists(const char *module) +{ + g_autofree char *cmd = g_strdup_printf("modinfo %s", module); + g_autofree char *stdout = NULL; + g_autofree char *stderr = NULL; + g_autoptr(GError) err = NULL; + int errStatus; + + if (g_spawn_command_line_sync(cmd, &stdout, &stderr, &errStatus, &err)) + return true; + + return false; +} + + +static bool virHostKernelModuleIsLoaded(const char *module) +{ + FILE *fp; + bool ret = false; + + if (!(fp = fopen("/proc/modules", "r"))) + return false; + + do { + char line[1024]; + + if (!fgets(line, sizeof(line), fp)) + break; + + if (STRPREFIX(line, module)) { + ret = true; + break; + } + + } while (1); + + VIR_FORCE_FCLOSE(fp); + + return ret; +} + + +int virHostValidatePowerPCModules(void) +{ + bool kvm_pr_exists = virHostKernelModuleExists("kvm_pr"); + bool kvm_pr_loaded = kvm_pr_exists && virHostKernelModuleIsLoaded("kvm_pr"); + bool kvm_hv_exists = virHostKernelModuleExists("kvm_hv"); + bool kvm_hv_loaded = kvm_hv_exists && virHostKernelModuleIsLoaded("kvm_hv"); + bool hostIsP8 = virHostCPUIsPower8(); + + virHostMsgCheck("QEMU", "%s", _("for PowerPC KVM modules loaded")); + + /* No Power KVM virtualization modules present on the host. */ + if (!kvm_hv_exists && !kvm_pr_exists) { + virHostMsgFail(VIR_HOST_VALIDATE_FAIL, + _("No kvm_hv or kvm_pr module present in " + "the host")); + return -1; + } + + /* Bail out for all non-Power8 CPUs if kvm_hv is not present. */ + if (!kvm_hv_exists && !hostIsP8) { + virHostMsgFail(VIR_HOST_VALIDATE_FAIL, + _("No kvm_hv module present in the host")); + return -1; + } + + /* Power8 CPUs virtualization works with any of kvm_hv and kvm_pr. + * Issue a warning if none are loaded. */ + if (hostIsP8) { + if (!kvm_hv_loaded && !kvm_pr_loaded) { + virHostMsgFail(VIR_HOST_VALIDATE_WARN, + _("Load kvm_hv or kvm_pr module " + "for better performance")); + return 0; + } + + virHostMsgPass(); + return 0; + } + + /* For non-Power8 hosts, show a warning if kvm_hv is not loaded. */ + if (!kvm_hv_loaded) { + virHostMsgFail(VIR_HOST_VALIDATE_WARN, + _("Load kvm_hv for better performance")); + return 0; + } + + virHostMsgPass(); + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 1b7e93e520..7a2933c8fd 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -83,3 +83,5 @@ int virHostValidateCGroupControllers(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level); + +int virHostValidatePowerPCModules(void); \ No newline at end of file diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index ff3c1f0231..8753c6a31d 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -57,6 +57,12 @@ int virHostValidateQEMU(void) if (virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SIE)) hasHwVirt = true; break; + case VIR_ARCH_PPC64: + case VIR_ARCH_PPC64LE: + hasVirtFlag = true; + if (virHostValidatePowerPCModules() == 0) + hasHwVirt = true; + break; default: hasHwVirt = false; } -- 2.23.0

2 3

[PATCH] news: Mention regression in virDomainBlockCopy with shallow/reuse flags
by Peter Krempa 13 Mar '20

13 Mar '20

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com> --- docs/news.xml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 7fd88f9998..92103ec308 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -48,6 +48,23 @@ <section title="Improvements"> </section> <section title="Bug fixes"> + <change> + <summary> + qemu: Open backing chain late for shallow block copy reusing external images + </summary> + <description> + With introduction of -blockdev specification of storage for qemu VMs + in libvirt-5.10 we've started opening the backing chain of the + destination/mirror of a virDomainBlockcopy started with + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT | VIR_DOMAIN_BLOCK_COPY_SHALLOW flags + when starting the job rather than when virDomainBlockJobAbort with + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT is issued. For users depending on + this undocumented quirk this caused a regression in behaviour as + the backing chain could not be modified while the copy of the top + image was progressing. Note that this fix also requires qemu-5.0 while + -blockdev is used starting from qemu-4.2. + </description> + </change> </section> </release> <release version="v6.1.0" date="2020-03-03"> -- 2.24.1

2 1

[PATCH 0/5] qemu: Allow late opening of backing chain on a shallow block-copy
by Peter Krempa 13 Mar '20

13 Mar '20

See patch 5/5 for explanation. As described in patch 1/5 this is based on patches which are not yet pushed in qemu. Peter Krempa (5): qemu: capabilities: Update qemu-5.0.0 capabilities for x86_64 qemuDomainBlockPivot: Move check prior to executing the pivot steps qemuDomainBlockCopyCommon: Record updated flags to block job qemu: capabilities: Introduce QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY qemu: blockcopy: Allow late opening of the backing chain of a shallow copy src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 75 +- .../caps_5.0.0.x86_64.replies | 2668 +++++++++-------- .../caps_5.0.0.x86_64.xml | 3 +- 5 files changed, 1435 insertions(+), 1314 deletions(-) -- 2.24.1

3 14

[libvirt PATCH] docs: Fix blockCopy's documentation on --pivot & --abort flags
by Kashyap Chamarthy 13 Mar '20

13 Mar '20

The --abort and --pivot flags for ``blockjob`` are mutually exclusive — you either abort and keep the destination copy as a point-in-time "snapshot"; or pivot to the new copy. But you cannot specify both flags at once. Rephrase blockCopy's doc fragment to that effect. Signed-off-by: Kashyap Chamarthy <kchamart(a)redhat.com> --- docs/manpages/virsh.rst | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index dc404ddfe8..e7d4933c93 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1254,18 +1254,19 @@ phases. Initially, the job must copy all data from the source, and during this phase, the job can only be canceled to revert back to the source disk, with no guarantees about the destination. After this phase completes, both the source and the destination remain mirrored until a -call to ``blockjob`` with the *--abort* and *--pivot* flags pivots over -to the copy, or a call without *--pivot* leaves the destination as a -faithful copy of that point in time. However, if *--wait* is specified, -then this command will block until the mirroring phase begins, or cancel -the operation if the optional *timeout* in seconds elapses or SIGINT is -sent (usually with ``Ctrl-C``). Using *--verbose* along with *--wait* -will produce periodic status updates. Using *--pivot* (similar to -``blockjob`` *--pivot*) or *--finish* (similar to ``blockjob`` *--abort*) -implies *--wait*, and will additionally end the job cleanly rather than -leaving things in the mirroring phase. If job cancellation is triggered -by timeout or by *--finish*, *--async* will return control to the user -as fast as possible, otherwise the command may continue to block a little +call is issued to ``blockjob`` with either the *--abort* flag, which +leaves the destination as a faithful copy of that point in time; or the +*--pivot* flag, which pivots over to the copy on the destination. +However, if *--wait* is specified, then this command will block until +the mirroring phase begins, or cancel the operation if the optional +*--timeout* in seconds elapses or SIGINT is sent (usually with +``Ctrl-C``). Using *--verbose* along with *--wait* will produce +periodic status updates. Using *--pivot* (similar to ``blockjob`` +*--pivot*) or *--finish* (similar to ``blockjob`` *--abort*) implies +*--wait*, and will additionally end the job cleanly rather than leaving +things in the mirroring phase. If job cancellation is triggered by +timeout or by *--finish*, *--async* will return control to the user as +fast as possible, otherwise the command may continue to block a little while longer until the job has actually cancelled. *path* specifies fully-qualified path of the disk. -- 2.21.0

1 0

[PATCH] apparmor: qemu load old shared objects
by Christian Ehrhardt 13 Mar '20

13 Mar '20

On qemu upgrades the old .so files usually are replaced. But on the other hand since a qemu process represents a guest instance it is usually kept around. That makes late addition of dynamic features e.g. 'hot-attach of a ceph disk' fail by trying to load a new version of e.f. block-rbd.so into an old still running qemu binary. Qemu adds a fallback to also load modules from a versioned directory in the temporary /var/run path. That way qemu is providing a way for packaging to store modules of an upgraded qemu package as needed until the next reboot. This change is allowing the qemu process access to these paths. Background: This is a continuation of a discussion at KVM Forum 2019 eventually becoming [1] and recently this change is queued to get into qemu properly [2]. [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg00005.html [2]: https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg03313.html Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com> --- src/security/apparmor/libvirt-qemu | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 80986aec61..22415c9dfd 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -164,6 +164,11 @@ /usr/{lib,lib64}/qemu/*.so mr, /usr/lib/(a){multiarch}/qemu/*.so mr, + # let qemu load old shared objects after upgrades (LP: #1847361) + /{var/,}run/qemu/*/*.so mr, + # but explicitly deny with auditing writing to these files + audit deny /{var/,}run/qemu/*/*.so w, + # swtpm /{usr/,}bin/swtpm rmix, /usr/{lib,lib64}/libswtpm_libtpms.so mr, -- 2.25.1

2 1

[PATCH v2] conf: Don't generate machine names with a dot
by Michal Privoznik 13 Mar '20

13 Mar '20

According to the linked BZ, machined expects either valid hostname or valid FQDN (see systemd commit v239-3092-gd65652f1f2). While in case of multiple dots, a trailing one doesn't violate FQDN, it does violate the rule in case of something simple, like "domain.". But it's safe to remove it in both cases. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1808499 Fixes: 45464db8ba502764cf37ec9335770248bdb3d9a8 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- v2 of: https://www.redhat.com/archives/libvir-list/2020-February/msg01138.html diff to v1: - Adjusted commit message as suggested by Jano - Fixed ".-" and "-." occurrences too, again suggested by Jano src/conf/domain_conf.c | 6 +++--- tests/virsystemdtest.c | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2d97daf80..246a78d39b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30933,7 +30933,7 @@ virDomainMachineNameAppendValid(virBufferPtr buf, if (strlen(virBufferCurrentContent(buf)) >= 64) break; - if (*name == '.') { + if (*name == '.' || *name == '-') { if (!skip_dot) virBufferAddChar(buf, *name); skip_dot = true; @@ -30948,8 +30948,8 @@ virDomainMachineNameAppendValid(virBufferPtr buf, virBufferAddChar(buf, *name); } - /* trailing dashes are not allowed */ - virBufferTrimChars(buf, "-"); + /* trailing dashes or dots are not allowed */ + virBufferTrimChars(buf, "-."); } #undef HOSTNAME_CHARS diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 1e36298189..f48a714cca 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -746,7 +746,8 @@ mymain(void) TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec-acdc-56b3f8c5f678)", 100, "qemu-100-kstest-network-device-default-httpksc9eed63e-981e-48ec"); TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec--cdc-56b3f8c5f678)", 10, - "qemu-10-kstest-network-device-default-httpksc9eed63e-981e-48ec"); + "qemu-10-kstest-network-device-default-httpksc9eed63e-981e-48ec-c"); + TEST_MACHINE("demo.-.test.", 11, "qemu-11-demo.test"); # define TESTS_PM_SUPPORT_HELPER(name, function) \ do { \ -- 2.24.1

2 1

[PATCH 0/2] qemu: fix crash in qemuDomainGetGuestInfo
by Peter Krempa 13 Mar '20

13 Mar '20

See patch 2/2 Peter Krempa (2): qemuAgentFSInfoFormatParams: Remove pointless returned value qemuDomainGetGuestInfo: Don't try to free a negative number of entries src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++---------------------- 2 files changed, 21 insertions(+), 24 deletions(-) -- 2.24.1

3 6

[libvirt PATCH 0/4] Use g_strerror instead of virStrerror (glib chronicles)
by Ján Tomko 13 Mar '20

13 Mar '20

2 5

[libvirt PATCH] tests: fix double-lock of monitor in hotplug test
by Daniel P. Berrangé 13 Mar '20

13 Mar '20

The qemuMonitorTestNew() function returns with the monitor object locked, and expects it to still be locked when qemuMonitorTestFree is called. The qemuhotplug test, however, explicitly unlocks the monitor, but then forgets to lock it again. As a result the qemuMonitorTestFree function is unlocking a mutex it doesn't own. This bug has existed forever, but since we use normal POSIX mutexes and don't check the return value of pthread_mutex_lock/unlock we didn't see the error. It was harmless until the switch to the per monitor event loop which requires the thread synchronization to work reliably, whereupon it started crashing. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- tests/qemuhotplugtest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8b411d63f0..d9244dca44 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -337,6 +337,8 @@ testQemuHotplug(const void *data) ret = testQemuHotplugUpdate(vm, dev); } + virObjectLock(priv->mon); + cleanup: VIR_FREE(domain_filename); VIR_FREE(device_filename); @@ -378,6 +380,7 @@ static void testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) { qemuDomainObjPrivatePtr priv; + qemuMonitorPtr mon; if (!data) return; @@ -396,6 +399,8 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) virObjectUnref(data->vm); } + mon = qemuMonitorTestGetMonitor(data->mon); + virObjectLock(mon); qemuMonitorTestFree(data->mon); VIR_FREE(data); } -- 2.24.1

2 1