February 2021 - Devel - libvirt List Archives

[PATCH] qemu_monitor: Document qemuMonitorUnregister()
by Michal Privoznik 24 Feb '21

24 Feb '21

The most important bit is that the caller is expected to pass locked monitor. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_monitor.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ed35da17e1..73f337a6be 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -850,6 +850,13 @@ qemuMonitorRegister(qemuMonitorPtr mon) } +/** + * qemuMonitorUnregister: + * @mon: monitor object + * + * Unregister monitor from the event loop. The monitor object + * must be locked before calling this function. + */ void qemuMonitorUnregister(qemuMonitorPtr mon) { -- 2.26.2

2 1

[PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes
by Eric Blake 24 Feb '21

24 Feb '21

The value '1.1k' is inexact; 1126.4 bytes is not possible, so we happen to truncate it to 1126. Our use of fractional sizes is intended for convenience, but when a user specifies a fraction that is not a clean translation to binary, truncating/rounding behind their backs can cause confusion. Better is to deprecate inexact values, which still leaves '1.5k' as valid, but alerts the user to spell out their values as a precise byte number in cases where they are currently being rounded. Note that values like '0.1G' in the testsuite need adjustment as a result. Since qemu_strtosz() does not have an Err** parameter, and plumbing that in would be a much larger task, we instead go with just directly emitting the deprecation warning to stderr. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- I'm not a fan of this patch, but am proposing it for discussion purposes. --- docs/system/deprecated.rst | 9 +++++++++ tests/test-cutils.c | 6 +++--- tests/test-keyval.c | 4 ++-- tests/test-qemu-opts.c | 4 ++-- util/cutils.c | 9 +++++++-- 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 113c2e933f1b..2c9cb849eec5 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix the value is hexadecimal. That is, '0x20M' is deprecated, and should be written either as '32M' or as '0x2000000'. +inexact sizes via scaled fractions (since 6.0) +'''''''''''''''''''''''''''''''''''''''''''''' + +Input parameters that take a size value should only use a fractional +size (such as '1.5M') that will result in an exact byte value. The +use of inexact values (such as '1.1M') that require truncation or +rounding is deprecated, and you should instead consider writing your +unusual size in bytes (here, '1153433' or '1153434' as desired). + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/tests/test-cutils.c b/tests/test-cutils.c index bad3a6099389..c6c33866277b 100644 --- a/tests/test-cutils.c +++ b/tests/test-cutils.c @@ -2124,11 +2124,11 @@ static void test_qemu_strtosz_float(void) g_assert_cmpint(res, ==, 1024); g_assert(endptr == str + 3); - /* For convenience, we permit values that are not byte-exact */ - str = "12.345M"; + /* Fractional values should still be byte-exact */ + str = "12.125M"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB)); + g_assert_cmpint(res, ==, (uint64_t) (12.125 * MiB)); g_assert(endptr == str + 7); } diff --git a/tests/test-keyval.c b/tests/test-keyval.c index e20c07cf3ea5..7a45c22942e6 100644 --- a/tests/test-keyval.c +++ b/tests/test-keyval.c @@ -525,7 +525,7 @@ static void test_keyval_visit_size(void) visit_free(v); /* Suffixes */ - qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T", + qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.125G,sz5=16777215T", NULL, NULL, &error_abort); v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); qobject_unref(qdict); @@ -537,7 +537,7 @@ static void test_keyval_visit_size(void) visit_type_size(v, "sz3", &sz, &error_abort); g_assert_cmphex(sz, ==, 2 * MiB); visit_type_size(v, "sz4", &sz, &error_abort); - g_assert_cmphex(sz, ==, GiB / 10); + g_assert_cmphex(sz, ==, GiB / 8); visit_type_size(v, "sz5", &sz, &error_abort); g_assert_cmphex(sz, ==, 16777215ULL * TiB); visit_check_struct(v, &error_abort); diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 6568e31a7229..549e994938fe 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -720,10 +720,10 @@ static void test_opts_parse_size(void) g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, 8); g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 1536); g_assert_cmphex(qemu_opt_get_size(opts, "size3", 0), ==, 2 * MiB); - opts = qemu_opts_parse(&opts_list_02, "size1=0.1G,size2=16777215T", + opts = qemu_opts_parse(&opts_list_02, "size1=0.125G,size2=16777215T", false, &error_abort); g_assert_cmpuint(opts_count(opts), ==, 2); - g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 10); + g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 8); g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215ULL * TiB); /* Beyond limit with suffix */ diff --git a/util/cutils.c b/util/cutils.c index 6a8a175e0d71..1154b9de131a 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -246,12 +246,13 @@ static int64_t suffix_mul(char suffix, int64_t unit) * The size parsing supports the following syntaxes * - 12345 - decimal, scale determined by @default_suffix and @unit * - 12345{bBkKmMgGtTpPeE} - decimal, scale determined by suffix and @unit - * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, and - * fractional portion is truncated to byte + * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, if + * fractional portion is exact * - 0x7fEE - hexadecimal, unit determined by @default_suffix * * The following cause a deprecation warning, and may be removed in the future * - 0xabc{kKmMgGtTpP} - hex with scaling suffix + * - 12345.678{kKmMgGtTpPeE} - decimal with inexact fraction that caused truncation * * The following are intentionally not supported * - octal, such as 08 @@ -342,6 +343,10 @@ static int do_strtosz(const char *nptr, const char **end, retval = -ERANGE; goto out; } + if (mul_required && fraction * mul != (uint64_t) (fraction * mul)) { + warn_report("Using a fractional size that is not an exact byte " + "multiple is deprecated: %s", nptr); + } *result = val * mul + (uint64_t) (fraction * mul); retval = 0; -- 2.30.1

2 2

[PATCH] NEW: Mention some bug fixes for the 7.1.0 dev cycle
by Jim Fehlig 24 Feb '21

24 Feb '21

Signed-off-by: Jim Fehlig <jfehlig(a)suse.com> --- NEWS.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 6ca539b1cd..8cb26d4d4c 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -88,6 +88,23 @@ v7.1.0 (unreleased) For more details why this is necessary see `systemd cgroup <https://systemd.io/CGROUP_DELEGATION/>`_ documentation. + * qemu: Fix swtpm device with aarch64 + + The TPM TIS device name for x86 is ``tpm-tis``, whereas for aarch64 it is + ``tpm-tis-device``. Fix the use of TPM TIS device with aarch64 by using + the proper device name when building the QEMU command line. + + * libxl: Fix domain shutdown + + Commit fa30ee04a2 introduced the possibility of a race between the + shutdown and death threads used to process domain shutdown and death + events from libxl. On normal domain shutdown the shutdown thread handles + all aspects of shutting down and cleaning up the domain. The death + thread is only used to handle out-of-band domain destruction and is + inhibited when domain shutdown is under libvirt's control. The race is + avoided by also inhibiting the death thread when libvirt starts the + shutdown thread. + v7.0.0 (2021-01-15) =================== -- 2.29.2

3 2

[libvirt PATCH 0/2] Fix bonehead errors in <hostdev> <teaming> support
by Laine Stump 24 Feb '21

24 Feb '21

I hadn't caught the code omission because my migration tester system was out of commission, and I stupidly assumed migration would work. The docs typo was pure lack of attention. This (sub) feature will be new for this release, and there is an open BZ for it downstream, so it would be nice to get the fix ACKed and pushed in time for the release. Laine Stump (2): qemu: allow migration of generic <hostdev> with <teaming> docs: fix bad cut/paste in <teaming> example docs/formatdomain.rst | 3 +-- src/qemu/qemu_migration.c | 16 ++++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) -- 2.29.2

2 3

[PATCH] qemuMonitorUnregister: Fix use-after-free of mon->watch
by Peng Liang 23 Feb '21

23 Feb '21

qemuMonitorUnregister will be called in multiple threads (e.g. threads in rpc worker pool and the vm event thread). In some cases, it isn't protected by the monitor lock, which may lead to call g_source_unref more than one time and a use-after-free problem eventually. To avoid similar problem in the future, use g_atomic_pointer_compare_and_exchange instead of adding a lock in the missing cases. Signed-off-by: Peng Liang <liangpeng10(a)huawei.com> --- src/qemu/qemu_monitor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0476d606f553..f4d05cd951c2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -853,10 +853,11 @@ qemuMonitorRegister(qemuMonitorPtr mon) void qemuMonitorUnregister(qemuMonitorPtr mon) { - if (mon->watch) { - g_source_destroy(mon->watch); - g_source_unref(mon->watch); - mon->watch = NULL; + GSource *watch = mon->watch; + + if (watch && g_atomic_pointer_compare_and_exchange(&mon->watch, watch, NULL)) { + g_source_destroy(watch); + g_source_unref(watch); } } -- 2.29.2

2 1

[PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes
by Eric Blake 23 Feb '21

23 Feb '21

Supporting '0x20M' looks odd, particularly since we have a 'B' suffix that is ambiguous for bytes, as well as a less-frequently-used 'E' suffix for extremely large exibytes. In practice, people using hex inputs are specifying values in bytes (and would have written 0x2000000, or possibly relied on default_suffix in the case of qemu_strtosz_MiB), and the use of scaling suffixes makes the most sense for inputs in decimal (where the user would write 32M). But rather than outright dropping support for hex-with-suffix, let's follow our deprecation policy. Sadly, since qemu_strtosz() does not have an Err** parameter, and plumbing that in would be a much larger task, we instead go with just directly emitting the deprecation warning to stderr. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- docs/system/deprecated.rst | 8 ++++++++ util/cutils.c | 10 +++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 2fcac7861e07..113c2e933f1b 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -146,6 +146,14 @@ library enabled as a cryptography provider. Neither the ``nettle`` library, or the built-in cryptography provider are supported on FIPS enabled hosts. +hexadecimal sizes with scaling multipliers (since 6.0) +'''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Input parameters that take a size value should only use a size suffix +(such as 'k' or 'M') when the base is written in decimal, and not when +the value is hexadecimal. That is, '0x20M' is deprecated, and should +be written either as '32M' or as '0x2000000'. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/util/cutils.c b/util/cutils.c index 22d27b0fd21b..6a8a175e0d71 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit) * fractional portion is truncated to byte * - 0x7fEE - hexadecimal, unit determined by @default_suffix * + * The following cause a deprecation warning, and may be removed in the future + * - 0xabc{kKmMgGtTpP} - hex with scaling suffix + * * The following are intentionally not supported * - octal, such as 08 * - fractional hex, such as 0x1.8 @@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end, int retval; const char *endptr, *f; unsigned char c; - bool mul_required = false; + bool mul_required = false, hex = false; uint64_t val; int64_t mul; double fraction = 0.0; @@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end, retval = -EINVAL; goto out; } + hex = true; } else if (*endptr == '.') { /* * Input looks like a fraction. Make sure even 1.k works @@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end, c = *endptr; mul = suffix_mul(c, unit); if (mul > 0) { + if (hex) { + warn_report("Using a multiplier suffix on hex numbers " + "is deprecated: %s", nptr); + } endptr++; } else { mul = suffix_mul(default_suffix, unit); -- 2.30.1

3 2

[PATCH] libxl: Add lock process indicator to libxlDomainObjPrivate object
by Jim Fehlig 23 Feb '21

23 Feb '21

The libvirt libxl driver has no access to FDs associated with VM disks. The disks are opened by libxl.so and any related FDs are not exposed to applications. The prevents using virtlockd's auto-release feature to release locks when the FD is closed. Acquiring and releasing locks is explicitly handled by the libxl driver. The current logic is structured such that locks are acquired in libxlDomainStart and released in libxlDomainCleanup. This works well except for migration, where the locks must be released on the source host before the domain can be started on the destination host, but the domain cannot be cleaned up until the migration confirmation stage. When libxlDomainCleanup if finally called in the confirm stage, locks are again released resulting in confusing errors from virtlockd and libvirtd virtlockd[8095]: resource busy: Lockspace resource 'xxxxxx' is not locked libvirtd[8050]: resource busy: Lockspace resource 'xxxxxx' is not locked libvirtd[8050]: Unable to release lease on testvm The error is also encountered in some error cases, e.g. when libxlDomainStart fails before acquiring locks and libxlDomainCleanup is still used for cleanup. In lieu of a mechanism to check if a lock has been acquired, this patch takes an easy approach to fixing the unnecessary lock releases by adding an indicator to the libxlDomainPrivate object that can be set when the lock is acquired and cleared when the lock is released. libxlDomainCleanup can then skip releasing the lock in cases where it was previously released or never acquired in the first place. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com> --- danpb and I discussed this issue on IRC and after pursuing several options I've decided to send this simple approach to the list for comment. I reworked the code in several ways but in the end cant escape the fact that there are occasions where domain cleanup needs to be performed but stopping the lock process is not needed. Another simple approach I pursued was creating a cleanup function that skipped virDomainLockProcessPause, but this approach was a bit cleaner IMO. Other suggestions welcomed. All is not lost in the effort as it resulted in several cleanup and improvement patches that I'll post after release. It includes decomposing libxlDomainStart and making it idempotent on error so libxlDomainCleanup only needs to be called after a successful call to libxlDomainStart. src/libxl/libxl_domain.c | 13 +++++++++---- src/libxl/libxl_domain.h | 1 + src/libxl/libxl_migration.c | 8 ++++++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 604b94a006..014f6aceca 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -861,10 +861,14 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME, vm->def, hostdev_flags); - VIR_FREE(priv->lockState); - if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) - VIR_WARN("Unable to release lease on %s", vm->def->name); - VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + if (priv->lockProcessRunning) { + VIR_FREE(priv->lockState); + if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) + VIR_WARN("Unable to release lease on %s", vm->def->name); + else + priv->lockProcessRunning = false; + VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + } libxlLoggerCloseFile(cfg->logger, vm->def->id); vm->def->id = -1; @@ -1426,6 +1430,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, priv->lockState) < 0) goto destroy_dom; VIR_FREE(priv->lockState); + priv->lockProcessRunning = true; /* Always enable domain death events */ if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW)) diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 00682546e0..5fb76bd303 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -68,6 +68,7 @@ struct _libxlDomainObjPrivate { virThreadPtr migrationDstReceiveThr; unsigned short migrationPort; char *lockState; + bool lockProcessRunning; struct libxlDomainJobObj job; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 631f67930d..a5a9df98ad 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1242,9 +1242,12 @@ libxlDomainMigrationSrcPerform(libxlDriverPrivatePtr driver, virObjectLock(vm); if (ret == 0) { - if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) + if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) == 0) { + priv->lockProcessRunning = false; + VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + } else { VIR_WARN("Unable to release lease on %s", vm->def->name); - VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + } } else { /* * Confirm phase will not be executed if perform fails. End the @@ -1377,6 +1380,7 @@ libxlDomainMigrationSrcConfirm(libxlDriverPrivatePtr driver, "xen:///system", vm, priv->lockState); + priv->lockProcessRunning = true; if (libxl_domain_resume(cfg->ctx, vm->def->id, 1, 0) == 0) { ret = 0; } else { -- 2.29.2

2 1

A brief look at deprecating our JSON extensions over RFC 8259
by Markus Armbruster 23 Feb '21

23 Feb '21

We use JSON in several external interfaces: * QMP * The guest agent's QMP * QAPIfied command line options when the option argument starts with '{' * The block layer's pseudo-protocol "json:" (which can get embedded in image headers) I *think* that's all. The JSON parser we use for these interfaces supports extensions over RFC 8259. Quoting json-lexer.c: - Extra escape sequence in strings: 0x27 (apostrophe) is recognized after escape, too - Single-quoted strings: Like double-quoted strings, except they're delimited by %x27 (apostrophe) instead of %x22 (quotation mark), and can't contain unescaped apostrophe, but can contain unescaped quotation mark. - Interpolation, if enabled: The lexer accepts %[A-Za-z0-9]*, and leaves rejecting invalid ones to the parser. Ignore interpolation; it's never enabled at external interfaces. This leaves single-quotes strings and the escape sequence to go with them. I disabled them as an experiment. Some 20 iotests, a qtest and two unit tests explode. The unit test testing the JSON parser is of course excused. The remaining qtest and the unit test could perhaps be dismissed as atypical use of QEMU from C. The iotests less so, I think. I looked at some iotest failures, and quickly found single-quoted strings used with all external interfaces except for qemu-ga's QMP. We could certainly tidy up the tests to stick to standard JSON. However, the prevalence of single-quoted strings in iotests makes me suspect that they are being used in the field as well. Deprecating the extension is likely more trouble than it's worth. Opinions?

6 17

[PATCH python] iothread: fix memory access out of bounds
by Hogan Wang 23 Feb '21

23 Feb '21

From: suruifeng <suruifeng(a)huawei.com> When the 'pcpu' is larger then the last 'iothr->cpumap' bits, set the list element to False to avoid out of bounds access 'iothr->cpumap'. Signed-off-by: suruifeng <suruifeng(a)huawei.com> Reviewed-by: Hogan Wang <hogan.wang(a)huawei.com> --- libvirt-override.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/libvirt-override.c b/libvirt-override.c index 1f55864..b099f51 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -1625,10 +1625,14 @@ libvirt_virDomainGetIOThreadInfo(PyObject *self ATTRIBUTE_UNUSED, VIR_PY_TUPLE_SET_GOTO(iothrtpl, 1, iothrmap, cleanup); for (pcpu = 0; pcpu < cpunum; pcpu++) - VIR_PY_LIST_SET_GOTO(iothrmap, pcpu, - PyBool_FromLong(VIR_CPU_USED(iothr->cpumap, - pcpu)), - cleanup); + if (VIR_CPU_MAPLEN(pcpu + 1) > iothr->cpumaplen) { + VIR_PY_LIST_SET_GOTO(iothrmap, pcpu, PyBool_FromLong(0), cleanup); + } else { + VIR_PY_LIST_SET_GOTO(iothrmap, pcpu, + PyBool_FromLong(VIR_CPU_USED(iothr->cpumap, + pcpu)), + cleanup); + } } py_retval = py_iothrinfo; -- 2.23.0

2 1

[libvirt PATCH] NEWS: mention cgroups on hosts with systemd bug fix
by Pavel Hrdina 23 Feb '21

23 Feb '21

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com> --- NEWS.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index bee49923a5..eac86d8a4e 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -71,6 +71,23 @@ v7.1.0 (unreleased) of relying on ``VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE`` if they need finer grained control. + * cgroups: Fix how we setup and configure cgroups on hosts with systemd + + When libvirt is running on host with systemd we register every VM with + machined which creates the VM root cgroup for us as well. Before this fix + we were directly modifying files in the VM root cgroup which was incorrect + because all the files are managed by systemd. The implication was that any + change done by libvirt to cgroup attributes supported by systemd could be + removed which happens for example by running ``systemctl daemon-reload``. + + To fix the issue libvirt now uses DBus calls for some of the cgroup + attributes that distributes the resources proportionally to the cgroup + siblings and for the rest we have new sub-cgroup that libvirt can managed + directly. + + For more details why this is necessary see + `systemd cgroug <https://systemd.io/CGROUP_DELEGATION/>`_ documentation. + v7.0.0 (2021-01-15) =================== -- 2.29.2

2 1