Messages in this thread | | | Subject | Re: [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd() | From | Roland Dreier <> | Date | Thu, 28 Feb 2008 12:24:50 -0800 |
| |
> 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; }
which still has the same bug.
Maybe a good way to handle this is just to make the get_file() not optional. I dunno... I feel like we've spent more discussion on this point than it deserves, so someone should just make a decision and I'll adapt the ib_uverbs code to work with whatever it is.
- R.
| |