Discussion:
projected date for mount.cifs to support DFS junction points
(too old to reply)
Steve French
2008-01-10 20:28:40 UTC
Permalink
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-01-11 09:07:49 UTC
Permalink
Post by 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.
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.
Steve French
2008-01-11 16:05:04 UTC
Permalink
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.
Post by Christoph Hellwig
Post by 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.
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
Christoph Hellwig
2008-01-13 19:40:42 UTC
Permalink
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.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-01-13 21:26:31 UTC
Permalink
Post by 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 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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-01-13 19:48:46 UTC
Permalink
+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.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-01-13 21:35:45 UTC
Permalink
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.
Post by Christoph Hellwig
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-01-13 19:50:57 UTC
Permalink
+#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.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-01-13 20:19:03 UTC
Permalink
[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.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q (Igor Mammedov)
2008-01-14 13:15:05 UTC
Permalink
Post by 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]
+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.
Post by Christoph Hellwig
+ 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
Christoph Hellwig
2008-01-14 21:53:35 UTC
Permalink
This post might be inappropriate. Click to display it.
Christoph Hellwig
2008-01-13 20:21:44 UTC
Permalink
+#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.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q (Igor Mammedov)
2008-02-15 16:37:35 UTC
Permalink
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.
Post by 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.
_______________________________________________
linux-cifs-client mailing list
https://lists.samba.org/mailman/listinfo/linux-cifs-client
.
--
Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com
Christoph Hellwig
2008-02-15 17:05:52 UTC
Permalink
Post by Q (Igor Mammedov)
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.
Post by Q (Igor Mammedov)
Here is what I've done the last weekend.
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?
Steve French
2008-02-15 21:02:19 UTC
Permalink
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
Here is what I've done the last weekend.
fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
Not merged yet.
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
fixed mixed case in struct member 0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)
Now merged into cifs-2.6.git
Post by Christoph Hellwig
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).
Post by Christoph Hellwig
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
Christoph Hellwig
2008-02-15 22:11:58 UTC
Permalink
If you like these kind of consolidation patches here's another one:
Steve French
2008-02-19 04:51:41 UTC
Permalink
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.
--
Thanks,

Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-02-25 20:25:50 UTC
Permalink
Merged into cifs-2.6.git tree
--
Thanks,

Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-03-08 18:43:45 UTC
Permalink
Post by Steve French
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-03-11 03:34:35 UTC
Permalink
Post by Christoph Hellwig
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
2008-03-11 12:39:53 UTC
Permalink
On Mon, 10 Mar 2008 22:34:35 -0500
Post by Steve French
Post by Christoph Hellwig
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
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 <***@redhat.com>
simo
2008-03-17 03:14:55 UTC
Permalink
Post by Jeff Layton
On Mon, 10 Mar 2008 22:34:35 -0500
Post by Steve French
Post by Christoph Hellwig
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
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.
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 <***@samba.org>
Senior Software Engineer at Red Hat Inc. <***@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q
2008-02-16 08:51:52 UTC
Permalink
-----Original Message-----
From: "Steve French" <***@gmail.com>
To: "Christoph Hellwig" <***@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
Post by Steve French
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
Here is what I've done the last weekend.
fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
Not merged yet.
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
fixed mixed case in struct member 0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)
Now merged into cifs-2.6.git
Post by Christoph Hellwig
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.
Post by Steve French
Post by Christoph Hellwig
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.
Post by Steve French
--
Thanks,
Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-02-16 13:32:13 UTC
Permalink
Post by Q
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 :)

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q (Igor Mammedov)
2008-03-04 12:38:50 UTC
Permalink
Hi Steve,

Looked through inode.c code again and rewrote/simplified patch5
See attachment for it.
Post by Q (Igor Mammedov)
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.
Post by 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.
_______________________________________________
linux-cifs-client mailing list
https://lists.samba.org/mailman/listinfo/linux-cifs-client
.
------------------------------------------------------------------------
_______________________________________________
linux-cifs-client mailing list
https://lists.samba.org/mailman/listinfo/linux-cifs-client
--
Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com
Christoph Hellwig
2008-03-08 18:41:27 UTC
Permalink
Post by Q (Igor Mammedov)
Hi Steve,
Looked through inode.c code again and rewrote/simplified patch5
See attachment for it.
From 03c5dcdd566f59240de7843e0a4dd3c3468a0ace Mon Sep 17 00:00:00 2001
From: Igor Mammedov <***@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.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q (Igor Mammedov)
2008-03-08 22:21:13 UTC
Permalink
Thanks for your comments. Fixed patch is attached.
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
Hi Steve,
Looked through inode.c code again and rewrote/simplified patch5
See attachment for it.
From 03c5dcdd566f59240de7843e0a4dd3c3468a0ace Mon Sep 17 00:00:00 2001
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) {
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.
Steve French
2008-03-09 03:49:35 UTC
Permalink
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)
Post by Q (Igor Mammedov)
Thanks for your comments. Fixed patch is attached.
Post by Q (Igor Mammedov)
Hi Steve,
Looked through inode.c code again and rewrote/simplified patch5
See attachment for it.
--
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-03-10 06:14:34 UTC
Permalink
Post by Q (Igor Mammedov)
Thanks for your comments. Fixed patch is attached.
Thanks, this looks perfect.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-03-10 16:20:44 UTC
Permalink
Post by Christoph Hellwig
Post by Q (Igor Mammedov)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q (Igor Mammedov)
2008-03-11 09:41:14 UTC
Permalink
Mem leak fix in inode.c
Steve French
2008-03-11 22:14:28 UTC
Permalink
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)
Post by Q (Igor Mammedov)
Mem leak fix in inode.c
--
Thanks,

Steve
Q (Igor Mammedov)
2008-03-12 09:28:55 UTC
Permalink
Post by 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
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.
Post by Steve French
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?"
Post by Steve French
On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
Post by Q (Igor Mammedov)
Mem leak fix in inode.c
--
Thanks,
Steve
Steve French
2008-03-22 22:48:20 UTC
Permalink
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)
Post by Q (Igor Mammedov)
Mem leak fix in inode.c
--
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Igor Mammedov
2008-04-18 16:40:59 UTC
Permalink
Post by 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 \).
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).
Post by Steve French
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)
Post by Steve French
On Tue, Mar 11, 2008 at 4:41 AM, Q (Igor Mammedov)
Post by Q (Igor Mammedov)
Mem leak fix in inode.c
--
Best regards,

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




--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-02-06 04:07:40 UTC
Permalink
Post by Christoph Hellwig
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.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-02-06 13:43:01 UTC
Permalink
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.
Post by Christoph Hellwig
Post by Christoph Hellwig
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2008-02-07 18:25:52 UTC
Permalink
Post by 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.
- 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-02-07 23:30:30 UTC
Permalink
Post by Christoph Hellwig
Post by 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.
- 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.
Post by Christoph Hellwig
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.
Post by Christoph Hellwig
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.
Post by Christoph Hellwig
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve French
2008-02-08 05:27:42 UTC
Permalink
Post by Christoph Hellwig
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Continue reading on narkive:
Loading...