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

[Christoph Hellwig <hch@infradead.org>]

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.