1. Patchew
  2. Libvirt
  3. View series

[PATCH] util: open XML files before calling libxml2

Daniel P. Berrangé posted 1 patch 3 months, 3 weeks ago 
src/util/virxml.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
[PATCH] util: open XML files before calling libxml2
Posted by Daniel P. Berrangé 3 months, 3 weeks ago 
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
Re: [PATCH] util: open XML files before calling libxml2
Posted by Peter Krempa 3 months, 2 weeks ago 
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/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.
Re: [PATCH] util: open XML files before calling libxml2
Posted by Peter Krempa 3 months, 2 weeks ago 
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.
Re: [PATCH] util: open XML files before calling libxml2
Posted by Richard W.M. Jones 3 months, 3 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.