[linux-cifs-client] review 4, was Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig]

[David, any chance you could look at the suggestion below to refactor
 the automount from ->follow_link code into a common helper now that
 we've grown a second copy from it]


+	if (cifs_sb->tcon->Flags & 0x2) {

Please don't use magic numbers but symbolic defines.


+static void*

static void *

+cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
+{
+	struct dfs_info3_param *referrals = NULL;
+	unsigned int num_referrals = 0;
+	struct cifs_sb_info *cifs_sb;
+	struct cifsSesInfo *ses;
+	char *full_path = NULL;
+	int xid, i;
+	int rc = 0;
+	struct vfsmount *mnt = ERR_PTR(-ENOENT);
+
+	cFYI(1, ("in %s", __FUNCTION__));
+	BUG_ON(IS_ROOT(dentry));
+
+	xid = GetXid();
+
+	dput(nd->dentry);
+	nd->dentry = dget(dentry);
+	if (d_mountpoint(nd->dentry))
+		goto out_follow;

A link should never be a mountpoint.

+	if (dentry->d_inode == NULL) {
+		rc = -EINVAL;
+		goto out_err;
+	}

d_inode can never be NULL if ->follow_inode, which is quite obvious
from how it's called.

+
+		/* connect to storage node */
+		if (referrals[i].flags & DFSREF_STORAGE_SERVER) {
+			int len;
+			len = strlen(referrals[i].node_name);
+			if (len < 2) {
+				cERROR(1, ("%s: Net Address path too short: %s",
+					__FUNCTION__, referrals[i].node_name));
+				rc = -EINVAL;
+				goto out_err;
+			} else {

your's jumping out with a goto here, so please remove the superflous
else and go down one indentation level to make the code more readable.

+	if (IS_ERR(mnt))
+		goto out_err;
+
+	mntget(mnt);
+	rc = do_add_mount(mnt, nd, nd->mnt->mnt_flags,
+				&cifs_dfs_automount_list);
+	if (rc < 0) {
+		mntput(mnt);
+		if (rc == -EBUSY)
+			goto out_follow;
+		goto out_err;
+	}
+	mntput(nd->mnt);
+	dput(nd->dentry);
+	nd->mnt = mnt;
+	nd->dentry = dget(mnt->mnt_root);

the version of the code in afs that you copy & pasted from in afs
with the switch statement looked more readable.  In fact it would
probably be useful if most of this could be split into a common
helper.


+#ifdef CONFIG_CIFS_DFS_UPCALL
+	mark_mounts_for_expiry(&cifs_dfs_automount_list);
+	mark_mounts_for_expiry(&cifs_dfs_automount_list);
+	shrink_submounts(vfsmnt, &cifs_dfs_automount_list);
+#endif

Should be a helper that can be stubbed out.