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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> 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 :|
