From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [linux-cifs-client] review 5, was Re: projected date for
	mount.cifs to support DFS junction points
Date: Sat, 8 Mar 2008 13:41:27 -0500
Message-ID: <20080308184127.GA1461@infradead.org>
References: <1199988975.7483.3.camel@gn2.draper.com> <524f69650801101228o3639363cp4c9710d747b71ead@mail.gmail.com> <20080111090749.GA14910@infradead.org> <524f69650801110805y56cdbe4nf7587e396b70f32c@mail.gmail.com> <20080113202144.GB24573@infradead.org> <47B5BFCF.60304@mail.ru> <47CD42DA.3070809@mail.ru>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Steve French <smfrench@gmail.com>,
	linux-cifs-client@lists.samba.org
To: "Q (Igor Mammedov)" <qwerty0987654321@mail.ru>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from bombadil.infradead.org ([18.85.46.34]:47284 "EHLO
	bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754163AbYCHSl2 (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Sat, 8 Mar 2008 13:41:28 -0500
Content-Disposition: inline
In-Reply-To: <47CD42DA.3070809@mail.ru>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.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.