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

[Matthias Bolte]

2010/1/22 Daniel P. Berrange <berrange at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qemu-Search-binaries-in-PATH-instead-of-hardcoding_v2.diff
Type: text/x-diff
Size: 10507 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100122/857c2af1/attachment-0001.bin>