Re: projected date for mount.cifs to support DFS junction points

Re: projected date for mount.cifs to support DFS junction points

[Steve French]

> to CIFS not supporting DFS junction points.  Any projected date for that
> to be supported?

I anticipate that it will make Linux kernel 2.6.25 (marked
experimental) and eventually in a cifs version 1.53 backported for
older kernels.

Most of the support required for CIFS DFS for the Linux client has
been written, reviewed and merged already.  The servers (Samba,
NetApp, Windows etc.) have supported DFS for years.   This week I am
reviewing four reasonably small patches to the Linux cifs client from
Igor Mammedov (also on the linux-cifs-client mailing list) that
provide the remaining pieces on the client side.  Igor has done some
excellent work here.

The two questions being discussed are:
1) the size of the mount data retained for each implicit submount
which occurs when we cross a DFS junction
2) the structure used to pass the dfs referral info around


-- 
Thanks,

Steve

Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig]

On Thu, Jan 10, 2008 at 02:28:40PM -0600, Steve French wrote:
> > to CIFS not supporting DFS junction points.  Any projected date for that
> > to be supported?
> 
> I anticipate that it will make Linux kernel 2.6.25 (marked
> experimental) and eventually in a cifs version 1.53 backported for
> older kernels.

If you want to get it into 2.6.25 get it out for review on -fsdevel
ASAP.  2.6.24 is almost done and it needs to be in acceptable state
before 2.6.25 opens.

Re: projected date for mount.cifs to support DFS junction points

[Steve French]

Igor's overview of the patch series is:
http://marc.info/?l=linux-cifs-client&r=1&b=200712&w=2

Although most of the CIFS DFS support is merged, there are three
remaining patches to finish review before merge.  One, for completing
CIFSGetDFSReferral, is probably less interesting to fsdevel:
http://marc.info/?l=linux-cifs-client&m=119798736327234&w=2

but the other two are more important:
http://marc.info/?l=linux-cifs-client&m=119798738227280&w=2
http://marc.info/?l=linux-cifs-client&m=119798739027293&w=2

Igor has indicated that he will look at a subsequent patch to more
efficiently store mountdata on shrinkable mounts by using a pointer to
the parent's mountdata.

On Jan 11, 2008 3:07 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Jan 10, 2008 at 02:28:40PM -0600, Steve French wrote:
> > > to CIFS not supporting DFS junction points.  Any projected date for that
> > > to be supported?
> >
> > I anticipate that it will make Linux kernel 2.6.25 (marked
> > experimental) and eventually in a cifs version 1.53 backported for
> > older kernels.
>
> If you want to get it into 2.6.25 get it out for review on -fsdevel
> ASAP.  2.6.24 is almost done and it needs to be in acceptable state
> before 2.6.25 opens.
>
>



-- 
Thanks,

Steve

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

[Christoph Hellwig]

Unfortunately I couldn't find an mbox archive of the cifs client list
anywhere, so I'll send you the review in reply to this mail, with
one reply per patch.

This is for the first patch:



+ *  fs/cifs/cifs_dfs_ref.c

Please don't mention file names in top of file comments, they serve no
use and get out of sync far too easily.  (Btw, lots of these comment
apply to multiple files and some or all of the patches, I'm not going
to repeat them when it happens again)

+ *   This library is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU Lesser General Public License as published
+ *   by the Free Software Foundation; either version 2.1 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This library is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU Lesser General Public License for more details.
+ *
+ *   You should have received a copy of the GNU Lesser General Public License
+ *   along with this library; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA

I strikes me as odd that this is LGPL, but I noticed other files in
fs/cifs/ have this aswell.  We don't ship a copy of the LGPL with the
kernel which is at least against this comment if not even against the
license.  And it'll revert to GPLv2 as part of the kernel anyway,
so it would be much easier if you just declared the code GPLv2.

+
+/* Resolves server name to ip address.
+ * input:
+ * 	unc - server UNC
+ * output:
+ * 	*ip_addr - pointer to server ip, caller responcible for freeing it.
+ * return 0 on success
+ */

This should be a kerneldoc comment.

+static int
+cifs_resolve_server_name_to_ip(const char *unc, char **ip_addr) {

opening curley brace goes on a line of it's own.

+	int rc = -EAGAIN;
+	struct key *rkey;
+	char *name;
+	int len;
+
+	if ((!ip_addr) || (!unc))
+		return -EINVAL;

Useless braces.  Should be:

	if (!ip_addr || !unc)
		return -EINVAL;

+	rkey = request_key(&key_type_cifs_resolver, name, "");
+	if (!IS_ERR(rkey)) {
+		len = strlen(rkey->payload.data);
+		*ip_addr = kmalloc(len+1, GFP_KERNEL);
+		if (*ip_addr) {
+			memcpy(*ip_addr, rkey->payload.data, len);
+			(*ip_addr)[len] = '\0';
+			cFYI(1, ("%s: resolved: %s to %s", __FUNCTION__,
+					rkey->description,
+					*ip_addr
+				));
+			rc = 0;
+		} else {
+			rc = -ENOMEM;
+		}
+		key_put(rkey);
+	} else {
+		cERROR(1, ("%s: unable to resolve: %s", __FUNCTION__, name));
+	}

please use proper goto based unwiding instea of the if-else galore.

+#ifndef _CIFS_DFS_REF_H
+#define _CIFS_DFS_REF_H
+
+#ifdef __KERNEL__
+extern struct key_type key_type_cifs_resolver;
+#endif /* KERNEL */

No need for __KERNEL__ ifdefs here.

+#ifdef CONFIG_CIFS_DFS_UPCALL
+	rc = register_key_type(&key_type_cifs_resolver);
+	if (rc)
+		goto out_unregister_key_type;
+#endif

Instead of the ifdef mess please put helpers like
register_resolver_key() into the conditionally compiled file and stub them
out in the header if the config option is not set.

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

[Christoph Hellwig]

+struct dfs_info3_param {
+	int flags; /* DFSREF_REFERRAL_SERVER, DFSREF_STORAGE_SERVER*/
+	int PathConsumed;
+	int server_type;
+	int ref_flag;
+	char *path_name;
+	char *node_name;
+};

Please avoid mixed case struct member names.

+
+static inline void init_dfs_info_param(struct dfs_info3_param *param)
+{
+	memset(param, 0, sizeof(struct dfs_info3_param));
+}

And open-coded memset at the caller would be more readable..

+
+static inline void free_dfs_info_param(struct dfs_info3_param *param)
+{
+	if (param) {
+		kfree(param->path_name);
+		kfree(param->node_name);
+		kfree(param);
+	}
+}

This one is completely unused.

--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -3879,8 +3879,8 @@ GetInodeNumOut:
 int
 CIFSGetDFSRefer(const int xid, struct cifsSesInfo *ses,
 		const unsigned char *searchName,
-		unsigned char **targetUNCs,
-		unsigned int *number_of_UNC_in_array,
+		struct dfs_info3_param **targetNode,
+		unsigned int *number_of_Nodes_in_array,
 		const struct nls_table *nls_codepage, int remap)

This function already was huge before the patch and grows even more.
Please consider refactoring it into small, readable helpers.


+	cFYI(1,
+			("Decoding GetDFSRefer response BCC: %d  Offset %d",
+			 pSMBr->ByteCount, data_offset));

Very strange formatting..

+/* BB: Probably we do not need this function any more.
+ * Anyway it never worked. May be one day we well need it.
+ */
 int
 connect_to_dfs_path(int xid, struct cifsSesInfo *pSesInfo,
 		    const char *old_path, const struct nls_table *nls_codepage,
 		    int remap)
 {
+	/*

Please just remove such dead code entirely.  If people want to resurrect
it for some reason they still have the git archives.

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

[Christoph Hellwig]

+#ifdef CONFIG_CIFS_DFS_UPCALL
+	/* copy mount params to sb for use in submounts */
+	/* BB: should we move this after the mount so we
+	 * do not have to do the copy on failed mounts?
+	 * BB: May be it is better to do simple copy before
+	 * complex operation (mount), and in case of fail
+	 * just exit instead of doing mount and attempting
+	 * undo it if this copy fails?*/
+	len = strlen(data);
+	cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL);
+	if (cifs_sb->mountdata == NULL) {
+		kfree(sb->s_fs_info);
+		sb->s_fs_info = NULL;
+		return -ENOMEM;
+	}
+	strncpy(cifs_sb->mountdata, data, len + 1);
+	cifs_sb->mountdata[len] = '\0';
+#endif

Please split the mount data handling into nice helpers that can
be stubbed out for !CONFIG_CIFS_DFS_UPCALL.

-static struct file_system_type cifs_fs_type = {
+struct file_system_type cifs_fs_type = {

This isn't actually used outside of cifsfs.c in this patch, so it should
not be made non-static here.

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

[Christoph Hellwig]

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

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

[Christoph Hellwig]

+#ifdef CONFIG_CIFS_DFS_UPCALL
+			if (is_remote) {
+				inode->i_op =
+					&cifs_dfs_referral_inode_operations;
+				inode->i_fop = NULL;

i_fop should never be set to NULL.  Just leave it alone so it stays
at &empty_fops.

+#ifdef CONFIG_CIFS_DFS_UPCALL
+			if (is_remote) {
+				inode->i_op =
+					&cifs_dfs_referral_inode_operations;
+				inode->i_fop = NULL;
+			} else {
+				inode->i_op = &cifs_dir_inode_ops;
+				inode->i_fop = &cifs_dir_ops;
+			}
+#else
 			inode->i_op = &cifs_dir_inode_ops;
 			inode->i_fop = &cifs_dir_ops;
+#endif

This code and everything surrounding it is duplicated in two functions.
Please refactor it into a common helper before adding new code to it.

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

[Steve French]

On Jan 13, 2008 1:40 PM, Christoph Hellwig <hch@infradead.org> wrote:
> Unfortunately I couldn't find an mbox archive of the cifs client list
> anywhere, so I'll send you the review in reply to this mail, with
> one reply per patch.
>

> + *   This library is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU Lesser General Public License as published
> + *   by the Free Software Foundation; either version 2.1 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This library is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> + *   the GNU Lesser General Public License for more details.
> + *
> + *   You should have received a copy of the GNU Lesser General Public License
> + *   along with this library; if not, write to the Free Software
> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> I strikes me as odd that this is LGPL, but I noticed other files in
> fs/cifs/ have this aswell.  We don't ship a copy of the LGPL with the
> kernel which is at least against this comment if not even against the
> license.  And it'll revert to GPLv2 as part of the kernel anyway,
> so it would be much easier if you just declared the code GPLv2.

The module info claims it is GPL, and of course mixed LGPL/GPL acts as
GPL within the kernel, even though most of the source files are LGPL
(and have been since the beginning).  The reason that most of the
files in the cifs module were created as LGPL is because there were
user space tools which wanted to use those files and it was easier
than trying to dual license the source files.   For example, Darren
Sawyer and Don Capps and some guys involved with SPEC have a cifs
benchmark library which plugs into a benchmark utility - and their
library uses the cifs LGPL files (with minor portability changes
apparently).


-- 
Thanks,

Steve

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

[Steve French]

I agree that CIFSGetDFSRefer needs to be reworked to be easier to
read.  This was one of the reasons that I wanted to look at this
particular patch more.

On Jan 13, 2008 1:48 PM, Christoph Hellwig <hch@infradead.org> wrote:
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -3879,8 +3879,8 @@ GetInodeNumOut:
>  int
>  CIFSGetDFSRefer(const int xid, struct cifsSesInfo *ses,
>                 const unsigned char *searchName,
> -               unsigned char **targetUNCs,
> -               unsigned int *number_of_UNC_in_array,
> +               struct dfs_info3_param **targetNode,
> +               unsigned int *number_of_Nodes_in_array,
>                 const struct nls_table *nls_codepage, int remap)
>
> This function already was huge before the patch and grows even more.
> Please consider refactoring it into small, readable helpers.
>
>
> +       cFYI(1,
> +                       ("Decoding GetDFSRefer response BCC: %d  Offset %d",
> +                        pSMBr->ByteCount, data_offset));
>
> Very strange formatting..
>
> +/* BB: Probably we do not need this function any more.
> + * Anyway it never worked. May be one day we well need it.
> + */
>  int
>  connect_to_dfs_path(int xid, struct cifsSesInfo *pSesInfo,
>                     const char *old_path, const struct nls_table *nls_codepage,
>                     int remap)
>  {
> +       /*
>
> Please just remove such dead code entirely.  If people want to resurrect
> it for some reason they still have the git archives.
>
>



-- 
Thanks,

Steve

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

[Q (Igor Mammedov)]

Christoph Hellwig wrote:
> [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]
> 
> 
> +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.

why link? after patch 5 are applied DFS junction point becomes directory
instead of link. That is what has been done in NFS code.

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

Actually I copy & pasted it from NFS code ...
fs/nfs/namespace.c:nfs_follow_mountpoint

I've tried to do submount/referral machinery in NFS code way.


-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com

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

[Christoph Hellwig]

On Mon, Jan 14, 2008 at 04:15:05PM +0300, Q (Igor Mammedov) wrote:
> > +	dput(nd->dentry);
> > +	nd->dentry = dget(dentry);
> > +	if (d_mountpoint(nd->dentry))
> > +		goto out_follow;
> > 
> > A link should never be a mountpoint.
> 
> why link? after patch 5 are applied DFS junction point becomes directory
> instead of link. That is what has been done in NFS code.

True, this is the ->follow_link on directory hack.

> Actually I copy & pasted it from NFS code ...
> fs/nfs/namespace.c:nfs_follow_mountpoint
> 
> I've tried to do submount/referral machinery in NFS code way.

Okay, and that came from afs.  I really think we should take large
parts of this back into the VFS because it's just to fragile to
be duplicated in various filesystems.  Probably not a blocker for
your patch but I'll keep an eye on consolidating this back into
the core.

Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig]

On Fri, Jan 11, 2008 at 09:07:49AM +0000, Christoph Hellwig wrote:
> If you want to get it into 2.6.25 get it out for review on -fsdevel
> ASAP.  2.6.24 is almost done and it needs to be in acceptable state
> before 2.6.25 opens.

So I've done an extensive review now, but the patches (or rather an
incomplete set, that's even dumber) made it into Linus tree without
things beeing addressed.  That's not how it's supposed to work.

Re: projected date for mount.cifs to support DFS junction points

[Steve French]

I only remember missing a loop unwinding on exit style comment of
yours that was not addressed in what got integrated.  I will go back
through your notes again to see if I missed one.

I meant to merge the final patch last week but ran out of time.  Will
try to finish that this week.

On Feb 5, 2008 10:07 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Jan 11, 2008 at 09:07:49AM +0000, Christoph Hellwig wrote:
> > If you want to get it into 2.6.25 get it out for review on -fsdevel
> > ASAP.  2.6.24 is almost done and it needs to be in acceptable state
> > before 2.6.25 opens.
>
> So I've done an extensive review now, but the patches (or rather an
> incomplete set, that's even dumber) made it into Linus tree without
> things beeing addressed.  That's not how it's supposed to work.
>
>



-- 
Thanks,

Steve

Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig]

On Wed, Feb 06, 2008 at 07:43:01AM -0600, Steve French wrote:
> I only remember missing a loop unwinding on exit style comment of
> yours that was not addressed in what got integrated.  I will go back
> through your notes again to see if I missed one.

 - there's still all that CONFIG_CIFS_DFS_UPCALL ifdefery left in
   cifsfs.c that should go into a helper
 - cifs_fs_type is made non-static but not actually used anywhere
 - dfs_info3_param still has the camelCase PathConsumed member name
 - dfs_shrink_umount_helper is called under ifdef instead of a proper
   stub
 - dns_resolve.[ch] still have the filename mentioned in the top of file
   comments
 - dns_resolve.c still has non-kerneldoc function description comments
 - dns_resolve.h still has the useless __KERNEL__ ifdef
 - the unused free_dfs_info_param function is still around
 - lots of useless and confusing braces left
 - dns_resolve_server_name_to_ip still has deeply nested conditionals
   instead of proper goto unwinding

There's a reason why we usually repost patches to the list after
addressing review comments..

and while I'm at it a lot of the non-DFS additions to cifs aren't quite
up to standards for kernel code either, lots of useless braces, wierd
coding style and ifdef mania.  What happened to the idea of running all
cifs patches past linux-fsdevel?  Also running checkpath.pl over them
might be a not too bad idea.

Re: projected date for mount.cifs to support DFS junction points

[Steve French]

On Feb 7, 2008 12:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Feb 06, 2008 at 07:43:01AM -0600, Steve French wrote:
> > I only remember missing a loop unwinding on exit style comment of
> > yours that was not addressed in what got integrated.  I will go back
> > through your notes again to see if I missed one.
>
>  - there's still all that CONFIG_CIFS_DFS_UPCALL ifdefery left in
>    cifsfs.c that should go into a helper
>  - cifs_fs_type is made non-static but not actually used anywhere
>  - dfs_info3_param still has the camelCase PathConsumed member name
>  - dfs_shrink_umount_helper is called under ifdef instead of a proper
>    stub
>  - dns_resolve.[ch] still have the filename mentioned in the top of file
>    comments
>  - dns_resolve.c still has non-kerneldoc function description comments
>  - dns_resolve.h still has the useless __KERNEL__ ifdef
>  - the unused free_dfs_info_param function is still around
>  - lots of useless and confusing braces left
>  - dns_resolve_server_name_to_ip still has deeply nested conditionals
>    instead of proper goto unwinding
OK - am going through the list.  I thought that Igor had addressed many of
these already.


> There's a reason why we usually repost patches to the list after
> addressing review comments..
AFAIK Igor Mammedov did post the modified DFS patch again (I remember
seeing e.g.
the 2nd or 3rd version of patch 1, the last before it was merged,
posted to the list and
which seemed to include your suggestions, and which did not get any
complaints on fsdevel),
but the additional list you compiled above helps.

> and while I'm at it a lot of the non-DFS additions to cifs aren't quite
> up to standards for kernel code either, lots of useless braces, wierd
> coding style and ifdef mania.
Shaggy made a good suggestion about how to remove the 23
cases of #ifdef CONFIG_CIFS_DEBUG2 (which make the relatively
new cifsacl.c harder to read).  I will post this later today or tomorrow.

> Also running checkpath.pl over them might be a not too bad idea.
I do and I also like checkpatch, it has helped a lot.   In late June
(and then following up a few months later) I did a very large set of
checkpatch corrections for cifs fixing over 80% of the warnings (by
diffing the cifs directory against an empty directory and running the
whole set through checkpatch).   cifs now has a similar number of
checkpatch warnings to other modules its size (and fewer than ext4).
There are two types of checkpatch warnings in particular that I have
not changed.   The cifsacl code (fs/cifs/cifsacl.c) did introduce a
few new checkpatch warnings.

I just ran the diff of the whole cifs directory (against the empty
directory) through the most recent checkpatch and fixed about 40% of
the
remaining cifs checkpatch warnings - most of the rest are not as
important (e.g. the issue discussed earlier about why cifs uses
typedefs in the network protocol definition header file in order to
match the original cifs protocol documentation).


-- 
Thanks,

Steve

Re: projected date for mount.cifs to support DFS junction points

[Steve French]

On Feb 7, 2008 12:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
> and while I'm at it a lot of the non-DFS additions to cifs aren't quite
> up to standards for kernel code either, lots of useless braces, wierd
> coding style and ifdef mania.
Reducing "ifdef mania" would help (there are about 120 "#ifdef
CONFIG_CIFS_somethings" in CIFS), I discussed getting rid of most
references to CONFIG_CIFS_DEBUG2 with Shaggy today.  One approach
would be to do something like the following (there are about 25 other
places that would be changed in the same way  but this gives the
idea).  Is this an acceptable approach (seems like the compiler should
optimize it out properly even with this change)

diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index 90e7624..722b722 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -25,8 +25,11 @@

 void cifs_dump_mem(char *label, void *data, int length);
 #ifdef CONFIG_CIFS_DEBUG2
+#define DEBUG2 2
 void cifs_dump_detail(struct smb_hdr *);
 void cifs_dump_mids(struct TCP_Server_Info *);
+#else
+#define DEBUG2 0
 #endif
 extern int traceSMB;		/* flag which enables the function below */
 void dump_smb(struct smb_hdr *, int);
diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index 842aaa9..381ddca 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -215,9 +215,7 @@ static void access_flags_to_mode(__le32 ace_flags,
int type, umode_t *pmode,

 	if (flags & GENERIC_ALL) {
 		*pmode |= (S_IRWXUGO & (*pbits_to_set));
-#ifdef CONFIG_CIFS_DEBUG2
-		cFYI(1, ("all perms"));
-#endif
+		cFYI(DEBUG2, ("all perms"));
 		return;
 	}
 	if ((flags & GENERIC_WRITE) ||
@@ -230,9 +228,7 @@ static void access_flags_to_mode(__le32 ace_flags,
int type, umode_t *pmode,
 			((flags & FILE_EXEC_RIGHTS) == FILE_EXEC_RIGHTS))
 		*pmode |= (S_IXUGO & (*pbits_to_set));

-#ifdef CONFIG_CIFS_DEBUG2
-	cFYI(1, ("access flags 0x%x mode now 0x%x", flags, *pmode));
-#endif
+	cFYI(DEBUG2, ("access flags 0x%x mode now 0x%x", flags, *pmode));
 	return;
 }

@@ -261,9 +257,7 @@ static void mode_to_access_flags(umode_t mode,
umode_t bits_to_use,
 	if (mode & S_IXUGO)
 		*pace_flags |= SET_FILE_EXEC_RIGHTS;

-#ifdef CONFIG_CIFS_DEBUG2
-	cFYI(1, ("mode: 0x%x, access flags now 0x%x", mode, *pace_flags));
-#endif
+	cFYI(DEBUG2, ("mode: 0x%x, access flags now 0x%x", mode, *pace_flags));
 	return;
 }




-- 
Thanks,

Steve

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

[Q (Igor Mammedov)]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

Sorry guys, but I have a lot of work for the last 3 weeks,
so I couldn't spare much time for a hobby and react quickly.

Here is what I've done the last weekend.
Attached:
 fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
 fixed mixed case in struct member (0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)


Also I noticed that patch 2/5 is not completely applied yet. I'll send Steve
interim patch I've made to make thing compiled and working.

Christoph Hellwig wrote:
> +#ifdef CONFIG_CIFS_DFS_UPCALL
> +			if (is_remote) {
> +				inode->i_op =
> +					&cifs_dfs_referral_inode_operations;
> +				inode->i_fop = NULL;
> 
> i_fop should never be set to NULL.  Just leave it alone so it stays
> at &empty_fops.
> 
> +#ifdef CONFIG_CIFS_DFS_UPCALL
> +			if (is_remote) {
> +				inode->i_op =
> +					&cifs_dfs_referral_inode_operations;
> +				inode->i_fop = NULL;
> +			} else {
> +				inode->i_op = &cifs_dir_inode_ops;
> +				inode->i_fop = &cifs_dir_ops;
> +			}
> +#else
>  			inode->i_op = &cifs_dir_inode_ops;
>  			inode->i_fop = &cifs_dir_ops;
> +#endif
> 
> This code and everything surrounding it is duplicated in two functions.
> Please refactor it into a common helper before adding new code to it.
> 
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 
> .
> 


-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com





[-- Attachment #2: 0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch --]
[-- Type: text/x-patch, Size: 7131 bytes --]

>From 490ed5e40713206bf3011880b03cd9f5766b5467 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Fri, 15 Feb 2008 19:31:42 +0300
Subject: [PATCH] DFS patch that connects inode with dfs handling ops
 if it is DFS junction point.

DFS junction point is detected by EREMOTE error from
CIFSSMBQPathInfo.Then we need to request server again,
this time with full path name so we could get correct
info for this inode.
It is final DFS patch that gets all patchset working
and it depends on all previous DFS patches.

Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
 fs/cifs/cifs_dfs_ref.c |   10 +++++
 fs/cifs/cifsfs.h       |   11 ++++++
 fs/cifs/inode.c        |   93 +++++++++++++++++++++---------------------------
 3 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index 413ee23..3161986 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -29,6 +29,16 @@ LIST_HEAD(cifs_dfs_automount_list);
  * DFS functions
 */
 
+void cifs_dfs_ref_inode_op_fixup(int is_remote, struct inode *inode)
+{
+	if (is_remote) {
+		inode->i_op = &cifs_dfs_referral_inode_operations;
+	} else {
+		inode->i_op = &cifs_dir_inode_ops;
+		inode->i_fop = &cifs_dir_ops;
+	}
+}
+
 void dfs_shrink_umount_helper(struct vfsmount *vfsmnt)
 {
 	mark_mounts_for_expiry(&cifs_dfs_automount_list);
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 195b14d..d693e2d 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -91,6 +91,17 @@ extern int cifs_dir_notify(struct file *, unsigned long arg);
 extern struct dentry_operations cifs_dentry_ops;
 extern struct dentry_operations cifs_ci_dentry_ops;
 
+#ifdef CONFIG_CIFS_DFS_UPCALL
+extern void cifs_dfs_ref_inode_op_fixup(int is_remote, struct inode *inode);
+#else
+static inline void cifs_dfs_ref_inode_op_fixup(int is_remote,
+			struct inode *inode)
+{
+	inode->i_op = &cifs_dir_inode_ops;
+	inode->i_fop = &cifs_dir_ops;
+}
+#endif /* DFS_UPCALL */
+
 /* Functions related to symlinks */
 extern void *cifs_follow_link(struct dentry *direntry, struct nameidata *nd);
 extern void cifs_put_link(struct dentry *direntry,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 6020add..79e61c5 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -37,7 +37,7 @@ 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;
+	int is_remote = 0;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
@@ -48,30 +48,10 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 /*	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;
-		}
+		/* BB: we need to revise code here
+		 * and do it as in cifs_get_inode_info
+		 * now it will return error -EREMOTE in case of dfs*/
+		return rc;
 	} else {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 type = le32_to_cpu(findData.Type);
@@ -200,8 +180,7 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 				inode->i_data.a_ops = &cifs_addr_ops;
 		} else if (S_ISDIR(inode->i_mode)) {
 			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
+			cifs_dfs_ref_inode_op_fixup(is_remote, inode);
 		} else if (S_ISLNK(inode->i_mode)) {
 			cFYI(1, ("Symbolic Link inode"));
 			inode->i_op = &cifs_symlink_inode_ops;
@@ -326,8 +305,9 @@ 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;
+	char *tmp_path = NULL;
 	char *buf = NULL;
+	int is_remote = 0;
 	int adjustTZ = FALSE;
 
 	pTcon = cifs_sb->tcon;
@@ -342,12 +322,38 @@ int cifs_get_inode_info(struct inode **pinode,
 
 	/* if file info not passed in then get it from server */
 	if (pfindData == NULL) {
+		int l_max_len;
+		const char *full_path;
+try_again_CIFSSMBQPathInfo:
+
 		buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
 		if (buf == NULL)
 			return -ENOMEM;
 		pfindData = (FILE_ALL_INFO *)buf;
+
+		cFYI(1, ("%s: pTcon->Flags: 0x%x", __FUNCTION__, pTcon->Flags));
+		if ((!is_remote) && (pTcon->Flags & 0x2)) {
+			/* use full path name for working with DFS */
+			l_max_len = strnlen(pTcon->treeName, MAX_TREE_SIZE + 1)
+				+ strnlen(search_path, MAX_PATHCONF) + 1;
+			kfree(tmp_path);
+			tmp_path = kmalloc(l_max_len, GFP_KERNEL);
+			if (tmp_path == NULL) {
+				kfree(buf);
+				return -ENOMEM;
+			}
+			strncpy(tmp_path, pTcon->treeName, l_max_len);
+			strcat(tmp_path, search_path);
+			tmp_path[l_max_len-1] = 0;
+			full_path = tmp_path;
+		} else {
+			full_path = search_path;
+		}
+
+		cFYI(1, ("%s: query server with full path: '%s'",
+					__FUNCTION__, full_path));
 		/* 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);
@@ -364,27 +370,11 @@ 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. */
+		if ((rc == -EREMOTE) && (!is_remote)) {
+			is_remote = 1;
+			kfree(buf);
+			buf = NULL;
+			goto try_again_CIFSSMBQPathInfo;
 		} else if (rc) {
 			kfree(buf);
 			return rc;
@@ -567,8 +557,7 @@ int cifs_get_inode_info(struct inode **pinode,
 				inode->i_data.a_ops = &cifs_addr_ops;
 		} else if (S_ISDIR(inode->i_mode)) {
 			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
+			cifs_dfs_ref_inode_op_fixup(is_remote, inode);
 		} else if (S_ISLNK(inode->i_mode)) {
 			cFYI(1, ("Symbolic Link inode"));
 			inode->i_op = &cifs_symlink_inode_ops;
-- 
1.5.3.7


[-- Attachment #3: 0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch --]
[-- Type: text/x-patch, Size: 1255 bytes --]

>From b15ae2e7e315abc7a3aad572b84d4604a4ab491b Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Fri, 15 Feb 2008 18:26:34 +0300
Subject: [PATCH]  Fixed mixed case name in structure dfs_info3_param

Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
 fs/cifs/cifs_dfs_ref.c |    2 +-
 fs/cifs/cifsglob.h     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index 3161986..39a0108 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -296,7 +296,7 @@ static void dump_referral(const struct dfs_info3_param *ref)
 	cFYI(1, ("DFS: node path: %s", ref->node_name));
 	cFYI(1, ("DFS: fl: %hd, srv_type: %hd", ref->flags, ref->server_type));
 	cFYI(1, ("DFS: ref_flags: %hd, path_consumed: %hd", ref->ref_flag,
-				ref->PathConsumed));
+				ref->path_consumed));
 }
 
 
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 5d32d8d..69a2e19 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -454,7 +454,7 @@ struct dir_notify_req {
 
 struct dfs_info3_param {
 	int flags; /* DFSREF_REFERRAL_SERVER, DFSREF_STORAGE_SERVER*/
-	int PathConsumed;
+	int path_consumed;
 	int server_type;
 	int ref_flag;
 	char *path_name;
-- 
1.5.3.7


[-- Attachment #4: 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

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

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Fri, Feb 15, 2008 at 07:37:35PM +0300, Q (Igor Mammedov) wrote:
> Sorry guys, but I have a lot of work for the last 3 weeks,
> so I couldn't spare much time for a hobby and react quickly.

No problem.  I know this problem very well as almost all of my core
kernel contributions are spare time as well.  Thanks for keeping
up the work.

> Here is what I've done the last weekend.
> Attached:
>  fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
>  fixed mixed case in struct member (0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)

The second one is trivially correct and should be pushed to Linus asap
as small cleanup.  Patch 1 is not exactly what I had in mind, I was
thinking about factoring out the common bits of cifs-cifs_get_inode_info
and cifs-cifs_get_inode_info_unix to avoid having all the logic to
set the various options twice.  I've attached two quick and untested
patches showing what I mean.  I think in this case having the ifdef
for that two line statement setting the inode operations here is fine.
But thinking about it I'm not even sure if it's good idea to make
dfs support conditional.  Any reason it can't just be included
unconditionally?



[-- Attachment #2: cifs-cifs_get_inode_info-cleanup --]
[-- Type: text/plain, Size: 4186 bytes --]

Index: linux-2.6/fs/cifs/inode.c
===================================================================
--- linux-2.6.orig/fs/cifs/inode.c	2008-02-15 17:44:48.000000000 +0100
+++ linux-2.6/fs/cifs/inode.c	2008-02-15 17:52:04.000000000 +0100
@@ -29,6 +29,46 @@
 #include "cifs_debug.h"
 #include "cifs_fs_sb.h"
 
+
+static void cifs_set_ops(struct inode *inode)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+
+	switch (inode->i_mode & S_IFMT) {
+	case S_IFREG:
+		inode->i_op = &cifs_file_inode_ops;
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
+			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+				inode->i_fop = &cifs_file_direct_nobrl_ops;
+			else
+				inode->i_fop = &cifs_file_direct_ops;
+		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+			inode->i_fop = &cifs_file_nobrl_ops;
+		else { /* not direct, send byte range locks */
+			inode->i_fop = &cifs_file_ops;
+		}
+
+
+		/* check if server can support readpages */
+		if (cifs_sb->tcon->ses->server->maxBuf <
+				PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
+			inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
+		else
+			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;
+		break;
+	case S_IFLNK:
+		inode->i_op = &cifs_symlink_inode_ops;
+		break;
+	default:
+		init_special_inode(inode, inode->i_mode, inode->i_rdev);
+		break;
+	}
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -178,39 +218,8 @@ int cifs_get_inode_info_unix(struct inod
 		cFYI(1, ("Size %ld and blocks %llu",
 			(unsigned long) inode->i_size,
 			(unsigned long long)inode->i_blocks));
-		if (S_ISREG(inode->i_mode)) {
-			cFYI(1, ("File inode"));
-			inode->i_op = &cifs_file_inode_ops;
-			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
-				if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-					inode->i_fop =
-						&cifs_file_direct_nobrl_ops;
-				else
-					inode->i_fop = &cifs_file_direct_ops;
-			} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-				inode->i_fop = &cifs_file_nobrl_ops;
-			else /* not direct, send byte range locks */
-				inode->i_fop = &cifs_file_ops;
-
-			/* check if server can support readpages */
-			if (pTcon->ses->server->maxBuf <
-			    PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
-				inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
-			else
-				inode->i_data.a_ops = &cifs_addr_ops;
-		} else if (S_ISDIR(inode->i_mode)) {
-			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
-		} else if (S_ISLNK(inode->i_mode)) {
-			cFYI(1, ("Symbolic Link inode"));
-			inode->i_op = &cifs_symlink_inode_ops;
-		/* tmp_inode->i_fop = */ /* do not need to set to anything */
-		} else {
-			cFYI(1, ("Init special inode"));
-			init_special_inode(inode, inode->i_mode,
-					   inode->i_rdev);
-		}
+
+		cifs_set_ops(inode);
 	}
 	return rc;
 }
@@ -546,36 +555,7 @@ int cifs_get_inode_info(struct inode **p
 			atomic_set(&cifsInfo->inUse, 1);
 		}
 
-		if (S_ISREG(inode->i_mode)) {
-			cFYI(1, ("File inode"));
-			inode->i_op = &cifs_file_inode_ops;
-			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
-				if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-					inode->i_fop =
-						&cifs_file_direct_nobrl_ops;
-				else
-					inode->i_fop = &cifs_file_direct_ops;
-			} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-				inode->i_fop = &cifs_file_nobrl_ops;
-			else /* not direct, send byte range locks */
-				inode->i_fop = &cifs_file_ops;
-
-			if (pTcon->ses->server->maxBuf <
-			     PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
-				inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
-			else
-				inode->i_data.a_ops = &cifs_addr_ops;
-		} else if (S_ISDIR(inode->i_mode)) {
-			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
-		} else if (S_ISLNK(inode->i_mode)) {
-			cFYI(1, ("Symbolic Link inode"));
-			inode->i_op = &cifs_symlink_inode_ops;
-		} else {
-			init_special_inode(inode, inode->i_mode,
-					   inode->i_rdev);
-		}
+		cifs_set_ops(inode);
 	}
 	kfree(buf);
 	return rc;

[-- Attachment #3: cifs-cifs_get_inode_info-cleanup-2 --]
[-- Type: text/plain, Size: 3870 bytes --]

Index: linux-2.6/fs/cifs/inode.c
===================================================================
--- linux-2.6.orig/fs/cifs/inode.c	2008-02-15 17:52:26.000000000 +0100
+++ linux-2.6/fs/cifs/inode.c	2008-02-15 18:01:35.000000000 +0100
@@ -69,6 +69,35 @@ static void cifs_set_ops(struct inode *i
 	}
 }
 
+static int cifs_get_inode_info_remote(struct cifs_sb_info *cifs_sb,
+		const unsigned char *search_path, int xid)
+{
+	struct cifsTconInfo *tcon = cifs_sb->tcon;
+	char *tmp_path;
+	size_t tmp_path_len;
+	int rc;
+
+	tmp_path_len = strnlen(tcon->treeName, MAX_TREE_SIZE + 1) +
+		       strnlen(search_path, MAX_PATHCONF) + 1;
+
+	tmp_path = kmalloc(tmp_path_len, GFP_KERNEL);
+	if (!tmp_path)
+		return -ENOMEM;
+
+	/*
+	 * Have to skip first of the double backslash of the UNC name.
+	 */
+	strncpy(tmp_path, tcon->treeName, MAX_TREE_SIZE);
+	strncat(tmp_path, search_path, MAX_PATHCONF);
+
+	rc = connect_to_dfs_path(xid, tcon->ses, /* treename + */ tmp_path,
+				 cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
+				 		CIFS_MOUNT_MAP_SPECIAL_CHR);
+
+	kfree(tmp_path);
+	return rc;
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -77,7 +106,6 @@ int cifs_get_inode_info_unix(struct inod
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
@@ -87,32 +115,12 @@ int cifs_get_inode_info_unix(struct inod
 					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;
-		}
-	} else {
+	if (rc == -EREMOTE)
+		return cifs_get_inode_info_remote(cifs_sb, search_path, xid);
+	else if (rc)
+		return rc;
+	else {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 type = le32_to_cpu(findData.Type);
 		__u64 num_of_bytes = le64_to_cpu(findData.NumOfBytes);
@@ -335,7 +343,6 @@ int cifs_get_inode_info(struct inode **p
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
 	char *buf = NULL;
 	int adjustTZ = FALSE;
 
@@ -372,33 +379,9 @@ int cifs_get_inode_info(struct inode **p
 		}
 	}
 	/* 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;
-		}
-	} else {
+	if (rc == -EREMOTE)
+		rc = cifs_get_inode_info_remote(cifs_sb, search_path, xid);
+	else if (!rc) {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 attr = le32_to_cpu(pfindData->Attributes);
 

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

[Steve French]

On 2/15/08, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Feb 15, 2008 at 07:37:35PM +0300, Q (Igor Mammedov) wrote:
>  > Here is what I've done the last weekend.
>  > Attached:
>  >  fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
Not merged yet.

>  >  fixed mixed case in struct member 0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)

Now merged into cifs-2.6.git

> The second one is trivially correct and should be pushed to Linus asap
>  as small cleanup.  Patch 1 is not exactly what I had in mind, I was
>  thinking about factoring out the common bits of cifs-cifs_get_inode_info
>  and cifs-cifs_get_inode_info_unix to avoid having all the logic to
>  set the various options twice.  I've attached two quick and untested
>  patches showing what I mean.  I think in this case having the ifdef
>  for that two line statement setting the inode operations here is fine.
I reviewed and merged into cifs-2.6.git one of the two patches from
Christoph (the cifs_set_ops one), but wanted to look more carefully at
the other (cifs_get_info_remote) to make sure that buf was being freed
in the cifs_get_inode_info path (otherwise it is fine).

>  But thinking about it I'm not even sure if it's good idea to make
>  dfs support conditional.  Any reason it can't just be included
>  unconditionally?
It would make the code slightly smaller (perhaps useful someday for
OLPC or embedded) and removes a piece of code that is not needed in
all home networks (although DFS is useful even to some of these).  I
lean toward removing the ifdef when it has made it through one or two
more release cycles and is no longer experimental.   SInce there are a
few experimental features (Kerberos and DFS) that are broadly useful -
but not all users need both, I don't mind keeping the configure for
each different for the short term but don't have a strong opinion on
this.

-- 
Thanks,

Steve

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

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 70 bytes --]

If you like these kind of consolidation patches here's another one:



[-- Attachment #2: cifs_mknod-cleanup --]
[-- Type: text/plain, Size: 11095 bytes --]

Index: linux-2.6/fs/cifs/inode.c
===================================================================
--- linux-2.6.orig/fs/cifs/inode.c	2008-02-15 22:46:08.000000000 +0100
+++ linux-2.6/fs/cifs/inode.c	2008-02-15 23:09:28.000000000 +0100
@@ -98,6 +98,90 @@ static int cifs_get_inode_info_remote(st
 	return rc;
 }
 
+static void cifs_unix_info_to_inode(struct inode *inode,
+		FILE_UNIX_BASIC_INFO *info, int force_uid_gid)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+	struct cifsInodeInfo *cifsInfo = CIFS_I(inode);
+	__u64 num_of_bytes = le64_to_cpu(info->NumOfBytes);
+	__u64 end_of_file = le64_to_cpu(info->EndOfFile);
+
+	inode->i_atime = cifs_NTtimeToUnix(le64_to_cpu(info->LastAccessTime));
+	inode->i_mtime =
+		cifs_NTtimeToUnix(le64_to_cpu(info->LastModificationTime));
+	inode->i_ctime = cifs_NTtimeToUnix(le64_to_cpu(info->LastStatusChange));
+	inode->i_mode = le64_to_cpu(info->Permissions);
+
+	/*
+	 * Since we set the inode type below we need to mask off
+	 * to avoid strange results if bits set above.
+	 */
+	inode->i_mode &= ~S_IFMT;
+	switch (le32_to_cpu(info->Type)) {
+	case UNIX_FILE:
+		inode->i_mode |= S_IFREG;
+		break;
+	case UNIX_SYMLINK:
+		inode->i_mode |= S_IFLNK;
+		break;
+	case UNIX_DIR:
+		inode->i_mode |= S_IFDIR;
+		break;
+	case UNIX_CHARDEV:
+		inode->i_mode |= S_IFCHR;
+		inode->i_rdev = MKDEV(le64_to_cpu(info->DevMajor),
+				      le64_to_cpu(info->DevMinor) & MINORMASK);
+		break;
+	case UNIX_BLOCKDEV:
+		inode->i_mode |= S_IFBLK;
+		inode->i_rdev = MKDEV(le64_to_cpu(info->DevMajor),
+				      le64_to_cpu(info->DevMinor) & MINORMASK);
+		break;
+	case UNIX_FIFO:
+		inode->i_mode |= S_IFIFO;
+		break;
+	case UNIX_SOCKET:
+		inode->i_mode |= S_IFSOCK;
+		break;
+	default:
+		/* safest to call it a file if we do not know */
+		inode->i_mode |= S_IFREG;
+		cFYI(1, ("unknown type %d", le32_to_cpu(info->Type)));
+		break;
+	}
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID) &&
+	    !force_uid_gid)
+		inode->i_uid = cifs_sb->mnt_uid;
+	else
+		inode->i_uid = le64_to_cpu(info->Uid);
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID) &&
+	    !force_uid_gid)
+		inode->i_gid = cifs_sb->mnt_gid;
+	else
+		inode->i_gid = le64_to_cpu(info->Gid);
+
+	inode->i_nlink = le64_to_cpu(info->Nlinks);
+
+	spin_lock(&inode->i_lock);
+	if (is_size_safe_to_change(cifsInfo, end_of_file)) {
+		/*
+		 * We can not safely change the file size here if the client
+		 * is writing to it due to potential races.
+		 */
+		i_size_write(inode, end_of_file);
+
+		/*
+		 * i_blocks is not related to (i_size / i_blksize),
+		 * but instead 512 byte (2**9) size is required for
+		 * calculating num blocks.
+		 */
+		inode->i_blocks = (512 - 1 + num_of_bytes) >> 9;
+	}
+	spin_unlock(&inode->i_lock);
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -122,7 +206,6 @@ int cifs_get_inode_info_unix(struct inod
 		return rc;
 	else {
 		struct cifsInodeInfo *cifsInfo;
-		__u32 type = le32_to_cpu(findData.Type);
 		__u64 num_of_bytes = le64_to_cpu(findData.NumOfBytes);
 		__u64 end_of_file = le64_to_cpu(findData.EndOfFile);
 
@@ -153,73 +236,8 @@ int cifs_get_inode_info_unix(struct inod
 		/* this is ok to set on every inode revalidate */
 		atomic_set(&cifsInfo->inUse, 1);
 
-		inode->i_atime =
-		    cifs_NTtimeToUnix(le64_to_cpu(findData.LastAccessTime));
-		inode->i_mtime =
-		    cifs_NTtimeToUnix(le64_to_cpu
-				(findData.LastModificationTime));
-		inode->i_ctime =
-		    cifs_NTtimeToUnix(le64_to_cpu(findData.LastStatusChange));
-		inode->i_mode = le64_to_cpu(findData.Permissions);
-		/* since we set the inode type below we need to mask off
-		   to avoid strange results if bits set above */
-		inode->i_mode &= ~S_IFMT;
-		if (type == UNIX_FILE) {
-			inode->i_mode |= S_IFREG;
-		} else if (type == UNIX_SYMLINK) {
-			inode->i_mode |= S_IFLNK;
-		} else if (type == UNIX_DIR) {
-			inode->i_mode |= S_IFDIR;
-		} else if (type == UNIX_CHARDEV) {
-			inode->i_mode |= S_IFCHR;
-			inode->i_rdev = MKDEV(le64_to_cpu(findData.DevMajor),
-				le64_to_cpu(findData.DevMinor) & MINORMASK);
-		} else if (type == UNIX_BLOCKDEV) {
-			inode->i_mode |= S_IFBLK;
-			inode->i_rdev = MKDEV(le64_to_cpu(findData.DevMajor),
-				le64_to_cpu(findData.DevMinor) & MINORMASK);
-		} else if (type == UNIX_FIFO) {
-			inode->i_mode |= S_IFIFO;
-		} else if (type == UNIX_SOCKET) {
-			inode->i_mode |= S_IFSOCK;
-		} else {
-			/* safest to call it a file if we do not know */
-			inode->i_mode |= S_IFREG;
-			cFYI(1, ("unknown type %d", type));
-		}
+		cifs_unix_info_to_inode(inode, &findData, 0);
 
-		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID)
-			inode->i_uid = cifs_sb->mnt_uid;
-		else
-			inode->i_uid = le64_to_cpu(findData.Uid);
-
-		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID)
-			inode->i_gid = cifs_sb->mnt_gid;
-		else
-			inode->i_gid = le64_to_cpu(findData.Gid);
-
-		inode->i_nlink = le64_to_cpu(findData.Nlinks);
-
-		spin_lock(&inode->i_lock);
-		if (is_size_safe_to_change(cifsInfo, end_of_file)) {
-		/* can not safely change the file size here if the
-		   client is writing to it due to potential races */
-			i_size_write(inode, end_of_file);
-
-		/* blksize needs to be multiple of two. So safer to default to
-		blksize and blkbits set in superblock so 2**blkbits and blksize
-		will match rather than setting to:
-		(pTcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE) & 0xFFFFFE00;*/
-
-		/* This seems incredibly stupid but it turns out that i_blocks
-		   is not related to (i_size / i_blksize), instead 512 byte size
-		   is required for calculating num blocks */
-
-		/* 512 bytes (2**9) is the fake blocksize that must be used */
-		/* for this calculation */
-			inode->i_blocks = (512 - 1 + num_of_bytes) >> 9;
-		}
-		spin_unlock(&inode->i_lock);
 
 		if (num_of_bytes < end_of_file)
 			cFYI(1, ("allocation size less than end of file"));
@@ -757,15 +775,10 @@ psx_del_no_retry:
 static void posix_fill_in_inode(struct inode *tmp_inode,
 	FILE_UNIX_BASIC_INFO *pData, int *pobject_type, int isNewInode)
 {
+	struct cifsInodeInfo *cifsInfo = CIFS_I(tmp_inode);
 	loff_t local_size;
 	struct timespec local_mtime;
 
-	struct cifsInodeInfo *cifsInfo = CIFS_I(tmp_inode);
-	struct cifs_sb_info *cifs_sb = CIFS_SB(tmp_inode->i_sb);
-
-	__u32 type = le32_to_cpu(pData->Type);
-	__u64 num_of_bytes = le64_to_cpu(pData->NumOfBytes);
-	__u64 end_of_file = le64_to_cpu(pData->EndOfFile);
 	cifsInfo->time = jiffies;
 	atomic_inc(&cifsInfo->inUse);
 
@@ -773,115 +786,27 @@ static void posix_fill_in_inode(struct i
 	local_mtime = tmp_inode->i_mtime;
 	local_size  = tmp_inode->i_size;
 
-	tmp_inode->i_atime =
-	    cifs_NTtimeToUnix(le64_to_cpu(pData->LastAccessTime));
-	tmp_inode->i_mtime =
-	    cifs_NTtimeToUnix(le64_to_cpu(pData->LastModificationTime));
-	tmp_inode->i_ctime =
-	    cifs_NTtimeToUnix(le64_to_cpu(pData->LastStatusChange));
-
-	tmp_inode->i_mode = le64_to_cpu(pData->Permissions);
-	/* since we set the inode type below we need to mask off type
-	   to avoid strange results if bits above were corrupt */
-	tmp_inode->i_mode &= ~S_IFMT;
-	if (type == UNIX_FILE) {
-		*pobject_type = DT_REG;
-		tmp_inode->i_mode |= S_IFREG;
-	} else if (type == UNIX_SYMLINK) {
-		*pobject_type = DT_LNK;
-		tmp_inode->i_mode |= S_IFLNK;
-	} else if (type == UNIX_DIR) {
-		*pobject_type = DT_DIR;
-		tmp_inode->i_mode |= S_IFDIR;
-	} else if (type == UNIX_CHARDEV) {
-		*pobject_type = DT_CHR;
-		tmp_inode->i_mode |= S_IFCHR;
-		tmp_inode->i_rdev = MKDEV(le64_to_cpu(pData->DevMajor),
-				le64_to_cpu(pData->DevMinor) & MINORMASK);
-	} else if (type == UNIX_BLOCKDEV) {
-		*pobject_type = DT_BLK;
-		tmp_inode->i_mode |= S_IFBLK;
-		tmp_inode->i_rdev = MKDEV(le64_to_cpu(pData->DevMajor),
-				le64_to_cpu(pData->DevMinor) & MINORMASK);
-	} else if (type == UNIX_FIFO) {
-		*pobject_type = DT_FIFO;
-		tmp_inode->i_mode |= S_IFIFO;
-	} else if (type == UNIX_SOCKET) {
-		*pobject_type = DT_SOCK;
-		tmp_inode->i_mode |= S_IFSOCK;
-	} else {
-		/* safest to just call it a file */
-		*pobject_type = DT_REG;
-		tmp_inode->i_mode |= S_IFREG;
-		cFYI(1, ("unknown inode type %d", type));
-	}
-
-#ifdef CONFIG_CIFS_DEBUG2
-	cFYI(1, ("object type: %d", type));
-#endif
-	tmp_inode->i_uid = le64_to_cpu(pData->Uid);
-	tmp_inode->i_gid = le64_to_cpu(pData->Gid);
-	tmp_inode->i_nlink = le64_to_cpu(pData->Nlinks);
-
-	spin_lock(&tmp_inode->i_lock);
-	if (is_size_safe_to_change(cifsInfo, end_of_file)) {
-		/* can not safely change the file size here if the
-		client is writing to it due to potential races */
-		i_size_write(tmp_inode, end_of_file);
-
-	/* 512 bytes (2**9) is the fake blocksize that must be used */
-	/* for this calculation, not the real blocksize */
-		tmp_inode->i_blocks = (512 - 1 + num_of_bytes) >> 9;
-	}
-	spin_unlock(&tmp_inode->i_lock);
-
-	if (S_ISREG(tmp_inode->i_mode)) {
-		cFYI(1, ("File inode"));
-		tmp_inode->i_op = &cifs_file_inode_ops;
-
-		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
-			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-				tmp_inode->i_fop = &cifs_file_direct_nobrl_ops;
-			else
-				tmp_inode->i_fop = &cifs_file_direct_ops;
+	cifs_unix_info_to_inode(tmp_inode, pData, 1);
+	cifs_set_ops(tmp_inode);
 
-		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-			tmp_inode->i_fop = &cifs_file_nobrl_ops;
-		else
-			tmp_inode->i_fop = &cifs_file_ops;
+	if (!S_ISREG(tmp_inode->i_mode))
+		return;
 
-		if ((cifs_sb->tcon) && (cifs_sb->tcon->ses) &&
-		   (cifs_sb->tcon->ses->server->maxBuf <
-			PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE))
-			tmp_inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
-		else
-			tmp_inode->i_data.a_ops = &cifs_addr_ops;
+	/*
+	 * No sense invalidating pages for new inode
+	 * since we we have not started caching
+	 * readahead file data yet.
+	 */
+	if (isNewInode)
+		return;
 
-		if (isNewInode)
-			return; /* No sense invalidating pages for new inode
-				   since we we have not started caching
-				   readahead file data yet */
-
-		if (timespec_equal(&tmp_inode->i_mtime, &local_mtime) &&
-			(local_size == tmp_inode->i_size)) {
-			cFYI(1, ("inode exists but unchanged"));
-		} else {
-			/* file may have changed on server */
-			cFYI(1, ("invalidate inode, readdir detected change"));
-			invalidate_remote_inode(tmp_inode);
-		}
-	} else if (S_ISDIR(tmp_inode->i_mode)) {
-		cFYI(1, ("Directory inode"));
-		tmp_inode->i_op = &cifs_dir_inode_ops;
-		tmp_inode->i_fop = &cifs_dir_ops;
-	} else if (S_ISLNK(tmp_inode->i_mode)) {
-		cFYI(1, ("Symbolic Link inode"));
-		tmp_inode->i_op = &cifs_symlink_inode_ops;
-/* tmp_inode->i_fop = *//* do not need to set to anything */
+	if (timespec_equal(&tmp_inode->i_mtime, &local_mtime) &&
+		(local_size == tmp_inode->i_size)) {
+		cFYI(1, ("inode exists but unchanged"));
 	} else {
-		cFYI(1, ("Special inode"));
-		init_special_inode(tmp_inode, tmp_inode->i_mode,
-				   tmp_inode->i_rdev);
+		/* file may have changed on server */
+		cFYI(1, ("invalidate inode, readdir detected change"));
+		invalidate_remote_inode(tmp_inode);
 	}
 }
 

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

[Q]

-----Original Message-----
From: "Steve French" <smfrench@gmail.com>
To: "Christoph Hellwig" <hch@infradead.org>
Date: Fri, 15 Feb 2008 15:02:19 -0600
Subject: Re: [linux-cifs-client] review 5, was Re: projected date for mount.cifs to support DFS junction points

> 
> On 2/15/08, Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, Feb 15, 2008 at 07:37:35PM +0300, Q (Igor Mammedov) wrote:
> >  > Here is what I've done the last weekend.
> >  > Attached:
> >  >  fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
> Not merged yet.
> 
> >  >  fixed mixed case in struct member 0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)
> 
> Now merged into cifs-2.6.git
> 
> > The second one is trivially correct and should be pushed to Linus asap
> >  as small cleanup.  Patch 1 is not exactly what I had in mind, I was
> >  thinking about factoring out the common bits of cifs-cifs_get_inode_info
> >  and cifs-cifs_get_inode_info_unix to avoid having all the logic to
> >  set the various options twice.  I've attached two quick and untested
> >  patches showing what I mean.  I think in this case having the ifdef
> >  for that two line statement setting the inode operations here is fine.
> I reviewed and merged into cifs-2.6.git one of the two patches from
> Christoph (the cifs_set_ops one), but wanted to look more carefully at
> the other (cifs_get_info_remote) to make sure that buf was being freed
> in the cifs_get_inode_info path (otherwise it is fine).

At first glance cifs_get_inode_info_remote won't work cause it's old dfs
code not new one. But I caught what Christoph meant now, and will try to
rewrite it this way.

> >  But thinking about it I'm not even sure if it's good idea to make
> >  dfs support conditional.  Any reason it can't just be included
> >  unconditionally?
> It would make the code slightly smaller (perhaps useful someday for
> OLPC or embedded) and removes a piece of code that is not needed in
> all home networks (although DFS is useful even to some of these).  I
> lean toward removing the ifdef when it has made it through one or two
> more release cycles and is no longer experimental.   SInce there are a
> few experimental features (Kerberos and DFS) that are broadly useful -
> but not all users need both, I don't mind keeping the configure for
> each different for the short term but don't have a strong opinion on
> this.

IMHO, +1 to keeping DFS ifdefs for a while.

> -- 
> Thanks,
> 
> Steve
> 

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

[Christoph Hellwig]

On Sat, Feb 16, 2008 at 11:51:52AM +0300, Q wrote:
> At first glance cifs_get_inode_info_remote won't work cause it's old dfs
> code not new one. But I caught what Christoph meant now, and will try to
> rewrite it this way.

Yes, this was supposed to be a refactoring of the existing code.  By
doing your patch ontop of it you just have to replace the
cifs_get_inode_info_remote with the correct content :)

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

[Steve French]

The patch looks fine - but since it does not set obj_type any more - I
want to think about it a little more since it may be useful coming
back from the open path (although the mode is probably good enough).
jra added support to Samba for a new POSIX open/create/mkdir request
(which we only use for mkdir at the moment) - using  this call for
open (when the server indicates support for it as Samba does) would
cut the number of roundtrips to the server on these calls as it does
now for mkdir.

On Feb 15, 2008 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> If you like these kind of consolidation patches here's another one:
>
>
>



-- 
Thanks,

Steve

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

[Steve French]

On Fri, Feb 15, 2008 at 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> If you like these kind of consolidation patches here's another one:

Merged into cifs-2.6.git tree


-- 
Thanks,

Steve

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

[Q (Igor Mammedov)]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

Hi Steve,

Looked through inode.c code again and rewrote/simplified patch5 
See attachment for it.


Q (Igor Mammedov) wrote:
> Sorry guys, but I have a lot of work for the last 3 weeks,
> so I couldn't spare much time for a hobby and react quickly.
> 
 > Also I noticed that patch 2/5 is not completely applied yet. I'll send Steve
> interim patch I've made to make thing compiled and working.
> 
> Christoph Hellwig wrote:
>> +#ifdef CONFIG_CIFS_DFS_UPCALL
>> +			if (is_remote) {
>> +				inode->i_op =
>> +					&cifs_dfs_referral_inode_operations;
>> +				inode->i_fop = NULL;
>>
>> i_fop should never be set to NULL.  Just leave it alone so it stays
>> at &empty_fops.
>>
>> +#ifdef CONFIG_CIFS_DFS_UPCALL
>> +			if (is_remote) {
>> +				inode->i_op =
>> +					&cifs_dfs_referral_inode_operations;
>> +				inode->i_fop = NULL;
>> +			} else {
>> +				inode->i_op = &cifs_dir_inode_ops;
>> +				inode->i_fop = &cifs_dir_ops;
>> +			}
>> +#else
>>  			inode->i_op = &cifs_dir_inode_ops;
>>  			inode->i_fop = &cifs_dir_ops;
>> +#endif
>>
>> This code and everything surrounding it is duplicated in two functions.
>> Please refactor it into a common helper before adding new code to it.
>>
>> _______________________________________________
>> linux-cifs-client mailing list
>> linux-cifs-client@lists.samba.org
>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>
>> .
>>
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client


-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com





[-- Attachment #2: 0005-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch --]
[-- Type: text/x-patch, Size: 7994 bytes --]

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

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

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 24eb4d3..7f501e4 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -29,8 +29,9 @@
 #include "cifs_debug.h"
 #include "cifs_fs_sb.h"
 
-
-static void cifs_set_ops(struct inode *inode)
+#define PATH_IN_DFS 1
+#define PATH_NOT_IN_DFS 0
+static void cifs_set_ops(const int in_dfs, struct inode *inode)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 
@@ -57,8 +58,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 (in_dfs == PATH_IN_DFS) {
+			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 +158,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 +190,31 @@ 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 char *tmp_path = NULL;
+	const unsigned char *full_path;
+	int in_dfs = PATH_NOT_IN_DFS;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
+
+	full_path = cifs_get_search_path(pTcon, search_path);
+	tmp_path = full_path != search_path?full_path:NULL;
+
+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) && (in_dfs == PATH_NOT_IN_DFS)) {
+			in_dfs = PATH_IN_DFS;
+			full_path = search_path;
+			goto try_again_CIFSSMBUnixQPathInfo;
 		}
+		kfree(tmp_path);
+		return rc;
 	} else {
 		struct cifsInodeInfo *cifsInfo;
 		__u64 num_of_bytes = le64_to_cpu(findData.NumOfBytes);
@@ -204,8 +223,10 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 		/* get new inode */
 		if (*pinode == NULL) {
 			*pinode = new_inode(sb);
-			if (*pinode == NULL)
+			if (*pinode == NULL) {
+				kfree(tmp_path);
 				return -ENOMEM;
+			}
 			/* 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 +258,9 @@ 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(in_dfs, inode);
 	}
+	kfree(tmp_path);
 	return rc;
 }
 
@@ -353,9 +375,11 @@ 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 char *tmp_path = NULL;
+	const unsigned char *full_path;
 	char *buf = NULL;
 	int adjustTZ = FALSE;
+	int in_dfs = PATH_NOT_IN_DFS;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
@@ -373,8 +397,13 @@ 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);
+		tmp_path = full_path != search_path?full_path:NULL;
+
+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 +411,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 +420,14 @@ 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) && (in_dfs == PATH_NOT_IN_DFS)) {
+			in_dfs = PATH_IN_DFS;
+			full_path = search_path;
+			goto try_again_CIFSSMBQPathInfo;
 		}
+		kfree(tmp_path);
+		kfree(buf);
+		return rc;
 	} else {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 attr = le32_to_cpu(pfindData->Attributes);
@@ -424,6 +436,7 @@ int cifs_get_inode_info(struct inode **pinode,
 		if (*pinode == NULL) {
 			*pinode = new_inode(sb);
 			if (*pinode == NULL) {
+				kfree(tmp_path);
 				kfree(buf);
 				return -ENOMEM;
 			}
@@ -573,8 +586,9 @@ int cifs_get_inode_info(struct inode **pinode,
 			atomic_set(&cifsInfo->inUse, 1);
 		}
 
-		cifs_set_ops(inode);
+		cifs_set_ops(in_dfs, inode);
 	}
+	kfree(tmp_path);
 	kfree(buf);
 	return rc;
 }
@@ -804,7 +818,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(PATH_NOT_IN_DFS, tmp_inode);
 
 	if (!S_ISREG(tmp_inode->i_mode))
 		return;
-- 
1.5.3.7


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

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

[Christoph Hellwig]

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.

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

[Christoph Hellwig]

On Mon, Feb 25, 2008 at 02:25:50PM -0600, Steve French wrote:
> On Fri, Feb 15, 2008 at 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > If you like these kind of consolidation patches here's another one:
> 
> Merged into cifs-2.6.git tree

Okay, now that we have the basic consolidation in can I get you to
review force_uid_gid paramter to cifs_unix_info_to_inode?  It seems
more than fishy to me to ignore the CIFS_MOUNT_OVERR_{UID,GID} options
in cifs_get_inode_info_unix but not in posix_fill_in_inode.  This
seems more like a missed propagation of changes for a newly added
feature to me.  If not it should at least get some documentation.

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

[Q (Igor Mammedov)]

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

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

[Steve French]

Merged but with two corrections: initialized full_path to null on line
376 since it could be used unitialized in one path later in
cifs_get_inode_info and also fixed a spelling error.

Thanks.

On Sat, Mar 8, 2008 at 4:21 PM, Q (Igor Mammedov)
<qwerty0987654321@mail.ru> wrote:
> 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:





-- 
Thanks,

Steve

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

[Christoph Hellwig]

On Sun, Mar 09, 2008 at 01:21:13AM +0300, Q (Igor Mammedov) wrote:
> Thanks for your comments. Fixed patch is attached.

Thanks, this looks perfect.

Steve, did you merge this version or the previous one?

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

[Steve French]

On Mon, Mar 10, 2008 at 1:14 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Mar 09, 2008 at 01:21:13AM +0300, Q (Igor Mammedov) wrote:
>  > Thanks for your comments. Fixed patch is attached.
>
>  Thanks, this looks perfect.
>
>  Steve, did you merge this version or the previous one?

I merged the fixed patch with the two corrections mentioned (the
unitialized pointer and the spelling mistake).

-- 
Thanks,

Steve

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

[Steve French]

On Sat, Mar 8, 2008 at 1:43 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Feb 25, 2008 at 02:25:50PM -0600, Steve French wrote:
>  > On Fri, Feb 15, 2008 at 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
>  Okay, now that we have the basic consolidation in can I get you to
>  review force_uid_gid paramter to cifs_unix_info_to_inode?  It seems
>  more than fishy to me to ignore the CIFS_MOUNT_OVERR_{UID,GID} options
>  in cifs_get_inode_info_unix but not in posix_fill_in_inode.  This
>  seems more like a missed propagation of changes for a newly added
>  feature to me.  If not it should at least get some documentation.

Jeff Layton and I have been discussing the issue of which uid to use
to fill in the inode->i_uid and I am leaning toward changing the
default behavior (for the mount to Windows server case) and adding
another mount parm to allow users to get the older behavior if they
want.

Unfortunately there are many cases (and the uid/gid owner fields of
inodes can get filled in potentially in various places
mkdir/create/mknod and lookup and readdir and of course setattr).  The
mount could be to:
1) server which support returning uid/gid owner fields for an inode
(e.g. Samba) on QueryPathInfo
2) servers which would support returning a uid, but for which the uids
on the server don't match the client
3) servers like Windows which don't support returning the inodes uid

and for the latter we also have the case of files/directories for
which the user has chowned it so we know what uid the app thinks it
should be (or newly created files/directories)

Some of this is documented, and I have started a table which needs to
be added to the CIFS User's guide - but the case I am most worried
about at the moment is the behavior when the server would support the
Unix extensions - but the user has overridden the uid or gid
(specified on mount, perhaps because the server and client's uids
differ).   For this case should we always use the default uid - or
should an app be allowed to do a chmod and should we honor the uid/gid
passed in on the mkdir/create/mknod (as we do for the equivalent
windows case).




-- 
Thanks,

Steve

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

[Q (Igor Mammedov)]

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

Mem leak fix in inode.c

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-memory-leak.patch --]
[-- Type: text/x-patch; name=0001-Fix-memory-leak.patch, Size: 982 bytes --]

From beace64a9005ca4a2916c4cf19302a961597a52c Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Tue, 11 Mar 2008 12:02:42 +0300
Subject: [PATCH] Fix memory leak

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

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 4f0ee67..7e316f2 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -207,6 +207,8 @@ try_again_CIFSSMBUnixQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
+			if (full_path != search_path)
+				kfree(full_path);
 			full_path = search_path;
 			goto try_again_CIFSSMBUnixQPathInfo;
 		}
@@ -418,6 +420,8 @@ try_again_CIFSSMBQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
+			if (full_path != search_path)
+				kfree(full_path);
 			full_path = search_path;
 			goto try_again_CIFSSMBQPathInfo;
 		}
-- 
1.5.3.7


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

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

[Jeff Layton]

On Mon, 10 Mar 2008 22:34:35 -0500
"Steve French" <smfrench@gmail.com> wrote:

> On Sat, Mar 8, 2008 at 1:43 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Mon, Feb 25, 2008 at 02:25:50PM -0600, Steve French wrote:
> >  > On Fri, Feb 15, 2008 at 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >  Okay, now that we have the basic consolidation in can I get you to
> >  review force_uid_gid paramter to cifs_unix_info_to_inode?  It seems
> >  more than fishy to me to ignore the CIFS_MOUNT_OVERR_{UID,GID} options
> >  in cifs_get_inode_info_unix but not in posix_fill_in_inode.  This
> >  seems more like a missed propagation of changes for a newly added
> >  feature to me.  If not it should at least get some documentation.
> 
> Jeff Layton and I have been discussing the issue of which uid to use
> to fill in the inode->i_uid and I am leaning toward changing the
> default behavior (for the mount to Windows server case) and adding
> another mount parm to allow users to get the older behavior if they
> want.
> 
> Unfortunately there are many cases (and the uid/gid owner fields of
> inodes can get filled in potentially in various places
> mkdir/create/mknod and lookup and readdir and of course setattr).  The
> mount could be to:
>
> 1) server which support returning uid/gid owner fields for an inode
> (e.g. Samba) on QueryPathInfo
> 2) servers which would support returning a uid, but for which the uids
> on the server don't match the client
> 3) servers like Windows which don't support returning the inodes uid
> 
> and for the latter we also have the case of files/directories for
> which the user has chowned it so we know what uid the app thinks it
> should be (or newly created files/directories)
> 
> Some of this is documented, and I have started a table which needs to
> be added to the CIFS User's guide - but the case I am most worried
> about at the moment is the behavior when the server would support the
> Unix extensions - but the user has overridden the uid or gid
> (specified on mount, perhaps because the server and client's uids
> differ).   For this case should we always use the default uid - or
> should an app be allowed to do a chmod and should we honor the uid/gid
> passed in on the mkdir/create/mknod (as we do for the equivalent
> windows case).
> 

Here's what I'd like to see...

The best option here is to have a new upcall that does idmapping to map
windows RIDs to unix UIDs. It wouldn't be too hard to do and I have it
on my to-do list, but it's pretty far down and it'll be a while before
I can get started on it. Even with that though, we'll need sensible
defaults for when the upcall doesn't work for some reason.

In the meantime we have some pretty messy inconsistencies, particularly
when POSIX extensions aren't supported. CIFS sets the in-memory inode's
mode to be consistent with the mkdir/open call, but the ownership is
set to whatever the default uid/gid is for the mount. This makes some
apps work (at least for a little while), but causes other problems. For
instance, someone can create a directory with a restrictive mode but
then can't actually write to it because they don't own it.

This also seems scary to me from a security standpoint. We're basically
allowing someone to create a file with an arbitrary mode that is owned
by a different user. That user is generally root by default.

We have 2 separate but related pieces of info to deal with:

1) uid/gid: for this I'd like to see an idmapping upcall. When that
info isn't available (daemon is down or no mapping exists), then we'd
fall back to using the "default" uid/gid for the mount. This should be
the behavior regardless of whether we have unix extensions enabled or
not. Right now, we trust that when unix extensions are enabled that we 
have unix uid/gid mapped to the same things on all machines. This isn't
necessarily the case.

2) mode: for this we have 3 cases. When cifsacl is specified, we'd
use the mode obtained via them. If not, then when unix extensions are
supported, we should use the mode obtained via them. Otherwise, the mode
should be governed by the file_mode/dir_mode of the share.

At the same time, we should also consider tightening up the default
file_mode/dir_mode. Right now it's 02767 (I think). We should change
that to be 0700, and allow admins to loosen it if they wish.

The current approach of allowing users to have different info in
in-memory inodes vs. what's recorded on the server seems very
problematic to me. This is something that leads to non-deterministic
behavior and that's worse than just breaking some apps outright.

Just my 2 lumps of copper and nickel...
-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve French]

After looking at the memleak in more detail - I am not convinced that
we should be altering the path on all calls to QueryPathInfo when
SMB_SHARE_IS_IN_DFS

This seems excessive - only a small subset of the directories (or
files) in a share which is in the DFS namespace would require a second
lookup

On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
<qwerty0987654321@mail.ru> wrote:
> Mem leak fix in inode.c
>



-- 
Thanks,

Steve

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

[Q (Igor Mammedov)]

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

On Wed, Mar 12, 2008 at 1:14 AM, Steve French <smfrench@gmail.com> wrote:

> After looking at the memleak in more detail - I am not convinced that
> we should be altering the path on all calls to QueryPathInfo when
> SMB_SHARE_IS_IN_DFS
>

SNIA CIFS-TR 1.0 spec:
3.6. DFS Pathnames
A DFS pathname adheres to the standard described in the FileNames section. A
DFS enabled
client accessing a DFS share should set the Flags2 bit 12 in all name based
SMB requests
indicating to the server that the enclosed pathname should be resolved in
the Distributed File
System namespace. The pathname should always have the full file name,
including the server
name and share name. If the server can resolve the DFS name to a piece of
local storage, the
local storage will be accessed. If the server determines that the DFS name
actually maps to a
different server share, the access to the name will fail with the 32-bit
status
STATUS_PATH_NOT_COVERED (0xC0000257), or DOS error ERRsrv/ERRbadpath.


> This seems excessive - only a small subset of the directories (or
> files) in a share which is in the DFS namespace would require a second
> lookup
>

In short words:
We must send full UNC to get error -EREMOTE because server
will return OK otherwise and we will get root share inode instead.
This error is the only way to get the knowledge whether it is dfs
path or not. That's why we send full path name when the tree is in
DFS (SMB_SHARE_IS_IN_DFS).

On the other hand: the second call to QueryPathInfo, when we already
know that the path is in DFS, is a questionable  matter.
I attempt to fill "future mount point" inode with data returned by
server for "link" path.  Theoreticaly we can fill it with some static values
here and avoid the second call. The question is "what values?"


>
> On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
> <qwerty0987654321@mail.ru> wrote:
> > Mem leak fix in inode.c
> >
>
>
>
> --
> Thanks,
>
> Steve
>

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

[-- Attachment #2: 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

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

[simo]

On Tue, 2008-03-11 at 08:39 -0400, Jeff Layton wrote:
> On Mon, 10 Mar 2008 22:34:35 -0500
> "Steve French" <smfrench@gmail.com> wrote:
> 
> > On Sat, Mar 8, 2008 at 1:43 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > > On Mon, Feb 25, 2008 at 02:25:50PM -0600, Steve French wrote:
> > >  > On Fri, Feb 15, 2008 at 4:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > >  Okay, now that we have the basic consolidation in can I get you to
> > >  review force_uid_gid paramter to cifs_unix_info_to_inode?  It seems
> > >  more than fishy to me to ignore the CIFS_MOUNT_OVERR_{UID,GID} options
> > >  in cifs_get_inode_info_unix but not in posix_fill_in_inode.  This
> > >  seems more like a missed propagation of changes for a newly added
> > >  feature to me.  If not it should at least get some documentation.
> > 
> > Jeff Layton and I have been discussing the issue of which uid to use
> > to fill in the inode->i_uid and I am leaning toward changing the
> > default behavior (for the mount to Windows server case) and adding
> > another mount parm to allow users to get the older behavior if they
> > want.
> > 
> > Unfortunately there are many cases (and the uid/gid owner fields of
> > inodes can get filled in potentially in various places
> > mkdir/create/mknod and lookup and readdir and of course setattr).  The
> > mount could be to:
> >
> > 1) server which support returning uid/gid owner fields for an inode
> > (e.g. Samba) on QueryPathInfo
> > 2) servers which would support returning a uid, but for which the uids
> > on the server don't match the client
> > 3) servers like Windows which don't support returning the inodes uid
> > 
> > and for the latter we also have the case of files/directories for
> > which the user has chowned it so we know what uid the app thinks it
> > should be (or newly created files/directories)
> > 
> > Some of this is documented, and I have started a table which needs to
> > be added to the CIFS User's guide - but the case I am most worried
> > about at the moment is the behavior when the server would support the
> > Unix extensions - but the user has overridden the uid or gid
> > (specified on mount, perhaps because the server and client's uids
> > differ).   For this case should we always use the default uid - or
> > should an app be allowed to do a chmod and should we honor the uid/gid
> > passed in on the mkdir/create/mknod (as we do for the equivalent
> > windows case).
> > 
> 
> Here's what I'd like to see...
> 
> The best option here is to have a new upcall that does idmapping to map
> windows RIDs to unix UIDs. It wouldn't be too hard to do and I have it
> on my to-do list, but it's pretty far down and it'll be a while before
> I can get started on it. Even with that though, we'll need sensible
> defaults for when the upcall doesn't work for some reason.
> 
> In the meantime we have some pretty messy inconsistencies, particularly
> when POSIX extensions aren't supported. CIFS sets the in-memory inode's
> mode to be consistent with the mkdir/open call, but the ownership is
> set to whatever the default uid/gid is for the mount. This makes some
> apps work (at least for a little while), but causes other problems. For
> instance, someone can create a directory with a restrictive mode but
> then can't actually write to it because they don't own it.
> 
> This also seems scary to me from a security standpoint. We're basically
> allowing someone to create a file with an arbitrary mode that is owned
> by a different user. That user is generally root by default.
> 
> We have 2 separate but related pieces of info to deal with:
> 
> 1) uid/gid: for this I'd like to see an idmapping upcall. When that
> info isn't available (daemon is down or no mapping exists), then we'd
> fall back to using the "default" uid/gid for the mount. This should be
> the behavior regardless of whether we have unix extensions enabled or
> not. Right now, we trust that when unix extensions are enabled that we 
> have unix uid/gid mapped to the same things on all machines. This isn't
> necessarily the case.
> 
> 2) mode: for this we have 3 cases. When cifsacl is specified, we'd
> use the mode obtained via them. If not, then when unix extensions are
> supported, we should use the mode obtained via them. Otherwise, the mode
> should be governed by the file_mode/dir_mode of the share.
> 
> At the same time, we should also consider tightening up the default
> file_mode/dir_mode. Right now it's 02767 (I think). We should change
> that to be 0700, and allow admins to loosen it if they wish.
> 
> The current approach of allowing users to have different info in
> in-memory inodes vs. what's recorded on the server seems very
> problematic to me. This is something that leads to non-deterministic
> behavior and that's worse than just breaking some apps outright.
> 
> Just my 2 lumps of copper and nickel...

Jeff, I second this entirely,
also as I asked before I'd like to be able to pass SIDs even when Unix
Extensions are in use, and not pass UIDs.

Passing SIDs would allow us to do a double mapping (on client and on
server) that will free us from the need to have the same IDs on all
machines.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Senior Software Engineer at Red Hat Inc. <ssorce@redhat.com>

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

[Steve French]

In testing this (which I plan to push up to cifs-2.6.git today), I
have been running into problems with the retry that you do.  With Unix
Extensions enabled Samba does not seem to like the path (looks like a
bug in the server recognizing dfs paths in POSIX form with / instead
of \).

Disabling Unix Extensions to Samba (to look like Windows), I noticed
that you get a path_not_found error on the retry (ie
\\server\share\dfs-ref works but not the retry on \dfs-ref) - this
happens even if you turn off dfs support in SMB flags2 (Samba seems to
ignore that flag on QueryPathInfo).

The mem leak fix helps - but it looks like we have to put in default
values for the inode (which is soon to be covered anyway) - the one
question I had was how to generate an inode number when we are using
the server's inode numbers for the directory (since we can't seem to
get the server to give us info on the inode without a path not found
or a path not covered error coming back - since it is a dfs junction).

On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
<qwerty0987654321@mail.ru> wrote:
> Mem leak fix in inode.c
>



-- 
Thanks,

Steve

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

[Igor Mammedov]

Steve French wrote:
> In testing this (which I plan to push up to cifs-2.6.git today), I
> have been running into problems with the retry that you do.  With Unix
> Extensions enabled Samba does not seem to like the path (looks like a
> bug in the server recognizing dfs paths in POSIX form with / instead
> of \).

it will not work int this case cause samba tell that it is symlink,
and client happily show it as symlink. I dont think that we could fix
anything here on the client side because it must display symlinks with
unix ext. enabled. It can be fixed on the server side by hiding dfs links
from client (bug in samba).

> 
> Disabling Unix Extensions to Samba (to look like Windows), I noticed
> that you get a path_not_found error on the retry (ie
> \\server\share\dfs-ref works but not the retry on \dfs-ref) - this
> happens even if you turn off dfs support in SMB flags2 (Samba seems to
> ignore that flag on QueryPathInfo).
> 
> The mem leak fix helps - but it looks like we have to put in default
> values for the inode (which is soon to be covered anyway) - the one
> question I had was how to generate an inode number when we are using
> the server's inode numbers for the directory (since we can't seem to
> get the server to give us info on the inode without a path not found
> or a path not covered error coming back - since it is a dfs junction).

I've noticed that mountpoint inode_ino is always = 2. it doesn't get inode
number share root form  the server even if we use serverino mount option.

So userspace code will see ino=2 in junction point (mounted dfs refferal)
and will not see actual ino of the inode we are mounted on. 
May be we can use it for thinking up some default ino value for dfs junction
point inode?

Another way is to fix bug in samba so that it could return path info when
path is not in dfs format. (how else we can get server generated ino if 
server refuses to do it)



> 
> On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
> <qwerty0987654321@mail.ru> wrote:
>> Mem leak fix in inode.c
>>
> 
> 
> 


-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com