From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: review 4, was Re: projected date for mount.cifs to support DFS
	junction points
Date: Sun, 13 Jan 2008 20:19:03 +0000
Message-ID: <20080113201903.GA24573@infradead.org>
References: <1199988975.7483.3.camel@gn2.draper.com> <524f69650801101228o3639363cp4c9710d747b71ead@mail.gmail.com> <20080111090749.GA14910@infradead.org> <524f69650801110805y56cdbe4nf7587e396b70f32c@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-cifs-client@lists.samba.org, sfrench@us.ibm.com,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	dhowells@redhat.com
To: Steve French <smfrench@gmail.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from pentafluge.infradead.org ([213.146.154.40]:34590 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752208AbYAMUTH (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Sun, 13 Jan 2008 15:19:07 -0500
Content-Disposition: inline
In-Reply-To: <524f69650801110805y56cdbe4nf7587e396b70f32c@mail.gmail.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

[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.