2010/1/22 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>:
> 2010/1/22 Daniel P. Berrange <berrange@xxxxxxxxxx>:
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list