On Thu, 22 Apr 2010 13:22:32 -0600, Andreas Dilger <adilger@xxxxxxx> wrote:
> On 2010-04-22, at 12:15, Aneesh Kumar K.V wrote:
> > +long do_sys_open_by_handle(struct file_handle *fh, int flags)
> > +{
>
> Note this is inconsistent with the patch description that says that
> the syscall is open_by_handle_at(). While the syscall is still using
> the "_at()" suffix, it is no longer passing any directory.
Fixed.
>
> > + sb = fs_get_sb(&fh->fsid);
> > + if (!sb)
> > + return -ESTALE;
>
> I was going to say that it would be nice to allow a fallback to the "_at" directory of the filesystem, but since the handle is specific to the exact filesystem it was created on anyway (inode number+generation, not even an rsync would be enough) the only way the handle is useful on another node is if the filesystem was cloned at the block device level (or is a shared filesystem identical on all nodes) so the UUID should be identical too.
>
> > + * Find the vfsmount for this superblock in the
> > + * current namespace
> > + */
> > + mnt = fs_get_vfsmount(current, sb);
> > + if (!mnt) {
> > + deactivate_super(sb);
> > + return -ESTALE;
> > + }
> > +
> > + dentry = handle_to_dentry(mnt, fh);
> > + if (IS_ERR(dentry)) {
> > + mntput(mnt);
> > + deactivate_super(sb);
> > + return PTR_ERR(dentry);
> > + }
>
> Why is this code doing "manual" cleanup instead of setting retval and
> goto out_sb: or out_mnt: type labels that only do partial cleanup?
Fixed
>
> > + fsnotify_open(filp->f_path.dentry);
> > + fd_install(fd, filp);
> > + mntput(mnt);
> > + deactivate_super(sb);
> > + return fd;
>
> This could set retval = fd and goto out_mnt: (skipping dput(dentry)) in conjunction with the change proposed below.
>
> > +err_out:
> > + mntput(mnt);
> > + deactivate_super(sb);
> > + dput(dentry);
> > + return retval;
> > +}
>
> I think it makes sense to do the dput(dentry) first, since it will be holding a reference on the sb and vfsmnt, and it also allows code simplification:
>
> out_err:
> dput(dentry);
> out_mnt:
> mntput(mnt);
> out_sb:
> deactivate_super(sb);
>
> return retval;
> }
>
Fixed
I sent a V4 version of the patch with compat syscall support.
-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
