[libvirt] [PATCH] qemu: Search binaries in PATH instead of hardcoding /usr/bin

[Daniel P. Berrange]

On Wed, Jan 20, 2010 at 02:46:10AM +0100, Matthias Bolte wrote:
> @@ -792,16 +797,30 @@ qemudCapsInitGuest(virCapsPtr caps,
>       */
>      if (STREQ(info->arch, hostmachine) ||
>          (STREQ(hostmachine, "x86_64") && STREQ(info->arch, "i686"))) {
> -        const char *const kvmbins[] = { "/usr/bin/qemu-kvm", /* Fedora */
> -                                        "/usr/bin/kvm" }; /* Upstream .spec */
> +        if (access("/dev/kvm", F_OK) == 0) {
> +            const char *const kvmbins[] = { "qemu-kvm", /* Fedora */
> +                                            "kvm" }; /* Upstream .spec */
> +
> +            for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
> +                kvmbin = virFindFileInPath(kvmbins[i]);
> +
> +                if (kvmbin == NULL || access(kvmbin, X_OK) != 0) {
> +                    VIR_FREE(kvmbin);
> +                    continue;
> +                }
>  
> -        for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
> -            if (access(kvmbins[i], X_OK) == 0 &&
> -                access("/dev/kvm", F_OK) == 0) {
>                  haskvm = 1;
> -                kvmbin = kvmbins[i];
> -                if (!binary)
> -                    binary = kvmbin;
> +
> +                if (binary == NULL) {
> +                    binary = strdup(kvmbin);

Why does this need to strdup(kvmbin), when virFindFileInPath() is
already returning a newly allocated string ?  Seems like we could
just avoid that 

> +
> +                    if (binary == NULL) {
> +                        virReportOOMError(NULL);
> +                        VIR_FREE(kvmbin);
> +                        return -1;
> +                    }
> +                }
> +
>                  break;
>              }

I think this loop is also leaking 'kvmbin', since it just
overrwrites 'kvmbin' on each iteration, the final cleanup
code will only free the last one that was allocated

> -                return -1;
> +                goto error;
>          }
>      }
>  
> +    VIR_FREE(binary);
> +    VIR_FREE(kvmbin);
> +
>      return 0;
> +
> +no_memory:
> +    virReportOOMError(NULL);
> +
> +error:
> +    VIR_FREE(binary);
> +    VIR_FREE(kvmbin);
> +    virCapabilitiesFreeMachines(machines, nmachines);
> +
> +    return -1;
>  }
>  


Generally the patch looks OK though

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|