[libvirt PATCH] qemu: fix missing error reports in capabilities probing

[Daniel P. Berrangé]

On Mon, Jul 13, 2020 at 09:44:07PM +0200, Michal Privoznik wrote:
> On 6/15/20 3:56 PM, Daniel P. Berrangé wrote:
> > The "virsh domcapabilities --arch ppc64" command will fail with no
> > error message set if qemu-system-ppc64 is not currently installed.
> > 
> > This is because virQEMUCapsCacheLookup() does not report any error
> > message if not capabilities can be obtained from the cache. Almost
> > all methods calling this expected an error to be set on failure.
> > 
> > Once that's fixed though, we see a further bug which is that
> > virQEMUCapsCacheLookupDefault() is passing a NULL binary path to
> > virQEMUCapsCacheLookup(), so we need to catch that too.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >   src/qemu/qemu_capabilities.c | 11 +++++++++++
> >   src/qemu/qemu_domain.c       |  4 +++-
> >   2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> 
> 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 2dad823a86..97096073e6 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -6068,8 +6068,10 @@ qemuDomainPostParseDataAlloc(const virDomainDef *def,
> >       virQEMUDriverPtr driver = opaque;
> >       if (!(*parseOpaque = virQEMUCapsCacheLookup(driver->qemuCapsCache,
> > -                                                def->emulator)))
> > +                                                def->emulator))) {
> > +        virResetLastError();
> >           return 1;
> > +    }
> >       return 0;
> >   }
> > 
> 
> I think we need to revisit this patch. In my testing, I have qemu built on a
> side from git and one domain that runs it. As I updated my system, but not
> rebuilt the qemu from git it no longer runs (fails to link):
> 
> ~/work/qemu/qemu.git/x86_64-softmmu/qemu-system-x86_64: error while loading
> shared libraries: libnettle.so.7: cannot open shared object file: No such
> file or directory
> 
> This is expected. But, virsh start fails with:
> 
> error: Failed to start domain fedora
> error: An error occurred, but the cause is unknown
> 
> I've tracked this down to the virResetLastError() in the hunk above. And it
> kind of makes sense - we failed to load capabilities on daemon startup (oh,
> yeah, daemon runs from git too, but it's been rebuilt) so we try again on
> domain startup. But now that I am reading the commit message it doesn't make
> much sense to me either. 'virsh domcapabilities' has nothing to do with
> PostParse callbacks, does it? Can it be that this error reset call is just
> misplaced?

I was attempting to preserve what I thought was existing behaviour.

I was seeing that  virFileCacheLookup returned NULL, but didn't set
any error. So I added an error report in virQEMUCapsCacheLookup,
and thus in reurn clearer the eror in virQEMUCapsCacheLookup.

It looks like i was mistaken about virFileCacheLookup tough - it
does indeed set an error, but not in all cases. So I thin we need
to revert part of my commit.

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