[libvirt] [PATCH] report invalid x86 cpu map error

[Daniel P. Berrangé]

On Tue, Jan 15, 2019 at 03:53:42PM +0100, Peter Krempa wrote:
> On Tue, Jan 15, 2019 at 15:42:41 +0100, Jiri Denemark wrote:
> > On Mon, Jan 14, 2019 at 15:40:32 +0000, Daniel P. Berrangé wrote:
> > > On Mon, Jan 14, 2019 at 04:15:01PM +0100, Jiri Denemark wrote:
> > > > On Mon, Jan 14, 2019 at 15:03:26 +0000, Daniel P. Berrangé wrote:
> > > > > On Mon, Jan 14, 2019 at 03:36:38PM +0100, Jiri Denemark wrote:
> > > > > > On Mon, Jan 14, 2019 at 14:21:42 +0000, Daniel P. Berrangé wrote:
> > > > > > > On Mon, Jan 14, 2019 at 02:56:43PM +0100, Jiri Denemark wrote:
> > > > > > > > On Mon, Jan 14, 2019 at 20:07:34 +0800, zhenwei pi wrote:
> 
> [...]
> 
> > > > > > virsh # cpu-models x86_64
> > > > > > error: failed to get CPU model names
> > > > > > error: Storage volume not found: no storage vol with matching path
> > > > > > '/vm/systemrescuecd-x86-2.5.1.iso'
> > > > > > 
> > > > > > So there's definitely a bug to be fixed somewhere.
> > > > > 
> > > > > Hmm, what steps did you use to get into that situation ?  It works as
> > > > > expected for me, re-reporting the original error.
> > > > 
> > > > I just moved away the CPU map file. But it appears I have a domain
> > > > defined pointing to a non-existent disk image. It seems the problem
> > > > shows up when multiple errors are raised when libvirtd is starting up.
> > > 
> > > Can you elaborate on that - what guest  & storage pool config do you
> > > have.  I'm struggling to see how to exercise the codepath that reports
> > > that error during startup.
> > 
> > While I had a domain pointing to an inexistent disk image, it did not
> > actually cause the strange error reported by virCPUx86GetMap. This is
> > what caused the error to be reported: I have a directory storage pool in
> > /vm, which is autostarted on libvirtd startup. That directory contained
> > sysres1.qcow file with systemrescuecd-x86-2.5.1.iso backing file. But
> > the backing file was missing. The storage driver refreshes all active
> > pools and once it gets to sysres1.qcow, virStorageBackendVolOpen fails
> > with "Storage volume not found: no storage vol with matching path
> > '/vm/systemrescuecd-x86-2.5.1.iso'".
> 
> And the code loading the map ends up calling virXMLParseHelper() which
> calls xmlCtxtReadFile. On failure it jumps to the 'error' label with the
> following code:
> 
>     if (virGetLastErrorCode() == VIR_ERR_OK) {
>         virGenericReportError(domcode, VIR_ERR_XML_ERROR,
>                               _("failed to parse xml document '%s'"),
>                               filename ? filename : "[inline data]");
>     }
> 
> So it looks like the previous error was not cleared and thus the XML
> parser does not raise it's own error.

Ahhh. Horrible. The earlier catchXMLError method is also broken, as
it does

    /* conditions for error printing */
    if (!ctxt ||
        (virGetLastErrorCode()) ||
        ctxt->input == NULL ||
        ctxt->lastError.level != XML_ERR_FATAL ||
        ctxt->lastError.message == NULL)
        return;



> The error then gets remembered forever in the virOnce infrastructure
> which actually works as intended.
> 
> I'm not sure whether there's any sane reason for the XML parser to avoid
> overwriting an error.

The code you quote shouldn't overwrite an error set by catchXMLError.

I think we need to just call virResetError at the start of the
virXMLParseHelper method.

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