Re: [Qemu-devel] [PATCH] kvm: Add helpers for checking and requiring kvm extensions

[Avi Kivity <avi@redhat.com>]

Juan Quintela wrote:
> Avi Kivity <avi@redhat.com> wrote:
>
> Hi
>
>   
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 36659a9..1642a2a 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -64,6 +64,30 @@ struct KVMState
>>  
>>  static KVMState *kvm_state;
>>  
>> +int kvm_check_extension(int extension)
>> +{
>> +    int ret;
>> +
>> +    ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, extension);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM_CHECK_EXTENSION failed: %s\n", strerror(errno));
>> +        exit(1);
>> +    }
>> +    return ret;
>> +}
>>
>>     
> Are you sure you want the exit(1) in this case?
>   

A negative result of kvm_ioctl() means the ioctl itself failed.  This 
shouldn't happen, so we exit.

If the extension is not present, it returns 0.

> With the exit() call, you are unable to check if one extension is
> present at all.  And you check the return of the following code.
>   
>>      s->coalesced_mmio = 0;
>>  #ifdef KVM_CAP_COALESCED_MMIO
>> -    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
>> -    if (ret > 0)
>> -        s->coalesced_mmio = ret;
>> +    s->coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
>>  #endif
>>     
>
>   

Here we use the return value (0 or 1).

> You can remove the ifdef at this point.
>   

It won't compile if KVM_CAP_COLAESCED_MMIO is not defined.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.