September 2021 - Devel - libvirt List Archives

[PATCH 0/6] qemu: Fix crash when validating a XML without disk's <target>
by Peter Krempa 08 Sep '21

08 Sep '21

Apart from fixing the crash the validation code is fixed to do the correct thing and a test case is added. Peter Krempa (6): qemuDomainDefValidateDiskLunSource: Unbreak error messages conf: validate: Move qemu-specific LUN disk validation to global validation conf: validate: Run global device definition validation before callbacks conf: Don't call 'virDomainDiskDefAssignAddress' when disk->dst is NULL virDomainDiskDefValidate: Move validation of disk target qemuxml2argvtest: Add test case for missing disk '<target>' src/conf/domain_conf.c | 1 + src/conf/domain_validate.c | 94 ++++++++++++++----- src/conf/domain_validate.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 50 ---------- src/qemu/qemu_domain.h | 3 - src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_validate.c | 3 - .../default-video-type-x86_64-caps-test-0.err | 2 +- .../disk-fdc-incompatible-address.err | 2 +- .../disk-ide-incompatible-address.err | 2 +- .../disk-missing-target-invalid.err | 1 + .../disk-missing-target-invalid.xml | 22 +++++ .../disk-sata-incompatible-address.err | 2 +- .../disk-scsi-incompatible-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../pseries-phb-invalid-target-index-1.err | 2 +- .../pseries-phb-invalid-target-index-2.err | 2 +- .../pseries-phb-invalid-target-index-3.err | 2 +- .../video-invalid-multiple-devices.err | 2 +- ...splay-device-pci-address.x86_64-latest.err | 2 +- tests/qemuxml2argvtest.c | 1 + 22 files changed, 109 insertions(+), 93 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-missing-target-invalid.err create mode 100644 tests/qemuxml2argvdata/disk-missing-target-invalid.xml -- 2.31.1

2 7

[PATCH v2 0/3] support for virtio-blk-pci discard option
by yuxiating＠huawei.com 08 Sep '21

08 Sep '21

From: yuxiating <yuxiating(a)huawei.com> v1->v2 add docs/formatdomain.rst for discard_enable add qemuxml2argvtest change virDomainDiskDiscardEnable to virTristateSwitch yuxiating (3): qemu: probe for virtio-blk-pci discard option support conf: support for virtio-blk-pci discard option qemu: command: support for virtio-blk-pci discard option docs/formatdomain.rst | 3 ++ docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 8 +++++ src/conf/domain_conf.h | 1 + src/conf/domain_validate.c | 6 ++++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 5 +++ .../caps_4.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.xml | 1 + .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + .../caps_5.2.0.riscv64.xml | 1 + .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../qemuxml2argvdata/disk-virtio-discard.args | 29 +++++++++++++++ .../qemuxml2argvdata/disk-virtio-discard.xml | 35 +++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 36 files changed, 125 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml -- 2.27.0

2 6

[libvirt PATCH 0/6] libxl: remove enum libxlHwcapVersion
by Ján Tomko 07 Sep '21

07 Sep '21

While looking at users of virCPU.*Free, I found some code that is supposed to deal with Xen < 4.7. The minimum version is 4.9 since the following commit: commit 68940b3fb3c43b8aa03cb6fd2f1d00b1737c9b2c Author: Jim Fehlig <jfehlig(a)suse.com> CommitDate: 2021-06-17 10:11:56 -0600 Xen: Bump minimum supported Xen version to 4.9 Remove the code dealing with the old version and use g_auto moer. Ján Tomko (6): libxl: capabilities: assume Xen version >= 4.7 libxl: remove enum libxlHwcapVersion libxl: capsInitCPU: fail if we can't initialize host features libxl: refactor libxlCapsInitCPU libxl: use g_auto in libxlCapsNodeData libxl: use g_auto in libxlDomainGetEmulatorType src/libxl/libxl_capabilities.c | 119 ++++++++++----------------------- 1 file changed, 37 insertions(+), 82 deletions(-) -- 2.31.1

2 7

Re: [PATCH 2/2] qemu: support for virtio-blk-pci discard options
by y005616296 07 Sep '21

07 Sep '21

>On Tue, Aug 31, 2021 at 20:50:04 +0800, yuxiating wrote: >> DISCARD and WRITE_ZEROES features for machine type >= 4.0 is enabled by default since >> commit 5c81161f804144b146607f890e84613a4cbad95c >> virtio-blk: add "discard" and "write-zeroes" properties >> Sometimes guestos has bugs DISCARD need to be disabled. > >I'm not very persuaded by this commit message, could you please elaborate how the bugs show themselves? > >Specifically this fells like a hack/workaround rather so a proper justification would be great. > Virtio_blk kernel driver has a bug that causes memory corruption in virtblk_setup_discard_write_zeroes(); af822aa68fbd ("block: virtio_blk: fix handling single range discard request") has fix it. However, some operating systems are not fixed and need to disabled on the QEMU side. >> Signed-off-by: yuxiating <yuxiating(a)huawei.com> >> --- >> src/conf/domain_conf.c | 15 +++++++++++++++ >> src/conf/domain_conf.h | 9 +++++++++ >> src/conf/domain_validate.c | 6 ++++++ >> src/libvirt_private.syms | 3 ++- >> src/qemu/qemu_command.c | 11 +++++++++++ >> 5 files changed, 43 insertions(+), 1 deletion(-) > >Please split the conf/* changes from the qemu changes. > >Also you didn't add any documentation to docs/formatdomain.rst which describes the XML which is unacceptable. > >Additionally you are also missing test cases for qemuxml2argvtest. > I will add a description to docs/ formatDomain.rst. qemuxml2argvtest will be added in version V2. >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index >> 6127513117..bfe4721e60 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -1278,6 +1278,13 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, >> "ignore", >> ); >> >> +VIR_ENUM_IMPL(virDomainDiskDiscardEnable, >> + VIR_DOMAIN_DISK_DISCARD_ENABLE_LAST, >> + "default", >> + "off", >> + "on", > >NACK to this hunk, 'default' value feels weird, and for the rest we have the virTristateBool type. > > I find virTristateSwitch more appropriate. >> +); >> + >> VIR_ENUM_IMPL(virDomainDiskDetectZeroes, >> VIR_DOMAIN_DISK_DETECT_ZEROES_LAST, >> "default", >> @@ -8930,6 +8937,10 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def, >> if (virXMLPropUInt(cur, "queues", 10, VIR_XML_PROP_NONE, &def->queues) < 0) >> return -1; >> >> + if (virXMLPropEnum(cur, "discard_enable", virDomainDiskDiscardEnableTypeFromString, >> + VIR_XML_PROP_NONZERO, &def->discard_enable) < 0) >> + return -1; >> + >> return 0; >> } >> >> @@ -23416,6 +23427,10 @@ virDomainDiskDefFormatDriver(virBuffer *buf, >> if (disk->queues) >> virBufferAsprintf(&attrBuf, " queues='%u'", disk->queues); >> >> + if (disk->discard_enable) >> + virBufferAsprintf(&attrBuf, " discard_enable='%s'", >> + >> + virDomainDiskDiscardEnableTypeToString(disk->discard_enable)); > >It even behaves exactly like virTristateBool. > I'll use virTristateSwitch instead >> + >> virDomainVirtioOptionsFormat(&attrBuf, disk->virtio); >> >> if (disk->src->metadataCacheMaxSize > 0) { diff --git >> a/src/conf/domain_conf.h b/src/conf/domain_conf.h index >> c7e6df7981..c39694a19e 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h > >[...] > >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index >> ab8a6c00c3..52a74dd2d5 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -1,5 +1,5 @@ >> -# >> # General private symbols. Add symbols here, and see src/meson.build >> for >> +# mainDiskDeviceTypeToString >> # more details. >> # >> # Keep this file sorted by header name, then by symbols with each header. > >NACK to this hunk too, you shouldn't need to modify the header here. > It was my mistake. >> @@ -377,6 +377,7 @@ virDomainDiskDefParseSource; >> virDomainDiskDetectZeroesTypeFromString; >> virDomainDiskDetectZeroesTypeToString; >> virDomainDiskDeviceTypeToString; >> +virDomainDiskDiscardEnableTypeToString; >> virDomainDiskDiscardTypeToString; >> virDomainDiskEmptySource; >> virDomainDiskErrorPolicyTypeFromString; >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index >> b230314f7f..894c8b17b9 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -1739,6 +1739,17 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, >> virBufferAsprintf(&opt, ",num-queues=%u", disk->queues); >> } >> >> + if (disk->discard_enable) { >> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DISCARD)) >> + { > >This capability was also checked in the validator so you don't need to do this here. > >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("virtio-blk discard property isn't supported by this " >> + "QEMU binary")); > > > I'm going to delete this check. >> + return NULL; >> + } >> + virBufferAsprintf(&opt, ",discard=%s", >> + virDomainDiskDiscardEnableTypeToString(disk->discard_enable)); >> + } >> + >> qemuBuildVirtioOptionsStr(&opt, disk->virtio); >> >> if (qemuBuildDeviceAddressStr(&opt, def, &disk->info) < 0) >> -- >> 2.27.0 >> >>

1 0

[PATCH] qemu_security: Set the label of monitor
by Peng Liang 06 Sep '21

06 Sep '21

Signed-off-by: Peng Liang <liangpeng10(a)huawei.com> --- src/qemu/qemu_security.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 19d957dd4b96..96755a62bd2c 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver, priv->chardevStdioLogd, migrated) < 0) goto cleanup; + if (priv->monConfig && + virSecurityManagerSetChardevLabel(driver->securityManager, + vm->def, + priv->monConfig, + priv->chardevStdioLogd) < 0) + goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, pid, priv->rememberOwner) < 0) -- 2.31.1

2 4

[libvirt PATCH 0/6] tests: refactor some functions (glib chronicles)
by Ján Tomko 06 Sep '21

06 Sep '21

Ján Tomko (6): tests: esxutils: refactor testParseDatastorePath tests: esxutils: reduce variable scope in testEscapeDatastoreItem tests: esxutils: reduce variable scope in testConvertWindows1252ToUTF8 tests: reduce variable scope in testSELinuxCheckLabels tests: refactor testSELinuxLoadDef virnetsockettest: refactor checkProtocols tests/esxutilstest.c | 46 +++++++++----------------------- tests/securityselinuxlabeltest.c | 41 +++++++++------------------- tests/virnetsockettest.c | 26 +++++++----------- 3 files changed, 34 insertions(+), 79 deletions(-) -- 2.31.1

2 7

[libvirt PATCH] secretxml2xmltest: refactor testCompareXMLToXMLFiles (glib chronicles)
by Ján Tomko 06 Sep '21

06 Sep '21

Use g_auto where possible and remove the pointless label. Signed-off-by: Ján Tomko <jtomko(a)redhat.com> --- tests/secretxml2xmltest.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index 48f48416b5..99aaa9db15 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -10,25 +10,19 @@ static int testCompareXMLToXMLFiles(const char *inxml, const char *outxml) { - char *actual = NULL; - int ret = -1; - virSecretDef *secret = NULL; + g_autofree char *actual = NULL; + g_autoptr(virSecretDef) secret = NULL; if (!(secret = virSecretDefParseFile(inxml))) - goto fail; + return -1; if (!(actual = virSecretDefFormat(secret))) - goto fail; + return -1; if (virTestCompareToFile(actual, outxml) < 0) - goto fail; + return -1; - ret = 0; - - fail: - VIR_FREE(actual); - virSecretDefFree(secret); - return ret; + return 0; } struct testInfo { -- 2.31.1

2 1

[libvirt PATCH 0/4] Reinstate -Wunused-but-set-variable
by Ján Tomko 06 Sep '21

06 Sep '21

Personally, I feel like all the cases caught by new Clang's -Wunused-but-set-variable behavior were also unnecessarily difficult to read for humans, we should be more obvious when relying purely on side effects of g_auto and the warning is worth keeping. Even if not, the first two patches should not be controversial. Ján Tomko (4): qemu: remove unused 'cfg' variables util: virIdentitySetCurrent: only unref the old identity on success rpc: mark source returned by virEventGLibAddSocketWatch as unused Revert "meson: avoid bogus warnings from clang and g_autoptr" build-aux/syntax-check.mk | 2 +- meson.build | 21 --------------------- src/qemu/qemu_driver.c | 9 --------- src/rpc/virnetclient.c | 6 +++--- src/util/viridentity.c | 4 +++- 5 files changed, 7 insertions(+), 35 deletions(-) -- 2.31.1

2 5

[libvirt PATCH 0/2] tests: bhyve: minor style tweaks for constants
by Ján Tomko 06 Sep '21

06 Sep '21

I found these lying around in a forgotten branch. Ján Tomko (2): tests: bhyve: remove magic constants tests: bhyve: use bitwise shift when defining flags tests/bhyveargv2xmltest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) -- 2.31.1

2 3

[PATCH] qemu_driver: Don't check for g_strdup_printf() retval
by Michal Privoznik 06 Sep '21

06 Sep '21

The g_strdup_printf() function can't fail really. There's no need to check for its return value. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_driver.c | 49 +++++++++--------------------------------- 1 file changed, 10 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbd48b1849..2ea67b941e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -357,13 +357,7 @@ qemuDomainSnapshotLoad(virDomainObj *vm, priv = vm->privateData; - if (!(snapDir = g_strdup_printf("%s/%s", baseDir, vm->def->name))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to allocate memory for " - "snapshot directory for domain %s"), - vm->def->name); - goto cleanup; - } + snapDir = g_strdup_printf("%s/%s", baseDir, vm->def->name); VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name, snapDir); @@ -379,11 +373,7 @@ qemuDomainSnapshotLoad(virDomainObj *vm, kill the whole process */ VIR_INFO("Loading snapshot file '%s'", entry->d_name); - if (!(fullpath = g_strdup_printf("%s/%s", snapDir, entry->d_name))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to allocate memory for path")); - continue; - } + fullpath = g_strdup_printf("%s/%s", snapDir, entry->d_name); if (virFileReadAll(fullpath, 1024*1024*1, &xmlStr) < 0) { /* Nothing we can do here, skip this one */ @@ -464,13 +454,7 @@ qemuDomainCheckpointLoad(virDomainObj *vm, virObjectLock(vm); priv = vm->privateData; - if (!(chkDir = g_strdup_printf("%s/%s", baseDir, vm->def->name))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to allocate memory for " - "checkpoint directory for domain %s"), - vm->def->name); - goto cleanup; - } + chkDir = g_strdup_printf("%s/%s", baseDir, vm->def->name); VIR_INFO("Scanning for checkpoints for domain %s in %s", vm->def->name, chkDir); @@ -486,11 +470,7 @@ qemuDomainCheckpointLoad(virDomainObj *vm, kill the whole process */ VIR_INFO("Loading checkpoint file '%s'", entry->d_name); - if (!(fullpath = g_strdup_printf("%s/%s", chkDir, entry->d_name))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to allocate memory for path")); - continue; - } + fullpath = g_strdup_printf("%s/%s", chkDir, entry->d_name); if (virFileReadAll(fullpath, 1024*1024*1, &xmlStr) < 0) { /* Nothing we can do here, skip this one */ @@ -626,8 +606,7 @@ qemuStateInitialize(bool privileged, if (!(qemu_driver->config = cfg = virQEMUDriverConfigNew(privileged, root))) goto error; - if (!(driverConf = g_strdup_printf("%s/qemu.conf", cfg->configBaseDir))) - goto error; + driverConf = g_strdup_printf("%s/qemu.conf", cfg->configBaseDir); if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0) goto error; @@ -2953,13 +2932,9 @@ qemuDomainSave(virDomainPtr dom, const char *path) static char * qemuDomainManagedSavePath(virQEMUDriver *driver, virDomainObj *vm) { - char *ret; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - if (!(ret = g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name))) - return NULL; - - return ret; + return g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name); } static int @@ -3461,8 +3436,7 @@ qemuDomainScreenshot(virDomainPtr dom, } } - if (!(tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", cfg->cacheDir))) - goto endjob; + tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", cfg->cacheDir); if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"), tmp); @@ -5296,8 +5270,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriver *driver, bool threadAdded = false; bool objectAdded = false; - if (!(alias = g_strdup_printf("iothread%u", iothread_id))) - return -1; + alias = g_strdup_printf("iothread%u", iothread_id); if (qemuMonitorCreateObjectProps(&props, "iothread", alias, NULL) < 0) goto cleanup; @@ -5429,8 +5402,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriver *driver, int new_niothreads = 0; qemuMonitorIOThreadInfo **new_iothreads = NULL; - if (!(alias = g_strdup_printf("iothread%u", iothread_id))) - return -1; + alias = g_strdup_printf("iothread%u", iothread_id); qemuDomainObjEnterMonitor(driver, vm); @@ -10716,8 +10688,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto endjob; - if (!(tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", cfg->cacheDir))) - goto endjob; + tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", cfg->cacheDir); /* Create a temporary filename. */ if ((fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { -- 2.31.1

2 2