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