Re: [PATCH v3 2/4] KVM: Add unified KVM_GET/SET_VCPU_STATE IOCTL

[Avi Kivity <avi@redhat.com>]

On 11/02/2009 06:20 PM, Jan Kiszka wrote:
> Add a new IOCTL pair to retrieve or set the VCPU state in one chunk.
> More precisely, the IOCTL is able to process a list of substates to be
> read or written. This list is easily extensible without breaking the
> existing ABI, thus we will no longer have to add new IOCTLs when we
> discover a missing VCPU state field or want to support new hardware
> features.
>    

I'm having some second thoughts about this.

What does the new API buy us?  Instead of declaring two new ioctls for 
new/fixed substates, we only have to declare one.  We still have the 
capability check.  We still have to declare a structure.

It's true that the internals are currently a mess.  We can fix that with 
a table-driven approach:

static struct kvm_state_ioctl state_ioctls[] = {
    {
         .get_ioctl = KVM_GET_FPU,
         .set_ioctl = KVM_SET_FPU,
         .get = kvm_get_fpu,
         .set = kvm_set_fpu,
         .size = sizeof(struct kvm_fpu), /* 0 for variable-size state */
     },
};

(btw, the new stuff would also benefit from this).

So, what's the real justification for the new ABI?

Jan, my apologies for raising this at such a very late stage in the 
review, after all the nits have been satisfactorily addressed.  But I 
want to make sure we don't bloat the interface without very good reasons.

-- 
error compiling committee.c: too many arguments to function