From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] kvm: Add helpers for checking and requiring
 kvm extensions
Date: Mon, 04 May 2009 09:57:55 +0300
Message-ID: <49FE91F3.8070806@redhat.com>
References: <1241350448-2340-1-git-send-email-avi@redhat.com> <m38wldmkgu.fsf@neno.mitica>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Anthony Liguori <anthony@codemonkey.ws>, qemu-devel@nongnu.org,
	kvm@vger.kernel.org
To: Juan Quintela <quintela@trasno.org>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:36491 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751860AbZEDG6f (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 4 May 2009 02:58:35 -0400
In-Reply-To: <m38wldmkgu.fsf@neno.mitica>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.