1. Patchew
  2. Libvirt
  3. [PATCH v2 0/3] util/xml: make xml parser error reporting less terrible
  4. View patch

[PATCH v2 3/3] util/xml: open XML files before calling libxml2

Daniel P. Berrangé posted 3 patches 1 year, 3 months ago
[PATCH v2 3/3] util/xml: open XML files before calling libxml2
Posted by Daniel P. Berrangé 1 year, 3 months 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 +++++---
 tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 797aa6f6ca..deed258bb0 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -1142,6 +1142,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;
@@ -1165,10 +1166,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) {
diff --git a/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err b/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err
index 0ddf1ea510..2aedf3eded 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 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml'
+Failed to open file 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml': No such file or directory
-- 
2.46.0
Re: [PATCH v2 3/3] util/xml: open XML files before calling libxml2
Posted by Richard W.M. Jones 1 year, 3 months ago 
On Tue, Oct 22, 2024 at 10:27:54AM +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 +++++---
>  tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 797aa6f6ca..deed258bb0 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -1142,6 +1142,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;
> @@ -1165,10 +1166,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) {
> diff --git a/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err b/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err
> index 0ddf1ea510..2aedf3eded 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 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml'
> +Failed to open file 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml': No such file or directory

I know it doesn't matter as the file is literally not there, but isn't
that supposed to be @ABS_SRCDIR@ or similar?

Anyway the series looks good, so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

I can't remember which (of probably many) bugs this relates to, but
maybe one should be mentioned in the commit.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [PATCH v2 3/3] util/xml: open XML files before calling libxml2
Posted by Daniel P. Berrangé 1 year, 3 months ago 
On Tue, Oct 22, 2024 at 10:33:43AM +0100, Richard W.M. Jones wrote:
> On Tue, Oct 22, 2024 at 10:27:54AM +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 +++++---
> >  tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err | 2 +-
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/util/virxml.c b/src/util/virxml.c
> > index 797aa6f6ca..deed258bb0 100644
> > --- a/src/util/virxml.c
> > +++ b/src/util/virxml.c
> > @@ -1142,6 +1142,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;
> > @@ -1165,10 +1166,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) {
> > diff --git a/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err b/tests/qemuxmlconfdata/nonexistent-file.x86_64-latest.err
> > index 0ddf1ea510..2aedf3eded 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 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml'
> > +Failed to open file 'ABS_SRCDIR/qemuxmlconfdata/nonexistent-file.xml': No such file or directory
> 
> I know it doesn't matter as the file is literally not there, but isn't
> that supposed to be @ABS_SRCDIR@ or similar?

No, for some reason in our test code we replace the literal
string "ABS_SRCDIR" without the common markers. I guess it
was unique enough, in practice, for tests.

> Anyway the series looks good, so:
> 
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 
> I can't remember which (of probably many) bugs this relates to, but
> maybe one should be mentioned in the commit.

It was something that libguestfs hit back in august IIRC but I couldn't
find the details.

With 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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.