Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()

[Avi Kivity <avi@qumranet.com>]

Davide Libenzi wrote:
>> I think that may be a bit cleaner than Al's approach, but it still
>> leaves the same trap that create_vcpu_fd() falls into.  The current
>> code is:
>>
>> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
>> {
>> 	int fd, r;
>> 	struct inode *inode;
>> 	struct file *file;
>>
>> 	r = anon_inode_getfd(&fd, &inode, &file,
>> 			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
>> 	if (r)
>> 		return r;
>> 	atomic_inc(&vcpu->kvm->filp->f_count);
>> 	return fd;
>> }
>>
>> and with your proposal, the natural way to write that becomes:
>>
>> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
>> {
>> 	int fd, r;
>>
>> 	r = anon_inode_getfd(&fd, NULL,
>> 			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
>> 	if (r)
>> 		return r;
>> 	atomic_inc(&vcpu->kvm->filp->f_count);
>> 	return fd;
>> }
>>     
>
> I don't know KVM code, but can't the "private_data" setup be completed 
> before calling anon_inode_getfd()?
>   

Creating the fd is the last thing done when creating a vcpu.

> Or ...
>
> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> {
> 	int fd, r;
>
> 	get_file(vcpu->kvm->filp);
> 	r = anon_inode_getfd(&fd, NULL,
> 			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
> 	if (r) {
> 		fput(vcpu->kvm->filp);
> 		return r;
> 	}
> 	return fd;
> }
>   

This seems reasonable.

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