3:54 a.m.
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
3:54 a.m.
New subject: [libvirt PATCH v4 1/5] schema: Make element "topology" in host CPU definition optional
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(a)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
4:08 a.m.
New subject: [libvirt PATCH v4 1/5] schema: Make element "topology" in host CPU definition optional
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(a)redhat.com>
---
5:29 a.m.
New subject: [libvirt PATCH v4 1/5] schema: Make element "topology" in host CPU definition optional
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(a)redhat.com>
3:17 a.m.
New subject: [libvirt PATCH v4 1/5] schema: Make element "topology" in host CPU definition optional
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(a)redhat.com>
Pushed now.
Jirka
3:54 a.m.
New subject: [libvirt PATCH v4 2/5] cpu: Wire in XML validation
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(a)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
4:23 a.m.
New subject: [libvirt PATCH v4 2/5] cpu: Wire in XML validation
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(a)redhat.com>
---
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:54 a.m.
New subject: [libvirt PATCH v4 3/5] tests: Rename some test files in cputestdata
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(a)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
4:19 a.m.
New subject: [libvirt PATCH v4 3/5] tests: Rename some test files in cputestdata
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(a)redhat.com>
---
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:54 a.m.
New subject: [libvirt PATCH v4 4/5] tests: Enable CPU XML validation in the tests.
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:05 a.m.
New subject: [libvirt PATCH v4 4/5] tests: Enable CPU XML validation in the tests.
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(a)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);
3:54 a.m.
New subject: [libvirt PATCH v4 5/5] virsh: Add "validate" argument to [hypervisor-]cpu-compare
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:25 a.m.
New subject: [libvirt PATCH v4 5/5] virsh: Add "validate" argument to [hypervisor-]cpu-compare
On Wed, Oct 07, 2020 at 10:54:58 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
docs/manpages/virsh.rst | 9 ++++++---
tools/virsh-host.c | 14 ++++++++++++++
2 files changed, 20 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
5:07 a.m.
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.
1508
days inactive
1509
days old
13 comments
3 participants
participants (3)
-
Jiri Denemark -
Peter Krempa -
Tim Wiederhake