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
--
|: 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 :|