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

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