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

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt; I can't remember which (of probably many) bugs this relates to, but maybe one should be mentioned in the commit.