Re: [PATCH/RFC 1/2] anon-inodes: Remove fd_install() fromanon_inode_getfd()

[Davide Libenzi]

On Thu, 28 Feb 2008, Roland Dreier wrote:

>  > If we let the caller call fd_install(), then it may be messed up WRT 
>  > cleanup (fd, file, inode).
> 
> Yes, that is a tiny bit tricky (need to call put_unused_fd() if you
> don't install the fd).
> 
>  > How about removing the inode pointer handout altogether, and *doing* 
>  > fd_install() inside anon_inode_getfd() like:
>  > 
>  > 	if (pfile != NULL) {
>  > 		get_file(file);
>  > 		*pfile = file;
>  > 	}
>  > 	fd_install(fd, file);
>  > 
>  > In this way, if the caller want the file* back, he gets the reference 
>  > bumped before fd_install().
> 
> 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()?
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;
}

Hmm...?



- Davide


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/