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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxx/message/BHYNL5IANIJWRYOTO77CRKHW57XTPVMC/
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.
