July 2021 - Devel - libvirt List Archives

[PATCH v3 0/2] domstats:add haltpolling time statistic interface
by Yang Fei 20 Jul '21

20 Jul '21

This series add the ability to statistic the halt polling time when VM execute HLT(arm is WFI). v1: https://listman.redhat.com/archives/libvir-list/2021-July/msg00029.html v2: https://listman.redhat.com/archives/libvir-list/2021-July/msg00339.html changes from v1: - Move virGetCgroupValueRaw to utils.c and rename it virGetValueRaw. So that we can call it to obtain halt polling time. - Helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue are added in a separate patch - Use STRPREFIX to match the path prefix. - Fix the logic that domstats will break when platform is non-linux, debugfs isn't mounted and so on. change from v2: - Drop patch 1, use virFileReadValueUllong() to get halt polling data. - Delete unnecessary error report in logs. - Remove the qemuDomainGetStatsCpuHaltPollTime function conditionally compiled on linux. - Document the new parameters in src/libvirt-domain.c. Yang Fei (2): util: Add virGetCpuHaltPollTime qemu: Introduce qemuDomainGetStatsCpuHaltPollTime src/libvirt-domain.c | 7 +++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 20 +++++++++++++++++++ src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 5 files changed, 75 insertions(+) -- 2.23.0

3 9

[PATCH] meson: disable bogus warnings from sanitizers on Fedora
by Daniel P. Berrangé 20 Jul '21

20 Jul '21

When building with sanitizers on Fedora we get a wierd error message In file included from /usr/include/string.h:519, from ../src/internal.h:28, from ../src/util/virsocket.h:21, from ../src/util/virsocketaddr.h:21, from ../src/util/virnetdevip.h:21, from ../src/util/virnetdevip.c:21: In function ‘memcpy’, inlined from ‘virNetDevGetifaddrsAddress’ at ../src/util/virnetdevip.c:702:13, inlined from ‘virNetDevIPAddrGet’ at ../src/util/virnetdevip.c:754:16: /usr/include/bits/string_fortified.h:29:10: error: ‘__builtin_memcpy’ offset [2, 27] from the object at ‘addr’ is out of the bounds of referenced subobject ‘ss_family’ with type ‘short unsigned int’ at offset 0 [-Werror=array-bounds] 29 | return __builtin___memcpy_chk (__dest, __src, __len, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 30 | __glibc_objsize0 (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /usr/include/bits/socket.h:175, from /usr/include/sys/socket.h:33, from ../src/util/virsocket.h:66, from ../src/util/virsocketaddr.h:21, from ../src/util/virnetdevip.h:21, from ../src/util/virnetdevip.c:21: ../src/util/virnetdevip.c: In function ‘virNetDevIPAddrGet’: /usr/include/bits/socket.h:193:5: note: subobject ‘ss_family’ declared here 193 | __SOCKADDR_COMMON (ss_); /* Address family, etc. */ | ^~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors The code is correct, and this only happens when building at -O2. The docs for -Warray-bounds say that a value of "2" is known to be liable to generate false positives. Rather than downgrade the check everywhere, we do it selectively for sanitizers. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- meson.build | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index ca4291e37a..d4c142eebb 100644 --- a/meson.build +++ b/meson.build @@ -227,6 +227,11 @@ alloc_max = run_command( # sanitizer instrumentation may enlarge stack frames stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 8192 +# array_bounds=2 check triggers false positive on some GCC +# versions when using sanitizers. Seen on Fedora 34 with +# GCC 11.1.1 +array_bounds = get_option('b_sanitize') == 'none' ? 2 : 1 + cc_flags += [ '-fasynchronous-unwind-tables', '-fexceptions', @@ -238,7 +243,7 @@ cc_flags += [ '-Waggressive-loop-optimizations', '-Walloc-size-larger-than=@0@'.format(alloc_max.stdout().strip()), '-Walloca', - '-Warray-bounds=2', + '-Warray-bounds=@0@'.format(array_bounds), '-Wattribute-alias=2', '-Wattribute-warning', '-Wattributes', -- 2.31.1

2 1

[PATCH v1] virtqemud: remove sysconfig file
by Olaf Hering 19 Jul '21

19 Jul '21

sysconfig files are owned by the admin of the host. He has the liberty to put anything he wants into these files. This makes it difficult to provide different defaults. Remove the sysconfig file and place the current desired default into the service file. Local customizations can now go either into /etc/sysconfig/virtqemud or /etc/systemd/system/virtqemud.service.d/my-knobs.conf Signed-off-by: Olaf Hering <olaf(a)aepfle.de> --- Untested, just to demonstrate how it could be done for every sysconfig file. libvirt.spec.in | 1 - src/qemu/meson.build | 5 ----- src/qemu/virtqemud.service.in | 1 + src/qemu/virtqemud.sysconf | 12 ------------ 4 files changed, 1 insertion(+), 18 deletions(-) delete mode 100644 src/qemu/virtqemud.sysconf diff --git a/libvirt.spec.in b/libvirt.spec.in index cb48dd0be0..2cc3e61e3c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1689,7 +1689,6 @@ exit 0 %if %{with_qemu} %files daemon-driver-qemu -%config(noreplace) %{_sysconfdir}/sysconfig/virtqemud %config(noreplace) %{_sysconfdir}/libvirt/virtqemud.conf %{_datadir}/augeas/lenses/virtqemud.aug %{_datadir}/augeas/lenses/tests/test_virtqemud.aug diff --git a/src/qemu/meson.build b/src/qemu/meson.build index 3898d23877..6b4dd58309 100644 --- a/src/qemu/meson.build +++ b/src/qemu/meson.build @@ -165,11 +165,6 @@ if conf.has('WITH_QEMU') 'in_file': files('virtqemud.init.in'), } - sysconf_files += { - 'name': 'virtqemud', - 'file': files('virtqemud.sysconf'), - } - virt_install_dirs += [ localstatedir / 'lib' / 'libvirt' / 'qemu', runstatedir / 'libvirt' / 'qemu', diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in index 8abc9d3a7f..a5cb145668 100644 --- a/src/qemu/virtqemud.service.in +++ b/src/qemu/virtqemud.service.in @@ -18,6 +18,7 @@ Documentation=https://libvirt.org [Service] Type=notify +Environment=VIRTQEMUD_ARGS="--timeout 120" EnvironmentFile=-@sysconfdir@/sysconfig/virtqemud ExecStart=@sbindir@/virtqemud $VIRTQEMUD_ARGS ExecReload=/bin/kill -HUP $MAINPID diff --git a/src/qemu/virtqemud.sysconf b/src/qemu/virtqemud.sysconf deleted file mode 100644 index 87b626e3ed..0000000000 --- a/src/qemu/virtqemud.sysconf +++ /dev/null @@ -1,12 +0,0 @@ -# Customizations for the virtqemud.service systemd unit - -VIRTQEMUD_ARGS="--timeout 120" - -# Override the QEMU/SDL default audio driver probing when -# starting virtual machines using SDL graphics -# -# NB these have no effect for VMs using VNC, unless vnc_allow_host_audio -# is enabled in /etc/libvirt/qemu.conf -#QEMU_AUDIO_DRV=sdl -# -#SDL_AUDIODRIVER=pulse

4 10

[libvirt PATCH] qemu: Signal domain condition in qemuProcessStop a bit later
by Jiri Denemark 19 Jul '21

19 Jul '21

Signaling the condition before vm->def->id is reset to -1 is dangerous: in case a waiting thread wakes up, it does not see anything interesting (the domain is still marked as running) and just enters virDomainObjWait where it waits forever because the condition will never be signalled again. Originally it was impossible to get into such situation because the vm object was locked all the time between signaling the condition and resetting vm->def->id, but after commit 860a999802 released in 6.8.0, qemuDomainObjStopWorker called in qemuProcessStop between virDomainObjBroadcast and setting vm->def->id to -1 unlocks the vm object giving other threads a chance to wake up and possibly hang. In real world, this can be easily reproduced by killing, destroying, or just shutting down (from the guest OS) a domain while it is being migrated somewhere else. The migration job would never finish. We can't fix this by reseting vm->def->id earlier because other functions (such as qemuProcessKill) do nothing when the domain is already marked as inactive. So let's make sure we delay signaling the domain condition to the point when a woken up thread can detect the domain is not active anymore. https://bugzilla.redhat.com/show_bug.cgi?id=1949869 Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/qemu/qemu_process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c972c90801..914f936e45 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7852,9 +7852,6 @@ void qemuProcessStop(virQEMUDriver *driver, if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque); - /* Wake up anything waiting on domain condition */ - virDomainObjBroadcast(vm); - if ((timestamp = virTimeStringNow()) != NULL) { qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n", timestamp, @@ -7925,6 +7922,9 @@ void qemuProcessStop(virQEMUDriver *driver, vm->def->id = -1; + /* Wake up anything waiting on domain condition */ + virDomainObjBroadcast(vm); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); -- 2.32.0

2 2

[PATCH 13/13] docs/deprecated: deprecate passing plugin args through `arg=`
by Mahmoud Mandour 19 Jul '21

19 Jul '21

Signed-off-by: Mahmoud Mandour <ma.mandourr(a)gmail.com> --- docs/system/deprecated.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index e2e0090878..aaf0ee5777 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -126,6 +126,12 @@ other options have been processed. This will either have no effect (if if they were not given. The property is therefore useless and should not be specified. +Plugin argument passing through ``arg=<string>`` (since 6.1) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Passing arguments through ``arg=`` is redundant is makes the command-line less +readable, especially when the argument itself consist of a name and a value, +e.g. ``arg="arg_name=arg_value"``. Therefore, the usage of ``arg`` is redundant. QEMU Machine Protocol (QMP) commands ------------------------------------ -- 2.25.1

2 1

[PATCH 0/5] virsh: avoid local enum helpers with 'vir' prefix
by Peter Krempa 19 Jul '21

19 Jul '21

Follow up to: https://listman.redhat.com/archives/libvir-list/2021-July/msg00281.html Peter Krempa (5): virsh-host: Avoid 'vir' prefix for locally declared VIR_ENUM* helpers virsh-domain: Avoid 'vir' prefix for locally declared VIR_ENUM* helpers virsh-network: Avoid 'vir' prefix for locally declared VIR_ENUM* helpers virsh-volume: Avoid 'vir' prefix for locally declared VIR_ENUM* helpers syntax-check: Prohibit 'vir' prefix for enum implementations in virsh build-aux/syntax-check.mk | 7 +++++++ tools/virsh-completer-domain.c | 8 ++++---- tools/virsh-completer-host.c | 2 +- tools/virsh-domain.c | 18 +++++++++--------- tools/virsh-domain.h | 8 ++++---- tools/virsh-host.c | 4 ++-- tools/virsh-host.h | 2 +- tools/virsh-network.c | 12 ++++++------ tools/virsh-volume.c | 6 +++--- 9 files changed, 37 insertions(+), 30 deletions(-) -- 2.31.1

2 6

[libvirt PATCH v2 0/2] Remove macros "VIR_INSERT_ELEMENT_COPY" and "VIR_INSERT_ELEMENT_COPY_INPLACE"
by Tim Wiederhake 19 Jul '21

19 Jul '21

See https://listman.redhat.com/archives/libvir-list/2021-July/msg00163.html Changes since V1: * Fixed a mistake in the copy-out of virQEMUCapsGetMachineTypesCaps, where the array was allocated with the un-adjusted size. Tim Wiederhake (2): virQEMUCapsGetMachineTypesCaps: Use GPtrArray viralloc: Delete VIR_INSERT_ELEMENT_COPY and VIR_INSERT_ELEMENT_COPY_INPLACE src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++------------------ src/util/viralloc.h | 22 ++++----------------- 2 files changed, 22 insertions(+), 37 deletions(-) -- 2.31.1

2 3

[PATCH 0/2] Add support for 'id' attribute for 'cachetune' element
by Kristina Hanicova 19 Jul '21

19 Jul '21

Kristina Hanicova (2): docs: Allow 'id' attribute for 'cachetune' element tests: Modify to test output value of <cachetune id='' docs/formatdomain.rst | 2 +- docs/schemas/domaincommon.rng | 5 ++++ tests/genericxml2xmlindata/cachetune.xml | 8 +++--- tests/genericxml2xmloutdata/cachetune.xml | 34 ----------------------- tests/genericxml2xmltest.c | 2 +- 5 files changed, 11 insertions(+), 40 deletions(-) delete mode 100644 tests/genericxml2xmloutdata/cachetune.xml -- 2.31.1

2 4

[PATCH] test_driver: Implement virDomainGetControlInfo and add test
by Luke Yue 19 Jul '21

19 Jul '21

As test driver won't have real background job running, in order to get all possible states, the time is used here to decide which state to be returned. The default time will get `ok` as return value. Note that using `virsh domtime fc4 200` won't take effect for the test driver, to get other states, you have to enter virsh interactive terminal and set time. Signed-off-by: Luke Yue <lukedyue(a)gmail.com> --- src/test/test_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++++ tests/virshtest.c | 11 ++++++++ 2 files changed, 74 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ef0ddab54d..892dc978f2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2080,6 +2080,68 @@ testDomainGetState(virDomainPtr domain, return 0; } +static int +testDomainGetControlInfo(virDomainPtr dom, + virDomainControlInfoPtr info, + unsigned int flags) +{ + virDomainObj *vm; + testDomainObjPrivate *priv; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + priv = vm->privateData; + + memset(info, 0, sizeof(*info)); + + if (priv->seconds > 0 && priv->seconds < 10000) { + info->state = VIR_DOMAIN_CONTROL_JOB; + info->stateTime = priv->seconds; + } else if (priv->seconds < 30000 && priv->seconds >= 10000) { + info->state = VIR_DOMAIN_CONTROL_OCCUPIED; + info->stateTime = priv->seconds - 10000; + } else if (priv->seconds < 60000 && priv->seconds >= 30000) { + info->state = VIR_DOMAIN_CONTROL_ERROR; + switch (priv->seconds % 4) { + case 0: + info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_NONE; + break; + + case 1: + info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_UNKNOWN; + break; + + case 2: + info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_MONITOR; + break; + + case 3: + info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_INTERNAL; + break; + + default: + info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_NONE; + break; + } + info->stateTime = priv->seconds - 30000; + } else { + info->state = VIR_DOMAIN_CONTROL_OK; + } + + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + static int testDomainGetTime(virDomainPtr dom, long long *seconds, @@ -9335,6 +9397,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetHostname = testDomainGetHostname, /* 5.5.0 */ .domainGetInfo = testDomainGetInfo, /* 0.1.1 */ .domainGetState = testDomainGetState, /* 0.9.2 */ + .domainGetControlInfo = testDomainGetControlInfo, /* 7.6.0 */ .domainGetTime = testDomainGetTime, /* 5.4.0 */ .domainSetTime = testDomainSetTime, /* 5.7.0 */ .domainSave = testDomainSave, /* 0.3.2 */ diff --git a/tests/virshtest.c b/tests/virshtest.c index c1974c46cb..fe0c420958 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -250,6 +250,13 @@ static int testCompareDomstateByName(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, NULL, argv); } +static int testCompareDomControlInfoByName(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "domcontrol", "fc4", NULL }; + const char *exp = "ok\n\n"; + return testCompareOutputLit(exp, NULL, argv); +} + struct testInfo { const char *const *argv; const char *result; @@ -334,6 +341,10 @@ mymain(void) testCompareDomstateByName, NULL) != 0) ret = -1; + if (virTestRun("virsh domcontrol (by name)", + testCompareDomControlInfoByName, NULL) != 0) + ret = -1; + /* It's a bit awkward listing result before argument, but that's a * limitation of C99 vararg macros. */ # define DO_TEST(i, result, ...) \ -- 2.32.0

2 1

[libvirt PATCH 1/2] nodedev: fix parent device of inactive mdevs
by Jonathon Jongsma 19 Jul '21

19 Jul '21

Inactive mdevs were simply formatting their parent name as the value received from mdevctl rather than looking up the libvirt nodedev name of the parent device. This resulted in a parent value of e.g. '0000:5b:00.0' instead of 'pci_0000_5b_00_0'. This prevented defining a new mdev device from the output of nodedev-dumpxml. Unfortunately, it's not simple to fix this comprehensively due to the fact that mdevctl supports defining (inactive) mdevs for parent devices that do not actually exist on the host (yet). So for those persistent mdev definitions that do not have a valid parent in the device list, the parent device will be set to the root "computer" device. Unfortunately, because the value of the 'parent' field now depends on the configuration of the host, the mdevctl parsing test will output 'computer' for all test devices. Fixing this would require a more extensive mock test environment. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1979761 Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com> fixup --- src/node_device/node_device_driver.c | 20 ++++++++++++++++++- .../mdevctl-list-multiple.out.xml | 8 ++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b4dd57e5f4..26b0c2f032 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1051,6 +1051,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, virJSONValue *props; virJSONValue *attrs; g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); + g_autofree char *parent_sysfs_path = NULL; /* the child object should have a single key equal to its uuid. * The value is an object describing the properties of the mdev */ @@ -1060,7 +1061,24 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, uuid = virJSONValueObjectGetKey(json, 0); props = virJSONValueObjectGetValue(json, 0); - child->parent = g_strdup(parent); + /* Look up id of parent device. mdevctl supports defining mdevs for parent + * devices that are not present on the system (to support starting mdevs on + * hotplug, etc) so the parent may not actually exist. */ + parent_sysfs_path = g_strdup_printf("/sys/class/mdev_bus/%s", parent); + if (virFileExists(parent_sysfs_path)) { + g_autofree char *canon_syspath = realpath(parent_sysfs_path, NULL); + virNodeDeviceObj *parentobj = NULL; + if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + canon_syspath))) { + virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parentobj); + child->parent = g_strdup(parentdef->name); + virNodeDeviceObjEndAPI(&parentobj); + + child->parent_sysfs_path = g_steal_pointer(&canon_syspath); + } + } + if (!child->parent) + child->parent = g_strdup("computer"); child->caps = g_new0(virNodeDevCapsDef, 1); child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml index cf7e966256..eead6f2a40 100644 --- a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -1,6 +1,6 @@ <device> <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name> - <parent>0000:00:02.0</parent> + <parent>computer</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> <uuid>200f228a-c80a-4d50-bfb7-f5a0e4e34045</uuid> @@ -9,7 +9,7 @@ </device> <device> <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name> - <parent>0000:00:02.0</parent> + <parent>computer</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> <uuid>de807ffc-1923-4d5f-b6c9-b20ecebc6d4b</uuid> @@ -18,7 +18,7 @@ </device> <device> <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name> - <parent>0000:00:02.0</parent> + <parent>computer</parent> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> <uuid>435722ea-5f43-468a-874f-da34f1217f13</uuid> @@ -28,7 +28,7 @@ </device> <device> <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> - <parent>matrix</parent> + <parent>computer</parent> <capability type='mdev'> <type id='vfio_ap-passthrough'/> <uuid>783e6dbb-ea0e-411f-94e2-717eaad438bf</uuid> -- 2.31.1

4 10