Re: review 5, was Re: projected date for mount.cifs to support DFS junction points

["Q (Igor Mammedov)" <qwerty0987654321@mail.ru>]

[-- Attachment #1.1: Type: text/plain, Size: 2019 bytes --]

Thanks for your comments. Fixed patch is attached.

On Sat, Mar 8, 2008 at 9:41 PM, Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Mar 04, 2008 at 03:38:50PM +0300, Q (Igor Mammedov) wrote:
> > Hi Steve,
> >
> > Looked through inode.c code again and rewrote/simplified patch5
> > See attachment for it.
>
> Thanks, this looks much better.  A few (mostly cosmetic) comments:
>
>
> >From 03c5dcdd566f59240de7843e0a4dd3c3468a0ace Mon Sep 17 00:00:00 2001
> From: Igor Mammedov <niallain@gmail.com>
> Date: Tue, 4 Mar 2008 15:12:27 +0300
> Subject: [PATCH] DFS patch that connects inode with dfs handling ops
>  if it is DFS junction point.
>
> +#define PATH_IN_DFS 1
> +#define PATH_NOT_IN_DFS 0
> +static void cifs_set_ops(const int in_dfs, struct inode *inode)
>
>
>        I think this would be better done as
>
> static void cifs_set_ops(struct inode *inode, bool is_dfs_referral)
>
>
>        Rationale:  a) flags should always come last
>                    b) if there's only two choices a boolean is better
>                       than flags
>
>
> +       full_path = cifs_get_search_path(pTcon, search_path);
> +       tmp_path = full_path != search_path?full_path:NULL;
>
>        tmp_path is only used to free full_path in case it's different
>        from search_path.  It think it would be easier and more clear
>        to just guard the kfree with an
>
>        if (full_path != search_path)
>                kfree(full_path);
>
>        Or am I missing something?
>
> +
> +               if ((rc == -EREMOTE) && (in_dfs == PATH_NOT_IN_DFS)) {
>
>        No need for the inner braces here, just
>
>                if (rc == -EREMOTE && in_dfs == PATH_NOT_IN_DFS) {
>
>        Or with my suggestions from above:
>
>                if (rc == -EREMOTE && is_dfs_reffereal) {
>
> +               kfree(tmp_path);
> +               return rc;
>
>        Please only do this cleanup and return in one place and use
>        an goto out_free_path to jump there from the inside of the
>        function.
>

[-- Attachment #1.2: Type: text/html, Size: 3145 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0005-DFS-patch-that-connects-inode-with-dfs-handling-ops.090308.patch --]
[-- Type: text/x-patch; name=0005-DFS-patch-that-connects-inode-with-dfs-handling-ops.090308.patch, Size: 7948 bytes --]

From e9901b3d09e09382fb73963da5f9475c9bbe6fb3 Mon Sep 17 00:00:00 2001
From: root <root@DMZ_ROUTER.(none)>
Date: Sun, 9 Mar 2008 07:03:38 -0400
Subject: [PATCH] DFS patch that connects inode with dfs handling ops
 if it is DFS junction point.

Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
 fs/cifs/inode.c |  133 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 24eb4d3..e4bfd70 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -30,7 +30,7 @@
 #include "cifs_fs_sb.h"
 
 
-static void cifs_set_ops(struct inode *inode)
+static void cifs_set_ops(struct inode *inode, const bool is_dfs_reffereal)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 
@@ -57,8 +57,12 @@ static void cifs_set_ops(struct inode *inode)
 			inode->i_data.a_ops = &cifs_addr_ops;
 		break;
 	case S_IFDIR:
-		inode->i_op = &cifs_dir_inode_ops;
-		inode->i_fop = &cifs_dir_ops;
+		if (is_dfs_reffereal) {
+			inode->i_op = &cifs_dfs_referral_inode_operations;
+		} else {
+			inode->i_op = &cifs_dir_inode_ops;
+			inode->i_fop = &cifs_dir_ops;
+		}
 		break;
 	case S_IFLNK:
 		inode->i_op = &cifs_symlink_inode_ops;
@@ -153,6 +157,30 @@ static void cifs_unix_info_to_inode(struct inode *inode,
 	spin_unlock(&inode->i_lock);
 }
 
+static const unsigned char *cifs_get_search_path(struct cifsTconInfo *pTcon,
+					const char *search_path)
+{
+	int tree_len;
+	int path_len;
+	char *tmp_path;
+
+	if (!(pTcon->Flags & SMB_SHARE_IS_IN_DFS))
+		return search_path;
+
+	/* use full path name for working with DFS */
+	tree_len = strnlen(pTcon->treeName, MAX_TREE_SIZE + 1);
+	path_len = strnlen(search_path, MAX_PATHCONF);
+
+	tmp_path = kmalloc(tree_len+path_len+1, GFP_KERNEL);
+	if (tmp_path == NULL)
+		return search_path;
+
+	strncpy(tmp_path, pTcon->treeName, tree_len);
+	strncpy(tmp_path+tree_len, search_path, path_len);
+	tmp_path[tree_len+path_len] = 0;
+	return tmp_path;
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -161,41 +189,28 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
+	const unsigned char *full_path;
+	bool is_dfs_reffereal = false;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
+
+	full_path = cifs_get_search_path(pTcon, search_path);
+
+try_again_CIFSSMBUnixQPathInfo:
 	/* could have done a find first instead but this returns more info */
-	rc = CIFSSMBUnixQPathInfo(xid, pTcon, search_path, &findData,
+	rc = CIFSSMBUnixQPathInfo(xid, pTcon, full_path, &findData,
 				  cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
 					CIFS_MOUNT_MAP_SPECIAL_CHR);
 /*	dump_mem("\nUnixQPathInfo return data", &findData,
 		 sizeof(findData)); */
 	if (rc) {
-		if (rc == -EREMOTE) {
-			tmp_path =
-			    kmalloc(strnlen(pTcon->treeName,
-					    MAX_TREE_SIZE + 1) +
-				    strnlen(search_path, MAX_PATHCONF) + 1,
-				    GFP_KERNEL);
-			if (tmp_path == NULL)
-				return -ENOMEM;
-
-			/* have to skip first of the double backslash of
-			   UNC name */
-			strncpy(tmp_path, pTcon->treeName, MAX_TREE_SIZE);
-			strncat(tmp_path, search_path, MAX_PATHCONF);
-			rc = connect_to_dfs_path(xid, pTcon->ses,
-						 /* treename + */ tmp_path,
-						 cifs_sb->local_nls,
-						 cifs_sb->mnt_cifs_flags &
-						    CIFS_MOUNT_MAP_SPECIAL_CHR);
-			kfree(tmp_path);
-
-			/* BB fix up inode etc. */
-		} else if (rc) {
-			return rc;
+		if (rc == -EREMOTE && !is_dfs_reffereal) {
+			is_dfs_reffereal = true;
+			full_path = search_path;
+			goto try_again_CIFSSMBUnixQPathInfo;
 		}
+		goto cgiiu_exit;
 	} else {
 		struct cifsInodeInfo *cifsInfo;
 		__u64 num_of_bytes = le64_to_cpu(findData.NumOfBytes);
@@ -204,8 +219,10 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 		/* get new inode */
 		if (*pinode == NULL) {
 			*pinode = new_inode(sb);
-			if (*pinode == NULL)
-				return -ENOMEM;
+			if (*pinode == NULL) {
+				rc = -ENOMEM;
+				goto cgiiu_exit;
+			}
 			/* Is an i_ino of zero legal? */
 			/* Are there sanity checks we can use to ensure that
 			   the server is really filling in that field? */
@@ -237,8 +254,11 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 			(unsigned long) inode->i_size,
 			(unsigned long long)inode->i_blocks));
 
-		cifs_set_ops(inode);
+		cifs_set_ops(inode, is_dfs_reffereal);
 	}
+cgiiu_exit:
+	if (full_path != search_path)
+		kfree(full_path);
 	return rc;
 }
 
@@ -353,9 +373,10 @@ int cifs_get_inode_info(struct inode **pinode,
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
+	const unsigned char *full_path;
 	char *buf = NULL;
 	int adjustTZ = FALSE;
+	bool is_dfs_reffereal = false;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
@@ -373,8 +394,12 @@ int cifs_get_inode_info(struct inode **pinode,
 		if (buf == NULL)
 			return -ENOMEM;
 		pfindData = (FILE_ALL_INFO *)buf;
+
+		full_path = cifs_get_search_path(pTcon, search_path);
+
+try_again_CIFSSMBQPathInfo:
 		/* could do find first instead but this returns more info */
-		rc = CIFSSMBQPathInfo(xid, pTcon, search_path, pfindData,
+		rc = CIFSSMBQPathInfo(xid, pTcon, full_path, pfindData,
 			      0 /* not legacy */,
 			      cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
 				CIFS_MOUNT_MAP_SPECIAL_CHR);
@@ -382,7 +407,7 @@ int cifs_get_inode_info(struct inode **pinode,
 		when server claims no NT SMB support and the above call
 		failed at least once - set flag in tcon or mount */
 		if ((rc == -EOPNOTSUPP) || (rc == -EINVAL)) {
-			rc = SMBQueryInformation(xid, pTcon, search_path,
+			rc = SMBQueryInformation(xid, pTcon, full_path,
 					pfindData, cifs_sb->local_nls,
 					cifs_sb->mnt_cifs_flags &
 					  CIFS_MOUNT_MAP_SPECIAL_CHR);
@@ -391,31 +416,12 @@ int cifs_get_inode_info(struct inode **pinode,
 	}
 	/* dump_mem("\nQPathInfo return data",&findData, sizeof(findData)); */
 	if (rc) {
-		if (rc == -EREMOTE) {
-			tmp_path =
-			    kmalloc(strnlen
-				    (pTcon->treeName,
-				     MAX_TREE_SIZE + 1) +
-				    strnlen(search_path, MAX_PATHCONF) + 1,
-				    GFP_KERNEL);
-			if (tmp_path == NULL) {
-				kfree(buf);
-				return -ENOMEM;
-			}
-
-			strncpy(tmp_path, pTcon->treeName, MAX_TREE_SIZE);
-			strncat(tmp_path, search_path, MAX_PATHCONF);
-			rc = connect_to_dfs_path(xid, pTcon->ses,
-						 /* treename + */ tmp_path,
-						 cifs_sb->local_nls,
-						 cifs_sb->mnt_cifs_flags &
-						   CIFS_MOUNT_MAP_SPECIAL_CHR);
-			kfree(tmp_path);
-			/* BB fix up inode etc. */
-		} else if (rc) {
-			kfree(buf);
-			return rc;
+		if (rc == -EREMOTE && !is_dfs_reffereal) {
+			is_dfs_reffereal = true;
+			full_path = search_path;
+			goto try_again_CIFSSMBQPathInfo;
 		}
+		goto cgii_exit;
 	} else {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 attr = le32_to_cpu(pfindData->Attributes);
@@ -424,8 +430,8 @@ int cifs_get_inode_info(struct inode **pinode,
 		if (*pinode == NULL) {
 			*pinode = new_inode(sb);
 			if (*pinode == NULL) {
-				kfree(buf);
-				return -ENOMEM;
+				rc = -ENOMEM;
+				goto cgii_exit;
 			}
 			/* Is an i_ino of zero legal? Can we use that to check
 			   if the server supports returning inode numbers?  Are
@@ -573,8 +579,11 @@ int cifs_get_inode_info(struct inode **pinode,
 			atomic_set(&cifsInfo->inUse, 1);
 		}
 
-		cifs_set_ops(inode);
+		cifs_set_ops(inode, is_dfs_reffereal);
 	}
+cgii_exit:
+	if (full_path != search_path)
+		kfree(full_path);
 	kfree(buf);
 	return rc;
 }
@@ -804,7 +813,7 @@ static void posix_fill_in_inode(struct inode *tmp_inode,
 	local_size  = tmp_inode->i_size;
 
 	cifs_unix_info_to_inode(tmp_inode, pData, 1);
-	cifs_set_ops(tmp_inode);
+	cifs_set_ops(tmp_inode, false);
 
 	if (!S_ISREG(tmp_inode->i_mode))
 		return;
-- 
1.5.3.2


[-- Attachment #3: Type: text/plain, Size: 172 bytes --]

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client