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

Davide Libenzi
2008-02-28T20:52:28Z

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