Re: [libvirt PATCH] qemu: fix missing error reports in capabilities probing
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é
> > ---
> > 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 :|
Re: [libvirt PATCH] qemu: fix missing error reports in capabilities probing
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é
---
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?
Michal
Re: [libvirt PATCH] qemu: fix missing error reports in capabilities probing
On 6/15/20 10:56 AM, 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é --- Reviewed-by: Daniel Henrique Barboza
[libvirt PATCH] qemu: fix missing error reports in capabilities probing
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é
---
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_capabilities.c b/src/qemu/qemu_capabilities.c
index aa90eab229..448d6fa175 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5517,6 +5517,11 @@ virQEMUCapsCacheLookup(virFileCachePtr cache,
priv->microcodeVersion = virHostCPUGetMicrocodeVersion();
ret = virFileCacheLookup(cache, binary);
+if (!ret) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("no capabilities available for %s"), binary);
+return NULL;
+}
VIR_DEBUG("Returning caps %p for %s", ret, binary);
return ret;
@@ -5664,6 +5669,12 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
probedbinary = virQEMUCapsGetDefaultEmulator(hostarch, arch);
binary = probedbinary;
}
+if (!binary) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unable to find any emulator to serve '%s'
architecture"),
+ archStr);
+goto cleanup;
+}
if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
goto cleanup;
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;
}
--
2.26.2
