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

[Matthias Bolte]

2010/1/22 Matthias Bolte <matthias.bolte at googlemail.com>:
> 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
>

Okay, pushed version 2.

Matthias