3:40 a.m.
XML schema validation for `virsh (hypervisor-)cpu-compare` has to be
enabled explicitly by passing the `--validate` flag. Having invalid
XML domain / cpu specification that appear to work can lead to hard
to find problems down the line, when e.g. migration of VMs does not
work as expected.
This series fixes a bug in the validation code and logs the schema
validation error to libvirtd's log file. User facing behaviour stays
unchanged.
See this conversation for more background:
https://listman.redhat.com/archives/libvir-list/2021-March/msg01214.html
Tim Wiederhake (2):
virxml: Fix schema validation of individual nodes
virCPUDefParseXML: Log schema validation errors
src/conf/cpu_conf.c | 16 +++++++---------
src/util/virxml.c | 13 ++++++-------
src/util/virxml.h | 1 -
3 files changed, 13 insertions(+), 17 deletions(-)
--
2.26.2
3:40 a.m.
New subject: [libvirt PATCH 1/2] virxml: Fix schema validation of individual nodes
xmlDocSetRootElement removes the node from its previous document tree,
effectively removing the "<cpu>" node from "<domain>" in
virCPUDefParseXML.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/util/virxml.c | 13 ++++++-------
src/util/virxml.h | 1 -
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 117f50f2bf..d816a8e08d 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -1285,16 +1285,15 @@ virXMLValidateAgainstSchema(const char *schemafile,
int
-virXMLValidateNodeAgainstSchema(const char *schemafile,
- xmlDocPtr doc,
- xmlNodePtr node)
+virXMLValidateNodeAgainstSchema(const char *schemafile, xmlNodePtr node)
{
- xmlNodePtr root;
int ret;
+ xmlDocPtr copy = xmlNewDoc(NULL);
- root = xmlDocSetRootElement(doc, node);
- ret = virXMLValidateAgainstSchema(schemafile, doc);
- xmlDocSetRootElement(doc, root);
+ xmlDocSetRootElement(copy, xmlCopyNode(node, true));
+ ret = virXMLValidateAgainstSchema(schemafile, copy);
+
+ xmlFreeDoc(copy);
return ret;
}
diff --git a/src/util/virxml.h b/src/util/virxml.h
index de171dce12..8a8ec2a6df 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -213,7 +213,6 @@ virXMLValidateAgainstSchema(const char *schemafile,
int
virXMLValidateNodeAgainstSchema(const char *schemafile,
- xmlDocPtr doc,
xmlNodePtr node);
void
--
2.26.2
3:40 a.m.
New subject: [libvirt PATCH 2/2] virCPUDefParseXML: Log schema validation errors
Schema validation was only performed if the "validateXML" flag was set.
Make invalid XML visible in the logs by always validating, but only treat
validation errors as errors if said flag is set.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/cpu_conf.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index c095ab0e89..c584b0f360 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -329,6 +329,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
VIR_XPATH_NODE_AUTORESTORE(ctxt)
int n;
size_t i;
+ g_autofree char *schemafile = NULL;
g_autofree char *cpuMode = NULL;
g_autofree char *fallback = NULL;
g_autofree char *vendor_id = NULL;
@@ -347,16 +348,13 @@ 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 (!(schemafile = virFileFindResource("cpu.rng",
+ abs_top_srcdir "/docs/schemas",
+ PKGDATADIR "/schemas")))
+ return -1;
- if (virXMLValidateNodeAgainstSchema(schemafile, ctxt->doc,
- ctxt->node) < 0)
+ if (virXMLValidateNodeAgainstSchema(schemafile, ctxt->node) < 0) {
+ if (validateXML)
return -1;
}
--
2.26.2
4:14 a.m.
On Thu, Apr 15, 2021 at 10:40:52AM +0200, Tim Wiederhake wrote:
XML schema validation for `virsh (hypervisor-)cpu-compare` has to be
enabled explicitly by passing the `--validate` flag. Having invalid
XML domain / cpu specification that appear to work can lead to hard
to find problems down the line, when e.g. migration of VMs does not
work as expected.
This series fixes a bug in the validation code and logs the schema
validation error to libvirtd's log file. User facing behaviour stays
unchanged.
I'm sceptical this is going to be beneficial. RNG validation error
messages can be obscure at the best of times. I almost always need
to look at the original input XML to understand what the RNG error
really means. IOW just having the RNG error logged is unlikely to
be enough to help people see what went wrong, assuming anyone even
pays attention to the log.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:25 a.m.
On Thu, Apr 15, 2021 at 10:14:02 +0100, Daniel P. Berrangé wrote:
On Thu, Apr 15, 2021 at 10:40:52AM +0200, Tim Wiederhake wrote:
> XML schema validation for `virsh (hypervisor-)cpu-compare` has to be
> enabled explicitly by passing the `--validate` flag. Having invalid
> XML domain / cpu specification that appear to work can lead to hard
> to find problems down the line, when e.g. migration of VMs does not
> work as expected.
>
> This series fixes a bug in the validation code and logs the schema
> validation error to libvirtd's log file. User facing behaviour stays
> unchanged.
I'm sceptical this is going to be beneficial. RNG validation error
messages can be obscure at the best of times. I almost always need
to look at the original input XML to understand what the RNG error
really means. IOW just having the RNG error logged is unlikely to
be enough to help people see what went wrong, assuming anyone even
pays attention to the log.
And in addition to that (now speaking generally about also other APIs
which take XML and have voluntary validation) users can have a working
XML for their use case which may be invalid.
Spamming the logs in such case may be a double edged sword; in some
cases users might fix the XML if they notice something's wrong or in
other cases they may get their logs spammed with a irrelevant error.
1317
days inactive
1317
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Peter Krempa -
Tim Wiederhake