[linux-cifs-client] Re: [PATCH] cifs: fix regression in DFS referral option parsing code

Jeff Layton jlayton at redhat.com
Fri Jan 2 15:34:05 GMT 2009


On Fri, 2 Jan 2009 01:17:09 +0300
"Q (Igor Mammedov)" <niallain at gmail.com> wrote:

> On Wed, Dec 10, 2008 at 10:14 PM, Jeff Layton <jlayton at redhat.com> wrote:
> > I believe this patch:
> >
> >    commit 2c55608f28444c3f33b10312881384c470ceed56
> >    Author: Igor Mammedov <niallain at gmail.com>
> >    Date:   Thu Oct 23 13:58:42 2008 +0400
> >
> >        Fixed parsing of mount options when doing DFS submount
> >
> > ...has caused a regression in the DFS referral handling code. I have the
> > following mounted:
> >
> > //hostname/dfsroot
> >
> > ...in this DFS root, I have a dfs referral link that points to:
> >
> > msdfs:hostname.foo.bar\export
> >
> > ...when I try to traverse this link, compose_mount_options attaches a
> > prefixpath to the options, even though one shouldn't be there. The
> > problem is that strlen(fullpath) is != to ref->path_consumed in this
> > situation so we end up trying to build a prefixpath out of nothing.
> >
> > I'm not convinced here that trying to work with path_consumed is at all
> > correct. Why shouldn't we just try to walk the string starting after the
> > sharename and use whatever is there as the prefixpath? The following
> > patch seems simpler to me and seems to fix the regressions I've seen in
> > my setup.
> >
> > Is there some reason we need to use the more complex scheme that was
> > introduced by the patch above?
> 
> Probably there is.
> I've added it to handle next case:
> 
> mounting remote DFSroot
> \\A\share\my\path\here
> as dfs referral for this path server returns
> \\B\share\disk1
> and server consumed
> \\A\share\my   part
> so new prefixpath should be \disk1\path\here
> I guess that why we should to take into account path_consumed field.
> 

Thanks Igor...

Steve ended up clarifying why we need to take path_consumed into
account. I think the problem here was ultimately due to us sending a
DFS request with a double backlash prefix. For some strange reason, DFS
paths are expected to have a single backslash prefix. There's a patch
now in Steve's tree that fixes this the right way, I think.

> >
> > Long term, I think we need to consider adding some generic functions
> > to do UNC parsing. That would be helpful in the standard cifs_mount
> > codepath as well and would allow us to make a robust function that
> > can handle different UNC variations.
> +1
> it'll save some pain in ...
> 
> PS:
> More over. The whole business with prefixpath "mount option" was that it
> is easier to parse complex URI in mount helper that in kernel. And it is
> done this way now. (correct me if I'm wrong)
> 
> But with DFS we had to parse URI in kernel any way and compose prefixpath
> mount option there too.
> So I'd like to propose make it depricated and use a full URI instead of a pair
> URI + prefixpath in mount options. This way we will extract prepath for sb only
> once in cifs_mount.
> 
> From dev point of view:
> That will save several code lines when handling dfs submounts and simplify code
> to some degree.
> 
> From users point of view:
> mount //srv/share/whaterver /mnt
> looks  better than:
> mount //srv/share /mnt -o prefixpath=whaterver
> 

I like that idea. It should also simplify the mount helper.

-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list