[libvirt PATCH v4 0/5] Make unknown XML elements fail CPU comparison
We currently ignore unknown elements in the CPU XML description, e.g. with virsh cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture name=3D"..."/></cpu>' (note the typo in "faeture") semantically identic to '<cpu/>'. No error is reported. This series adds checks for unrecognized attributes and elements in the "<cpu>" element, catching this kind of mistake. V1: https://www.redhat.com/archives/libvir-list/2020-September/msg00933.html V2: https://www.redhat.com/archives/libvir-list/2020-September/msg01073.html V3: https://www.redhat.com/archives/libvir-list/2020-September/msg01555.html Changed: * Some patches from V3 are merged * Changes to commit messages * Perform testing by means of virschematest Tim Wiederhake (5): schema: Make element "topology" in host CPU definition optional cpu: Wire in XML validation tests: Rename some test files in cputestdata tests: Enable CPU XML validation in the tests. virsh: Add "validate" argument to [hypervisor-]cpu-compare docs/manpages/virsh.rst | 9 ++-- docs/schemas/cpu_test.rng | 48 +++++++++++++++++++ docs/schemas/cputypes.rng | 24 +++++----- docs/schemas/meson.build | 1 + include/libvirt/libvirt-host.h | 2 + src/bhyve/bhyve_driver.c | 7 ++- src/conf/cpu_conf.c | 25 ++++++++-- src/conf/cpu_conf.h | 6 ++- src/conf/domain_conf.c | 3 +- src/cpu/cpu.c | 5 +- src/cpu/cpu.h | 3 +- src/libxl/libxl_driver.c | 7 ++- src/qemu/qemu_domain.c | 5 +- src/qemu/qemu_driver.c | 18 +++++-- src/qemu/qemu_migration_cookie.c | 3 +- tests/cputest.c | 15 +++--- ...invalid.xml =3D> ppc64-guest-compat-bad.xml} | 0 ...invalid.xml =3D> ppc64-guest-legacy-bad.xml} | 0 ...id.xml =3D> ppc64-host+guest-compat-bad.xml} | 0 ...id.xml =3D> ppc64-host+guest-legacy-bad.xml} | 0 tests/virschematest.c | 1 + tools/virsh-host.c | 14 ++++++ 22 files changed, 153 insertions(+), 43 deletions(-) create mode 100644 docs/schemas/cpu_test.rng rename tests/cputestdata/{ppc64-guest-compat-invalid.xml =3D> ppc64-guest-co= mpat-bad.xml} (100%) rename tests/cputestdata/{ppc64-guest-legacy-invalid.xml =3D> ppc64-guest-le= gacy-bad.xml} (100%) rename tests/cputestdata/{ppc64-host+guest-compat-invalid.xml =3D> ppc64-hos= t+guest-compat-bad.xml} (100%) rename tests/cputestdata/{ppc64-host+guest-legacy-invalid.xml =3D> ppc64-hos= t+guest-legacy-bad.xml} (100%) --=20 2.26.2
This element is not always present, see e.g. x86_64-cpuid-Xeon-X5460-host.xml, x86_64-cpuid-Pentium-P6100-host.xml, or x86_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/schemas/cputypes.rng | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng index 88f6904343..aea6ff0267 100644 --- a/docs/schemas/cputypes.rng +++ b/docs/schemas/cputypes.rng @@ -336,17 +336,19 @@ </attribute> </element> </optional> - <element name="topology"> - <attribute name="sockets"> - <ref name="positiveInteger"/> - </attribute> - <attribute name="cores"> - <ref name="positiveInteger"/> - </attribute> - <attribute name="threads"> - <ref name="positiveInteger"/> - </attribute> - </element> + <optional> + <element name="topology"> + <attribute name="sockets"> + <ref name="positiveInteger"/> + </attribute> + <attribute name="cores"> + <ref name="positiveInteger"/> + </attribute> + <attribute name="threads"> + <ref name="positiveInteger"/> + </attribute> + </element> + </optional> <zeroOrMore> <element name="feature"> <attribute name="name"> -- 2.26.2
On Wed, Oct 07, 2020 at 10:54:54 +0200, Tim Wiederhake wrote:
This element is not always present, see e.g. x86_64-cpuid-Xeon-X5460-host.xml, x86_64-cpuid-Pentium-P6100-host.xml, or x86_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml.
Unfortunately this is not enough to persuade me that the change is correct though. Jirka?
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> ---
On Wed, Oct 07, 2020 at 11:08:21 +0200, Peter Krempa wrote:
On Wed, Oct 07, 2020 at 10:54:54 +0200, Tim Wiederhake wrote:
This element is not always present, see e.g. x86_64-cpuid-Xeon-X5460-host.xml, x86_64-cpuid-Pentium-P6100-host.xml, or x86_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml.
Unfortunately this is not enough to persuade me that the change is correct though.
Jirka?
Right, these are just cputest output files so they can't really be used as an evidence for optional <topology>. But it doesn't really matter that much since the host CPU definition is output only in capabilities XML. APIs that accept host CPU definition as their input work only on model, vendor, and features thus ignoring any topology element. Also looking at the CPU formatting code we format <topology> only if (def->sockets && def->dies && def->cores && def->threads). The question is whether caps->host.cpu would even be allocated when the topology is unknown. In any case, I think it's fine to mark it as optional in the schema. Another option would be to mock some topology in cputest to make sure the host CPU definitions contain the corresponding element. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On Wed, Oct 07, 2020 at 12:29:42 +0200, Jiri Denemark wrote:
On Wed, Oct 07, 2020 at 11:08:21 +0200, Peter Krempa wrote:
On Wed, Oct 07, 2020 at 10:54:54 +0200, Tim Wiederhake wrote:
This element is not always present, see e.g. x86_64-cpuid-Xeon-X5460-host.xml, x86_64-cpuid-Pentium-P6100-host.xml, or x86_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml.
Unfortunately this is not enough to persuade me that the change is correct though.
Jirka?
Right, these are just cputest output files so they can't really be used as an evidence for optional <topology>.
But it doesn't really matter that much since the host CPU definition is output only in capabilities XML. APIs that accept host CPU definition as their input work only on model, vendor, and features thus ignoring any topology element.
Also looking at the CPU formatting code we format <topology> only if (def->sockets && def->dies && def->cores && def->threads). The question is whether caps->host.cpu would even be allocated when the topology is unknown. In any case, I think it's fine to mark it as optional in the schema. Another option would be to mock some topology in cputest to make sure the host CPU definitions contain the corresponding element.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Pushed now. Jirka
This adds a new value to virConnectCompareCPUFlags, "VIR_CONNECT_CPU_VALIDATE_XML", that governs XML document validation in virCPUDefParseXML. In src/conf/cpu_conf.c, include configmake.h for PKGDATADIR and virfile.h for virFileFindResource. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- include/libvirt/libvirt-host.h | 2 ++ src/bhyve/bhyve_driver.c | 7 +++++-- src/conf/cpu_conf.c | 25 +++++++++++++++++++++---- src/conf/cpu_conf.h | 6 ++++-- src/conf/domain_conf.c | 3 ++- src/cpu/cpu.c | 5 +++-- src/cpu/cpu.h | 3 ++- src/libxl/libxl_driver.c | 7 +++++-- src/qemu/qemu_domain.c | 5 +++-- src/qemu/qemu_driver.c | 18 +++++++++++++----- src/qemu/qemu_migration_cookie.c | 3 ++- tests/cputest.c | 5 +++-- 12 files changed, 65 insertions(+), 24 deletions(-) diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h index 6972834175..4caed94a77 100644 --- a/include/libvirt/libvirt-host.h +++ b/include/libvirt/libvirt-host.h @@ -754,6 +754,8 @@ typedef enum { typedef enum { VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE = (1 << 0), /* treat incompatible CPUs as failure */ + VIR_CONNECT_COMPARE_CPU_VALIDATE_XML = (1 << 1), /* validate the xml + document */ } virConnectCompareCPUFlags; int virConnectCompareCPU(virConnectPtr conn, diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 91f41aa238..78c3241293 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1441,14 +1441,17 @@ bhyveConnectCompareCPU(virConnectPtr conn, int ret = VIR_CPU_COMPARE_ERROR; virCapsPtr caps = NULL; bool failIncompatible; + bool validateXML; - virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE, + virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE | + VIR_CONNECT_COMPARE_CPU_VALIDATE_XML, VIR_CPU_COMPARE_ERROR); if (virConnectCompareCPUEnsureACL(conn) < 0) goto cleanup; failIncompatible = !!(flags & VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE); + validateXML = !!(flags & VIR_CONNECT_COMPARE_CPU_VALIDATE_XML); if (!(caps = bhyveDriverGetCapabilities(driver))) goto cleanup; @@ -1464,7 +1467,7 @@ bhyveConnectCompareCPU(virConnectPtr conn, } } else { ret = virCPUCompareXML(caps->host.arch, caps->host.cpu, - xmlDesc, failIncompatible); + xmlDesc, failIncompatible, validateXML); } cleanup: diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index dea950ce68..1910470836 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -20,9 +20,11 @@ #include <config.h> +#include "configmake.h" #include "virerror.h" #include "viralloc.h" #include "virbuffer.h" +#include "virfile.h" #include "cpu_conf.h" #include "domain_conf.h" #include "virstring.h" @@ -281,7 +283,8 @@ virCPUDefCopy(const virCPUDef *cpu) int virCPUDefParseXMLString(const char *xml, virCPUType type, - virCPUDefPtr *cpu) + virCPUDefPtr *cpu, + bool validateXML) { xmlDocPtr doc = NULL; xmlXPathContextPtr ctxt = NULL; @@ -295,7 +298,7 @@ virCPUDefParseXMLString(const char *xml, if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt))) goto cleanup; - if (virCPUDefParseXML(ctxt, NULL, type, cpu) < 0) + if (virCPUDefParseXML(ctxt, NULL, type, cpu, validateXML) < 0) goto cleanup; ret = 0; @@ -323,7 +326,8 @@ int virCPUDefParseXML(xmlXPathContextPtr ctxt, const char *xpath, virCPUType type, - virCPUDefPtr *cpu) + virCPUDefPtr *cpu, + bool validateXML) { g_autoptr(virCPUDef) def = NULL; g_autofree xmlNodePtr *nodes = NULL; @@ -348,6 +352,19 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, return -1; } + if (validateXML) { + g_autofree char *schemafile = NULL; + + if (!(schemafile = virFileFindResource("cpu.rng", + abs_top_srcdir "/docs/schemas", + PKGDATADIR "/schemas"))) + return -1; + + if (virXMLValidateNodeAgainstSchema(schemafile, ctxt->doc, + ctxt->node) < 0) + return -1; + } + def = virCPUDefNew(); if (type == VIR_CPU_TYPE_AUTO) { @@ -1146,7 +1163,7 @@ virCPUDefListParse(const char **xmlCPUs, if (!(doc = virXMLParseStringCtxt(xmlCPUs[i], _("(CPU_definition)"), &ctxt))) goto error; - if (virCPUDefParseXML(ctxt, NULL, cpuType, &cpus[i]) < 0) + if (virCPUDefParseXML(ctxt, NULL, cpuType, &cpus[i], false) < 0) goto error; xmlXPathFreeContext(ctxt); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 24c51e3a63..3ef14b7932 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -192,13 +192,15 @@ virCPUDefCopyWithoutModel(const virCPUDef *cpu); int virCPUDefParseXMLString(const char *xml, virCPUType type, - virCPUDefPtr *cpu); + virCPUDefPtr *cpu, + bool validateXML); int virCPUDefParseXML(xmlXPathContextPtr ctxt, const char *xpath, virCPUType mode, - virCPUDefPtr *cpu); + virCPUDefPtr *cpu, + bool validateXML); bool virCPUDefIsEqual(virCPUDefPtr src, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 175b632a38..c003b5c030 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21917,7 +21917,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainDefTunablesParse(def, ctxt, xmlopt, flags) < 0) goto error; - if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu) < 0) + if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu, + false) < 0) goto error; if (virDomainNumaDefParseXML(def->numa, ctxt) < 0) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 188c5d86b5..bf94811960 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -109,14 +109,15 @@ virCPUCompareResult virCPUCompareXML(virArch arch, virCPUDefPtr host, const char *xml, - bool failIncompatible) + bool failIncompatible, + bool validateXML) { g_autoptr(virCPUDef) cpu = NULL; VIR_DEBUG("arch=%s, host=%p, xml=%s", virArchToString(arch), host, NULLSTR(xml)); - if (virCPUDefParseXMLString(xml, VIR_CPU_TYPE_AUTO, &cpu) < 0) + if (virCPUDefParseXMLString(xml, VIR_CPU_TYPE_AUTO, &cpu, validateXML) < 0) return VIR_CPU_COMPARE_ERROR; return virCPUCompare(arch, host, cpu, failIncompatible); diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index ba8fdd07ba..cc2d132275 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -153,7 +153,8 @@ virCPUCompareResult virCPUCompareXML(virArch arch, virCPUDefPtr host, const char *xml, - bool failIncompatible); + bool failIncompatible, + bool validateXML); virCPUCompareResult virCPUCompare(virArch arch, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e28c649688..824ed60dfd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6529,19 +6529,22 @@ libxlConnectCompareCPU(virConnectPtr conn, libxlDriverConfigPtr cfg; int ret = VIR_CPU_COMPARE_ERROR; bool failIncompatible; + bool validateXML; - virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE, + virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE | + VIR_CONNECT_COMPARE_CPU_VALIDATE_XML, VIR_CPU_COMPARE_ERROR); if (virConnectCompareCPUEnsureACL(conn) < 0) return ret; failIncompatible = !!(flags & VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE); + validateXML = !!(flags & VIR_CONNECT_COMPARE_CPU_VALIDATE_XML); cfg = libxlDriverConfigGet(driver); ret = virCPUCompareXML(cfg->caps->host.arch, cfg->caps->host.cpu, - xmlDesc, failIncompatible); + xmlDesc, failIncompatible, validateXML); virObjectUnref(cfg); return ret; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0687f4584c..9623123d3c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3130,7 +3130,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, qemuDomainSetPrivatePathsOld(driver, vm); - if (virCPUDefParseXML(ctxt, "./cpu", VIR_CPU_TYPE_GUEST, &priv->origCPU) < 0) + if (virCPUDefParseXML(ctxt, "./cpu", VIR_CPU_TYPE_GUEST, &priv->origCPU, + false) < 0) goto error; priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)", @@ -9997,7 +9998,7 @@ qemuDomainSaveCookieParse(xmlXPathContextPtr ctxt G_GNUC_UNUSED, return -1; if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, - &cookie->cpu) < 0) + &cookie->cpu, false) < 0) return -1; cookie->slirpHelper = virXPathBoolean("boolean(./slirpHelper)", ctxt) > 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5aaa969409..8ef812cd94 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12202,20 +12202,23 @@ qemuConnectCompareCPU(virConnectPtr conn, virQEMUDriverPtr driver = conn->privateData; g_autoptr(virCPUDef) cpu = NULL; bool failIncompatible; + bool validateXML; - virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE, + virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE | + VIR_CONNECT_COMPARE_CPU_VALIDATE_XML, VIR_CPU_COMPARE_ERROR); if (virConnectCompareCPUEnsureACL(conn) < 0) return VIR_CPU_COMPARE_ERROR; failIncompatible = !!(flags & VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE); + validateXML = !!(flags & VIR_CONNECT_COMPARE_CPU_VALIDATE_XML); if (!(cpu = virQEMUDriverGetHostCPU(driver))) return VIR_CPU_COMPARE_ERROR; return virCPUCompareXML(driver->hostarch, cpu, - xmlDesc, failIncompatible); + xmlDesc, failIncompatible, validateXML); } @@ -12270,18 +12273,21 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virQEMUCaps) qemuCaps = NULL; bool failIncompatible; + bool validateXML; virCPUDefPtr hvCPU; virCPUDefPtr cpu = NULL; virArch arch; virDomainVirtType virttype; - virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE, + virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE | + VIR_CONNECT_COMPARE_CPU_VALIDATE_XML, VIR_CPU_COMPARE_ERROR); if (virConnectCompareHypervisorCPUEnsureACL(conn) < 0) goto cleanup; failIncompatible = !!(flags & VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE); + validateXML = !!(flags & VIR_CONNECT_COMPARE_CPU_VALIDATE_XML); qemuCaps = virQEMUCapsCacheLookupDefault(driver->qemuCapsCache, emulator, @@ -12305,10 +12311,12 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, } if (ARCH_IS_X86(arch)) { - ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible); + ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible, + validateXML); } else if (ARCH_IS_S390(arch) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON)) { - if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu) < 0) + if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu, + validateXML) < 0) goto cleanup; if (!cpu->model) { diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 91694000d8..abe797759d 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -1263,7 +1263,8 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, return -1; if (flags & QEMU_MIGRATION_COOKIE_CPU && - virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &mig->cpu) < 0) + virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &mig->cpu, + false) < 0) return -1; if (flags & QEMU_MIGRATION_COOKIE_ALLOW_REBOOT && diff --git a/tests/cputest.c b/tests/cputest.c index 383da94938..78a7f2437a 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -81,7 +81,7 @@ cpuTestLoadXML(virArch arch, const char *name) if (!(doc = virXMLParseFileCtxt(xml, &ctxt))) goto cleanup; - virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu); + virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu, false); cleanup: xmlXPathFreeContext(ctxt); @@ -120,7 +120,8 @@ cpuTestLoadMultiXML(virArch arch, for (i = 0; i < n; i++) { ctxt->node = nodes[i]; - if (virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_HOST, &cpus[i]) < 0) + if (virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_HOST, &cpus[i], + false) < 0) goto cleanup_cpus; } -- 2.26.2
On Wed, Oct 07, 2020 at 10:54:55 +0200, Tim Wiederhake wrote:
This adds a new value to virConnectCompareCPUFlags, "VIR_CONNECT_CPU_VALIDATE_XML", that governs XML document validation in virCPUDefParseXML.
In src/conf/cpu_conf.c, include configmake.h for PKGDATADIR and virfile.h for virFileFindResource.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The files contained the "-invalid" marker in their filename, marking them as test cases that are supposed to fail. Unfortunately, the "-invalid" marker does not discriminate between different tests the files might be used in. A later patch will introduce a new test validating the XML. This test is not supposed to fail, as the files contain valid XML. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/cputest.c | 10 +++++----- ...t-compat-invalid.xml => ppc64-guest-compat-bad.xml} | 0 ...t-legacy-invalid.xml => ppc64-guest-legacy-bad.xml} | 0 ...pat-invalid.xml => ppc64-host+guest-compat-bad.xml} | 0 ...acy-invalid.xml => ppc64-host+guest-legacy-bad.xml} | 0 5 files changed, 5 insertions(+), 5 deletions(-) rename tests/cputestdata/{ppc64-guest-compat-invalid.xml => ppc64-guest-compat-bad.xml} (100%) rename tests/cputestdata/{ppc64-guest-legacy-invalid.xml => ppc64-guest-legacy-bad.xml} (100%) rename tests/cputestdata/{ppc64-host+guest-compat-invalid.xml => ppc64-host+guest-compat-bad.xml} (100%) rename tests/cputestdata/{ppc64-host+guest-legacy-invalid.xml => ppc64-host+guest-legacy-bad.xml} (100%) diff --git a/tests/cputest.c b/tests/cputest.c index 78a7f2437a..90f319bf9c 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -1137,10 +1137,10 @@ mymain(void) DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-exact", VIR_CPU_COMPARE_INCOMPATIBLE); DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-legacy", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-legacy-incompatible", VIR_CPU_COMPARE_INCOMPATIBLE); - DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-legacy-invalid", VIR_CPU_COMPARE_ERROR); + DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-legacy-bad", VIR_CPU_COMPARE_ERROR); DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-compat-none", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-compat-valid", VIR_CPU_COMPARE_IDENTICAL); - DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-compat-invalid", VIR_CPU_COMPARE_ERROR); + DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-compat-bad", VIR_CPU_COMPARE_ERROR); DO_TEST_COMPARE(VIR_ARCH_PPC64, "host", "guest-compat-incompatible", VIR_CPU_COMPARE_INCOMPATIBLE); /* guest updates for migration @@ -1158,10 +1158,10 @@ mymain(void) DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-nofallback", VIR_CPU_COMPARE_INCOMPATIBLE); DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-legacy", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-legacy-incompatible", VIR_CPU_COMPARE_INCOMPATIBLE); - DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-legacy-invalid", VIR_CPU_COMPARE_ERROR); + DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-legacy-bad", VIR_CPU_COMPARE_ERROR); DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-compat-none", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-compat-valid", VIR_CPU_COMPARE_IDENTICAL); - DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-compat-invalid", VIR_CPU_COMPARE_ERROR); + DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-compat-bad", VIR_CPU_COMPARE_ERROR); DO_TEST_UPDATE(VIR_ARCH_PPC64, "host", "guest-compat-incompatible", VIR_CPU_COMPARE_INCOMPATIBLE); /* computing baseline CPUs */ @@ -1216,7 +1216,7 @@ mymain(void) DO_TEST_GUESTCPU(VIR_ARCH_PPC64, "host", "guest-nofallback", ppc_models, -1); DO_TEST_GUESTCPU(VIR_ARCH_PPC64, "host", "guest-legacy", ppc_models, 0); DO_TEST_GUESTCPU(VIR_ARCH_PPC64, "host", "guest-legacy-incompatible", ppc_models, -1); - DO_TEST_GUESTCPU(VIR_ARCH_PPC64, "host", "guest-legacy-invalid", ppc_models, -1); + DO_TEST_GUESTCPU(VIR_ARCH_PPC64, "host", "guest-legacy-bad", ppc_models, -1); DO_TEST_CPUID(VIR_ARCH_X86_64, "A10-5800K", JSON_HOST); DO_TEST_CPUID(VIR_ARCH_X86_64, "Atom-D510", JSON_NONE); diff --git a/tests/cputestdata/ppc64-guest-compat-invalid.xml b/tests/cputestdata/ppc64-guest-compat-bad.xml similarity index 100% rename from tests/cputestdata/ppc64-guest-compat-invalid.xml rename to tests/cputestdata/ppc64-guest-compat-bad.xml diff --git a/tests/cputestdata/ppc64-guest-legacy-invalid.xml b/tests/cputestdata/ppc64-guest-legacy-bad.xml similarity index 100% rename from tests/cputestdata/ppc64-guest-legacy-invalid.xml rename to tests/cputestdata/ppc64-guest-legacy-bad.xml diff --git a/tests/cputestdata/ppc64-host+guest-compat-invalid.xml b/tests/cputestdata/ppc64-host+guest-compat-bad.xml similarity index 100% rename from tests/cputestdata/ppc64-host+guest-compat-invalid.xml rename to tests/cputestdata/ppc64-host+guest-compat-bad.xml diff --git a/tests/cputestdata/ppc64-host+guest-legacy-invalid.xml b/tests/cputestdata/ppc64-host+guest-legacy-bad.xml similarity index 100% rename from tests/cputestdata/ppc64-host+guest-legacy-invalid.xml rename to tests/cputestdata/ppc64-host+guest-legacy-bad.xml -- 2.26.2
On Wed, Oct 07, 2020 at 10:54:56 +0200, Tim Wiederhake wrote:
The files contained the "-invalid" marker in their filename, marking them as test cases that are supposed to fail. Unfortunately, the
are supposed to fail in the virschematest.
"-invalid" marker does not discriminate between different tests the files might be used in.
A later patch will introduce a new test validating the XML. This test is not supposed to fail, as the files contain valid XML.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/schemas/cpu_test.rng | 48 +++++++++++++++++++++++++++++++++++++++ docs/schemas/meson.build | 1 + tests/cputest.c | 2 +- tests/virschematest.c | 1 + 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 docs/schemas/cpu_test.rng diff --git a/docs/schemas/cpu_test.rng b/docs/schemas/cpu_test.rng new file mode 100644 index 0000000000..471958bb7b --- /dev/null +++ b/docs/schemas/cpu_test.rng @@ -0,0 +1,48 @@ +<?xml version="1.0"?> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + <include href="cpu.rng"> + <start> + <choice> + <ref name="guestcpu"/> + <ref name="hostcpu"/> + <ref name="cpudata"/> + <ref name="cputest"/> + </choice> + </start> + </include> + + <define name="cpudata"> + <element name="cpudata"> + <attribute name="arch"><text/></attribute> + <oneOrMore> + <choice> + <element name="cpuid"> + <attribute name="eax_in"><ref name="hexuint"/></attribute> + <attribute name="ecx_in"><ref name="hexuint"/></attribute> + <attribute name="eax"><ref name="hexuint"/></attribute> + <attribute name="ebx"><ref name="hexuint"/></attribute> + <attribute name="ecx"><ref name="hexuint"/></attribute> + <attribute name="edx"><ref name="hexuint"/></attribute> + </element> + <element name="msr"> + <attribute name="eax"><ref name="hexuint"/></attribute> + <attribute name="edx"><ref name="hexuint"/></attribute> + <attribute name="index"><ref name="hexuint"/></attribute> + </element> + </choice> + </oneOrMore> + </element> + </define> + + <define name="cputest"> + <element name="cpuTest"> + <oneOrMore> + <choice> + <ref name="guestcpu"/> + <ref name="hostcpu"/> + </choice> + </oneOrMore> + </element> + </define> + +</grammar> diff --git a/docs/schemas/meson.build b/docs/schemas/meson.build index bb6a48787f..51c917d1ac 100644 --- a/docs/schemas/meson.build +++ b/docs/schemas/meson.build @@ -2,6 +2,7 @@ docs_schema_files = [ 'basictypes.rng', 'capability.rng', 'cpu.rng', + 'cpu_test.rng', 'cputypes.rng', 'domainbackup.rng', 'domaincaps.rng', diff --git a/tests/cputest.c b/tests/cputest.c index 90f319bf9c..e31c2c0820 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -81,7 +81,7 @@ cpuTestLoadXML(virArch arch, const char *name) if (!(doc = virXMLParseFileCtxt(xml, &ctxt))) goto cleanup; - virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu, false); + virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu, true); cleanup: xmlXPathFreeContext(ctxt); diff --git a/tests/virschematest.c b/tests/virschematest.c index 17eb2a4b34..fa7c64c0b8 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -198,6 +198,7 @@ mymain(void) } while (0) DO_TEST_DIR("capability.rng", "capabilityschemadata", "vircaps2xmldata"); + DO_TEST_DIR("cpu_test.rng", "cputestdata"); DO_TEST_DIR("domain.rng", "domainschemadata", "qemuxml2argvdata", "xmconfigdata", "qemuxml2xmloutdata", "lxcxml2xmldata", -- 2.26.2
On Wed, Oct 07, 2020 at 10:54:57 +0200, Tim Wiederhake wrote: Please always provide a short summary. E.g. that this is adding schema validation via virschematest with a custom schema to match the undescribed files.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/schemas/cpu_test.rng | 48 +++++++++++++++++++++++++++++++++++++++
This schema describes some internals which must NOT be part of the installed schema. As of such this not acceptable upstream. I will adapt your patch in my upcoming series which actually improves virschematest and this will come in very handy. NACK as we must not shipt the schema for internals.
docs/schemas/meson.build | 1 + tests/cputest.c | 2 +- tests/virschematest.c | 1 + 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 docs/schemas/cpu_test.rng
diff --git a/tests/cputest.c b/tests/cputest.c index 90f319bf9c..e31c2c0820 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -81,7 +81,7 @@ cpuTestLoadXML(virArch arch, const char *name) if (!(doc = virXMLParseFileCtxt(xml, &ctxt))) goto cleanup;
- virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu, false); + virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu, true);
This is mixing two changes at once. Adding schema validation via virschematest and via the integrated validator. Since I must NACK the bits for virschematest you'll need to re-home this hunk somewhere.
cleanup: xmlXPathFreeContext(ctxt);
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/manpages/virsh.rst | 9 ++++++--- tools/virsh-host.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index c855269041..8fee4c7afe 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -866,7 +866,7 @@ cpu-compare :: - cpu-compare FILE [--error] + cpu-compare FILE [--error] [--validate] Compare CPU definition from XML <file> with host CPU. (See ``hypervisor-cpu-compare`` command for comparing the CPU definition with the CPU @@ -882,7 +882,8 @@ the CPU definition. For more information on guest CPU definition see: `https://libvirt.org/formatdomain.html#elementsCPU <https://libvirt.org/formatdomain.html#elementsCPU>`__. If *--error* is specified, the command will return an error when the given CPU is incompatible with host CPU and a message providing more details about the -incompatibility will be printed out. +incompatibility will be printed out. If *--validate* is specified, validates +the format of the XML document against an internal RNG schema. cpu-models @@ -928,7 +929,7 @@ hypervisor-cpu-compare :: - hypervisor-cpu-compare FILE [virttype] [emulator] [arch] [machine] [--error] + hypervisor-cpu-compare FILE [virttype] [emulator] [arch] [machine] [--error] [--validate] Compare CPU definition from XML <file> with the CPU the hypervisor is able to provide on the host. (This is different from ``cpu-compare`` which compares the @@ -951,6 +952,8 @@ specifies the path to the emulator, *arch* specifies the CPU architecture, and *machine* specifies the machine type. If *--error* is specified, the command will return an error when the given CPU is incompatible with the host CPU and a message providing more details about the incompatibility will be printed out. +If *--validate* is specified, validates the format of the XML document against +an internal RNG schema. hypervisor-cpu-baseline diff --git a/tools/virsh-host.c b/tools/virsh-host.c index d4eb3f977d..cda2ef4ac1 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1214,6 +1214,10 @@ static const vshCmdOptDef opts_cpu_compare[] = { .type = VSH_OT_BOOL, .help = N_("report error if CPUs are incompatible") }, + {.name = "validate", + .type = VSH_OT_BOOL, + .help = N_("validate the XML document against schema") + }, {.name = NULL} }; @@ -1230,6 +1234,9 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "error")) flags |= VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE; + if (vshCommandOptBool(cmd, "validate")) + flags |= VIR_CONNECT_COMPARE_CPU_VALIDATE_XML; + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) return false; @@ -1636,6 +1643,10 @@ static const vshCmdOptDef opts_hypervisor_cpu_compare[] = { .type = VSH_OT_BOOL, .help = N_("report error if CPUs are incompatible") }, + {.name = "validate", + .type = VSH_OT_BOOL, + .help = N_("validate the XML document against schema") + }, {.name = NULL} }; @@ -1657,6 +1668,9 @@ cmdHypervisorCPUCompare(vshControl *ctl, if (vshCommandOptBool(cmd, "error")) flags |= VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE; + if (vshCommandOptBool(cmd, "validate")) + flags |= VIR_CONNECT_COMPARE_CPU_VALIDATE_XML; + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0 || vshCommandOptStringReq(ctl, cmd, "virttype", &virttype) < 0 || vshCommandOptStringReq(ctl, cmd, "emulator", &emulator) < 0 || -- 2.26.2
On Wed, Oct 07, 2020 at 10:54:58 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/manpages/virsh.rst | 9 ++++++--- tools/virsh-host.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Wed, Oct 07, 2020 at 10:54:53 +0200, Tim Wiederhake wrote:
We currently ignore unknown elements in the CPU XML description, e.g. with virsh cpu-compare and hypervisor-cpu-compare. This makes '<cpu><faeture name=3D"..."/></cpu>' (note the typo in "faeture") semantically identic to '<cpu/>'. No error is reported.
This series adds checks for unrecognized attributes and elements in the "<cpu>" element, catching this kind of mistake.
V1: https://www.redhat.com/archives/libvir-list/2020-September/msg00933.html V2: https://www.redhat.com/archives/libvir-list/2020-September/msg01073.html V3: https://www.redhat.com/archives/libvir-list/2020-September/msg01555.html
Changed: * Some patches from V3 are merged * Changes to commit messages * Perform testing by means of virschematest
Tim Wiederhake (5): schema: Make element "topology" in host CPU definition optional cpu: Wire in XML validation tests: Rename some test files in cputestdata tests: Enable CPU XML validation in the tests. virsh: Add "validate" argument to [hypervisor-]cpu-compare
Patches 2, 3, and 5 are pushed now.
participants (3)
-
Jiri Denemark -
Peter Krempa -
Tim Wiederhake