[libvirt] [PATCH v2] qemu: Fix using guest architecture as lookup key

[Daniel P. Berrange]

On Tue, Sep 15, 2015 at 04:44:13PM +0200, Andrea Bolognani wrote:
> When looking for a QEMU binary suitable for running ppc64le guests
> we have to take into account the fact that we use the QEMU target
> as key for the hash, so direct comparison is not good enough.
> 
> Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
> virQEMUCapsFindTarget() function and use that both when looking
> for QEMU binaries available on the system and when looking up
> QEMU capabilities later.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
> ---
>  src/qemu/qemu_capabilities.c | 100 ++++++++++++++++++++++++++++---------------
>  1 file changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 4ad1bdb..6757907 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -386,6 +386,28 @@ static const char *virQEMUCapsArchToString(virArch arch)
>      return virArchToString(arch);
>  }
>  
> +/* Given a host and guest architectures, find a suitable QEMU target.
> + *
> + * This is meant to be used as a second attempt if qemu-system-$guestarch
> + * can't be found, eg. on a x86_64 host you want to use qemu-system-i386,
> + * if available, instead of qemu-system-x86_64 to run i686 guests */
> +static virArch
> +virQEMUCapsFindTarget(virArch hostarch,
> +                      virArch guestarch)
> +{
> +    /* Both ppc64 and ppc64le guests can use the ppc64 target */
> +    if (ARCH_IS_PPC64(guestarch))
> +        guestarch = VIR_ARCH_PPC64;
> +
> +    /* armv7l guests on aarch64 hosts can use the aarch64 target
> +     * i686 guests on x86_64 hosts can use the x86_64 target */
> +    if ((guestarch == VIR_ARCH_ARMV7L && hostarch == VIR_ARCH_AARCH64) ||
> +        (guestarch == VIR_ARCH_I686 && hostarch == VIR_ARCH_X86_64)) {
> +        return hostarch;
> +    }
> +
> +    return guestarch;
> +}
>  
>  static virCommandPtr
>  virQEMUCapsProbeCommand(const char *qemu,
> @@ -690,51 +712,52 @@ virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid)
>      return ret;
>  }
>  
> -
>  static char *
> -virQEMUCapsFindBinaryForArch(virArch hostarch,
> -                             virArch guestarch)
> +virQEMUCapsFindBinary(const char *format,
> +                      const char *archstr)
>  {
> -    char *ret;
> -    const char *archstr;
> -    char *binary;
> -
> -    if (ARCH_IS_PPC64(guestarch))
> -        archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
> -    else
> -        archstr = virQEMUCapsArchToString(guestarch);
> +    char *ret = NULL;
> +    char *binary = NULL;
>  
> -    if (virAsprintf(&binary, "qemu-system-%s", archstr) < 0)
> -        return NULL;
> +    if (virAsprintf(&binary, format, archstr) < 0)
> +        goto out;
>  
>      ret = virFindFileInPath(binary);
>      VIR_FREE(binary);
> -    if (ret && !virFileIsExecutable(ret))
> -        VIR_FREE(ret);
> +    if (ret && virFileIsExecutable(ret))
> +        goto out;
>  
> -    if (guestarch == VIR_ARCH_ARMV7L &&
> -        !ret &&
> -        hostarch == VIR_ARCH_AARCH64) {
> -        ret = virFindFileInPath("qemu-system-aarch64");
> -        if (ret && !virFileIsExecutable(ret))
> -            VIR_FREE(ret);
> -    }
> +    VIR_FREE(ret);
>  
> -    if (guestarch == VIR_ARCH_I686 &&
> -        !ret &&
> -        hostarch == VIR_ARCH_X86_64) {
> -        ret = virFindFileInPath("qemu-system-x86_64");
> -        if (ret && !virFileIsExecutable(ret))
> -            VIR_FREE(ret);
> -    }
> + out:
> +    return ret;
> +}
> +
> +static char *
> +virQEMUCapsFindBinaryForArch(virArch hostarch,
> +                             virArch guestarch)
> +{
> +    char *ret = NULL;
> +    const char *archstr;
> +
> +    /* First attempt: try the guest architecture as it is */
> +    archstr = virQEMUCapsArchToString(guestarch);
> +    if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
> +        goto out;
>  
> -    if (guestarch == VIR_ARCH_I686 &&
> -        !ret) {
> -        ret = virFindFileInPath("qemu");
> -        if (ret && !virFileIsExecutable(ret))
> -            VIR_FREE(ret);
> +    /* Second attempt: use some arch-specific rules */
> +    archstr = virQEMUCapsArchToString(virQEMUCapsFindTarget(hostarch,
> +                                                            guestarch));

Nitpick, we could avoid the second search if virQEMUCapsFindTarget()
returns a value that == the original guestarch.

> +    if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
> +        goto out;
> +
> +    /* Third attempt, i686 only: try 'qemu' */
> +    if (guestarch == VIR_ARCH_I686) {
> +        if ((ret = virQEMUCapsFindBinary("%s", "qemu")) != NULL)
> +            goto out;
>      }
>  
> + out:
>      return ret;
>  }
>  
> @@ -3793,10 +3816,17 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr cache,
>  
>      virMutexLock(&cache->lock);
>      ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
> -    VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
> +    if (!ret) {
> +        /* If the first attempt at findind capabilities has failed, try
> +         * again using the QEMU target as lookup key instead */
> +        data.arch = virQEMUCapsFindTarget(virArchFromHost(), data.arch);
> +        ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
> +    }

Here too, skip the hash search if data.arch == data.arch from before.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|