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.
> + 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?
> + 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;
}
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
--
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
