[PATCH] util: open XML files before calling libxml2
Libxml2 has awful error reporting behaviour when reading files. When we fail to load a file from the test driver we see: $ virsh -c test:///wibble.xml I/O warning : failed to load external entity "/wibble.xml" error: failed to connect to the hypervisor error: XML error: failed to parse xml document '/wibble.xml' where the I/O warning line is something printed by libxml2 itself, which also lacks any useful detail. Switching to our own file reading code we can massively improve things: $ ./build/tools/virsh -c test:///wibble.xml error: failed to connect to the hypervisor error: Failed to open file '/wibble.xml': No such file or directory Using 10 MB as an upper limit on XML file size ought to be sufficient for any XML files libvirt is reading. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virxml.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index a7b75fd7b3..f6b937b277 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1133,6 +1133,7 @@ virXMLParseHelper(int domcode, xmlNodePtr rootnode; const char *docname; int parseFlags = XML_PARSE_NONET | XML_PARSE_NOWARNING; + g_autofree char *xmlStrPtr = NULL; if (filename) docname = filename; @@ -1155,10 +1156,11 @@ virXMLParseHelper(int domcode, } if (filename) { - xml = xmlCtxtReadFile(pctxt, filename, NULL, parseFlags); - } else { - xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags); + if (virFileReadAll(filename, 1024*1024*10, &xmlStrPtr) < 0) + return NULL; + xmlStr = xmlStrPtr; } + xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags); if (!xml) { if (virGetLastErrorCode() == VIR_ERR_OK) { -- 2.45.2
On Wed, Aug 07, 2024 at 03:59:52PM +0100, Daniel P. Berrangé wrote:
Libxml2 has awful error reporting behaviour when reading files. When we fail to load a file from the test driver we see:
$ virsh -c test:///wibble.xml I/O warning : failed to load external entity "/wibble.xml" error: failed to connect to the hypervisor error: XML error: failed to parse xml document '/wibble.xml'
where the I/O warning line is something printed by libxml2 itself, which also lacks any useful detail.
Switching to our own file reading code we can massively improve things:
$ ./build/tools/virsh -c test:///wibble.xml error: failed to connect to the hypervisor error: Failed to open file '/wibble.xml': No such file or directory
Using 10 MB as an upper limit on XML file size ought to be sufficient for any XML files libvirt is reading.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virxml.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c index a7b75fd7b3..f6b937b277 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1133,6 +1133,7 @@ virXMLParseHelper(int domcode, xmlNodePtr rootnode; const char *docname; int parseFlags = XML_PARSE_NONET | XML_PARSE_NOWARNING; + g_autofree char *xmlStrPtr = NULL;
if (filename) docname = filename; @@ -1155,10 +1156,11 @@ virXMLParseHelper(int domcode, }
if (filename) { - xml = xmlCtxtReadFile(pctxt, filename, NULL, parseFlags); - } else { - xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags); + if (virFileReadAll(filename, 1024*1024*10, &xmlStrPtr) < 0) + return NULL; + xmlStr = xmlStrPtr; } + xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags);
if (!xml) { if (virGetLastErrorCode() == VIR_ERR_OK) {
Seems sensible: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Aug 07, 2024 at 15:59:52 +0100, Daniel P. Berrangé wrote:
Libxml2 has awful error reporting behaviour when reading files. When we fail to load a file from the test driver we see:
$ virsh -c test:///wibble.xml I/O warning : failed to load external entity "/wibble.xml" error: failed to connect to the hypervisor error: XML error: failed to parse xml document '/wibble.xml'
where the I/O warning line is something printed by libxml2 itself, which also lacks any useful detail.
Switching to our own file reading code we can massively improve things:
$ ./build/tools/virsh -c test:///wibble.xml error: failed to connect to the hypervisor error: Failed to open file '/wibble.xml': No such file or directory
Using 10 MB as an upper limit on XML file size ought to be sufficient for any XML files libvirt is reading.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virxml.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c index a7b75fd7b3..f6b937b277 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1133,6 +1133,7 @@ virXMLParseHelper(int domcode, xmlNodePtr rootnode; const char *docname; int parseFlags = XML_PARSE_NONET | XML_PARSE_NOWARNING; + g_autofree char *xmlStrPtr = NULL;
if (filename) docname = filename; @@ -1155,10 +1156,11 @@ virXMLParseHelper(int domcode, }
if (filename) { - xml = xmlCtxtReadFile(pctxt, filename, NULL, parseFlags); - } else { - xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags); + if (virFileReadAll(filename, 1024*1024*10, &xmlStrPtr) < 0) + return NULL; + xmlStr = xmlStrPtr; } + xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, url, NULL, parseFlags);
if (!xml) { if (virGetLastErrorCode() == VIR_ERR_OK) {
With my patch: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/BHYN... which adds some test cases relevant to this change applied I see following changes: diff --git a/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err b/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err index 59e92917f9..9a6e2ef94b 100644 --- a/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err +++ b/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err @@ -1 +1 @@ -XML error: failed to parse xml document '/home/pipo/libvirt/tests/qemuxmlconfdata/nonexistent-file.xml' +Failed to open file '/home/pipo/libvirt/tests/qemuxmlconfdata/nonexistent-file.xml': No such file or directory While this improves things ... diff --git a/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err index 601f547cc6..77fe29cff0 100644 --- a/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err +++ b/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err @@ -1,3 +1,3 @@ -/home/pipo/libvirt/tests/qemuxmlconfdata/broken-xml-invalid.xml:2: Couldn't find end of Start Tag dom line 1 +(domain_definition):2: Couldn't find end of Start Tag dom line 1 (null) ^ This is in turn a significant regression in error message quality as it no longer references the filename.
On Thu, Aug 08, 2024 at 09:29:41 +0200, Peter Krempa wrote:
On Wed, Aug 07, 2024 at 15:59:52 +0100, Daniel P. Berrangé wrote:
[...]
diff --git a/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err index 601f547cc6..77fe29cff0 100644 --- a/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err +++ b/tests/qemuxmlconfdata/broken-xml-invalid.x86_64-latest.err @@ -1,3 +1,3 @@ -/home/pipo/libvirt/tests/qemuxmlconfdata/broken-xml-invalid.xml:2: Couldn't find end of Start Tag dom line 1 +(domain_definition):2: Couldn't find end of Start Tag dom line 1 (null) ^
This is in turn a significant regression in error message quality as it no longer references the filename.
Forgot to add that the '(domain_definition)' is an arbitrary string that libvirt passes to libxml so unless libxml would try to interpret it somehow it should be an easy fix.
participants (3)
-
Daniel P. Berrangé -
Peter Krempa -
Richard W.M. Jones