Re: [libvirt] [PATCH] qemu: Search binaries in PATH instead of hardcoding /usr/bin
2010/1/22 Matthias Bolte <matthias.bolte(a)googlemail.com>:
2010/1/22 Daniel P. Berrange <berrange(a)redhat.com>:
> 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
The common case is binary pointing to a QEMU binary and if KVM could
be used and is available kvmbin points to a KVM enabled QEMU binary.
So the paths a different in the common case. In the cleanup section
both strings need to be freed.
The special case is if binary is NULL but we find a KVM binary, then
binary is strdup'ed from kvmbin. Before this patch binary and kvmbin
were stack allocated and binary = kvmbin was okay. Now binary = kvmbin
would result in a double free with
VIR_FREE(binary);
VIR_FREE(kvmbin);
But you're right, I just thought about it and
binary = strdup(kvmbin);
could be changed back to
binary = kvmbin;
and the cleanup code could be changed to
if (binary == kvmbin) {
/* don't double free */
VIR_FREE(binary);
} else {
VIR_FREE(binary);
VIR_FREE(kvmbin);
}
to avoid a double free.
>> +
>> + 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
No leak here. The for loop can be left at 3 points:
- continue; if binary not found or not executable
- return -1; on OOM error
- break; on success
In both negative cases kvmbin is freed.
>> - 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
>
Version 2 of the patch is attached.
Matthias
Okay, pushed version 2.
Matthias