1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH v2] qemu: fixing auto-detecting binary in domain capabilities

Daniel P. Berrangé posted 1 patch 4 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200117182401.547942-1-berrange@redhat.com
src/qemu/qemu_capabilities.c | 45 ++++++++++++++++++------------------
1 file changed, 22 insertions(+), 23 deletions(-)
[libvirt PATCH v2] qemu: fixing auto-detecting binary in domain capabilities
Posted by Daniel P. Berrangé 4 years, 3 months ago 
The virConnectGetDomainCapabilities API accepts either a binary path
to the emulator, or desired guest arch. If guest arch is not given,
then the host arch is assumed.

In the case where the binary is not given, the code tried to find the
emulator binary in the existing list of cached emulator capabilities.
This is not valid since we switched to lazy population of the cache in:

  commit 3dd91af01f30c5bda6328454ef49f3afece755d6
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Mon Dec 2 13:04:26 2019 +0000

    qemu: stop creating capabilities at driver startup

As a result of this change, if there are no persistent guests defined
using the requested guest architecture, virConnectGetDomainCapabilities
will fail to find an emulator binary.

The solution is to stop relying on the cached capabilities to find the
binary and instead use the same logic we use to pick default a binary
per arch when populating capabilities.

Tested-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_capabilities.c | 45 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 498348ad58..14c2aa34d2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5280,10 +5280,13 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
                               const char **retMachine)
 {
     int virttype = VIR_DOMAIN_VIRT_NONE;
-    int arch = virArchFromHost();
+    virArch hostarch = virArchFromHost();
+    virArch arch = hostarch;
     virDomainVirtType capsType;
     virQEMUCapsPtr qemuCaps = NULL;
     virQEMUCapsPtr ret = NULL;
+    virArch arch_from_caps;
+    g_autofree char *probedbinary = NULL;
 
     if (virttypeStr &&
         (virttype = virDomainVirtTypeFromString(virttypeStr)) < 0) {
@@ -5299,31 +5302,27 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
         goto cleanup;
     }
 
-    if (binary) {
-        virArch arch_from_caps;
+    if (!binary) {
+        probedbinary = virQEMUCapsGetDefaultEmulator(hostarch, arch);
+        binary = probedbinary;
+    }
 
-        if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
-            goto cleanup;
+    if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
+        goto cleanup;
 
-        arch_from_caps = virQEMUCapsGetArch(qemuCaps);
+    arch_from_caps = virQEMUCapsGetArch(qemuCaps);
 
-        if (arch_from_caps != arch &&
-            !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) ||
-              (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) ||
-              (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) ||
-              (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("architecture from emulator '%s' doesn't "
-                             "match given architecture '%s'"),
-                           virArchToString(arch_from_caps),
-                           virArchToString(arch));
-            goto cleanup;
-        }
-    } else {
-        if (!(qemuCaps = virQEMUCapsCacheLookupByArch(cache, arch)))
-            goto cleanup;
-
-        binary = virQEMUCapsGetBinary(qemuCaps);
+    if (arch_from_caps != arch &&
+        !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) ||
+          (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) ||
+          (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) ||
+          (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("architecture from emulator '%s' doesn't "
+                         "match given architecture '%s'"),
+                       virArchToString(arch_from_caps),
+                       virArchToString(arch));
+        goto cleanup;
     }
 
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
-- 
2.23.0
Re: [libvirt PATCH v2] qemu: fixing auto-detecting binary in domain capabilities
Posted by Boris Fiuczynski 4 years, 3 months ago 
Looks ok and also fixes on my system the problem.
Tested-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 1/17/20 7:24 PM, Daniel P. Berrangé wrote:
> The virConnectGetDomainCapabilities API accepts either a binary path
> to the emulator, or desired guest arch. If guest arch is not given,
> then the host arch is assumed.
> 
> In the case where the binary is not given, the code tried to find the
> emulator binary in the existing list of cached emulator capabilities.
> This is not valid since we switched to lazy population of the cache in:
> 
>    commit 3dd91af01f30c5bda6328454ef49f3afece755d6
>    Author: Daniel P. Berrangé <berrange@redhat.com>
>    Date:   Mon Dec 2 13:04:26 2019 +0000
> 
>      qemu: stop creating capabilities at driver startup
> 
> As a result of this change, if there are no persistent guests defined
> using the requested guest architecture, virConnectGetDomainCapabilities
> will fail to find an emulator binary.
> 
> The solution is to stop relying on the cached capabilities to find the
> binary and instead use the same logic we use to pick default a binary
> per arch when populating capabilities.
> 
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/qemu/qemu_capabilities.c | 45 ++++++++++++++++++------------------
>   1 file changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 498348ad58..14c2aa34d2 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5280,10 +5280,13 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
>                                 const char **retMachine)
>   {
>       int virttype = VIR_DOMAIN_VIRT_NONE;
> -    int arch = virArchFromHost();
> +    virArch hostarch = virArchFromHost();
> +    virArch arch = hostarch;
>       virDomainVirtType capsType;
>       virQEMUCapsPtr qemuCaps = NULL;
>       virQEMUCapsPtr ret = NULL;
> +    virArch arch_from_caps;
> +    g_autofree char *probedbinary = NULL;
>   
>       if (virttypeStr &&
>           (virttype = virDomainVirtTypeFromString(virttypeStr)) < 0) {
> @@ -5299,31 +5302,27 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
>           goto cleanup;
>       }
>   
> -    if (binary) {
> -        virArch arch_from_caps;
> +    if (!binary) {
> +        probedbinary = virQEMUCapsGetDefaultEmulator(hostarch, arch);
> +        binary = probedbinary;
> +    }
>   
> -        if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
> -            goto cleanup;
> +    if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
> +        goto cleanup;
>   
> -        arch_from_caps = virQEMUCapsGetArch(qemuCaps);
> +    arch_from_caps = virQEMUCapsGetArch(qemuCaps);
>   
> -        if (arch_from_caps != arch &&
> -            !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) ||
> -              (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) ||
> -              (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) ||
> -              (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) {
> -            virReportError(VIR_ERR_INVALID_ARG,
> -                           _("architecture from emulator '%s' doesn't "
> -                             "match given architecture '%s'"),
> -                           virArchToString(arch_from_caps),
> -                           virArchToString(arch));
> -            goto cleanup;
> -        }
> -    } else {
> -        if (!(qemuCaps = virQEMUCapsCacheLookupByArch(cache, arch)))
> -            goto cleanup;
> -
> -        binary = virQEMUCapsGetBinary(qemuCaps);
> +    if (arch_from_caps != arch &&
> +        !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) ||
> +          (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) ||
> +          (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) ||
> +          (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("architecture from emulator '%s' doesn't "
> +                         "match given architecture '%s'"),
> +                       virArchToString(arch_from_caps),
> +                       virArchToString(arch));
> +        goto cleanup;
>       }
>   
>       if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [libvirt PATCH v2] qemu: fixing auto-detecting binary in domain capabilities
Posted by Michal Privoznik 4 years, 3 months ago 
On 1/17/20 7:24 PM, Daniel P. Berrangé wrote:
> The virConnectGetDomainCapabilities API accepts either a binary path
> to the emulator, or desired guest arch. If guest arch is not given,
> then the host arch is assumed.
> 
> In the case where the binary is not given, the code tried to find the
> emulator binary in the existing list of cached emulator capabilities.
> This is not valid since we switched to lazy population of the cache in:
> 
>    commit 3dd91af01f30c5bda6328454ef49f3afece755d6
>    Author: Daniel P. Berrangé <berrange@redhat.com>
>    Date:   Mon Dec 2 13:04:26 2019 +0000
> 
>      qemu: stop creating capabilities at driver startup
> 
> As a result of this change, if there are no persistent guests defined
> using the requested guest architecture, virConnectGetDomainCapabilities
> will fail to find an emulator binary.
> 
> The solution is to stop relying on the cached capabilities to find the
> binary and instead use the same logic we use to pick default a binary
> per arch when populating capabilities.
> 
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/qemu/qemu_capabilities.c | 45 ++++++++++++++++++------------------
>   1 file changed, 22 insertions(+), 23 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Re: [libvirt PATCH v2] qemu: fixing auto-detecting binary in domain capabilities
Posted by Boris Fiuczynski 4 years, 3 months ago 
On 1/17/20 7:24 PM, Daniel P. Berrangé wrote:
> The virConnectGetDomainCapabilities API accepts either a binary path
> to the emulator, or desired guest arch. If guest arch is not given,
> then the host arch is assumed.

I forgot to mention that virConnectBaselineHypervisorCPU and 
virConnectCompareHypervisorCPU APIs are effected in the same way.

> 
> In the case where the binary is not given, the code tried to find the
> emulator binary in the existing list of cached emulator capabilities.
> This is not valid since we switched to lazy population of the cache in:
> 
>    commit 3dd91af01f30c5bda6328454ef49f3afece755d6
>    Author: Daniel P. Berrangé <berrange@redhat.com>
>    Date:   Mon Dec 2 13:04:26 2019 +0000
> 
>      qemu: stop creating capabilities at driver startup
> 
> As a result of this change, if there are no persistent guests defined
> using the requested guest architecture, virConnectGetDomainCapabilities
> will fail to find an emulator binary.
> 
> The solution is to stop relying on the cached capabilities to find the
> binary and instead use the same logic we use to pick default a binary
> per arch when populating capabilities.
> 
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
...
-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.