Re: [PATCH 3/4] kvm: Add kvm_has_pit_state2 helper

[Juan Quintela <quintela@redhat.com>]

Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> To be used for in-kernel PIT emulation.

....

> +    int pit_state2;

This is used as a bool.

>      int xsave, xcrs;
>      int many_ioeventfds;
>      int irqchip_inject_ioctl;
> @@ -954,6 +955,10 @@ int kvm_init(void)
>      s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
>  #endif
>  
> +#ifdef KVM_CAP_PIT_STATE2
> +    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
> +#endif
> +

[ this happened to me when I was reviewing this patch, but culprit is
not this patch]

really kvm_check_extension() should also return a bool, but that is a
bigger change that this patch series tend to introduce.

So, I went to "man ioctl"

> RETURN VALUE
>        Usually, on success zero is returned.  A few ioctl() requests  use  the
>        return  value  as an output parameter and return a nonnegative value on
>       success.  On error, -1 is returned, and errno is set appropriately.

Usually is the important word there.

And then went to kvm-all.c

int kvm_check_extension(KVMState *s, unsigned int extension)
{
    int ret;

    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, extension);
    if (ret < 0) {
        ret = 0;
    }

    return ret;
}

What? it allways return zero?  Something should be wrong here.  I will
expect kvm_check_extension() to work by now.

Yes, kvm_ioctl() return 1 went the extension is there, just for the
people confused like me.

Later, Juan.