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@xxxxxxxxx>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
