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

[Q (Igor Mammedov)]

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.

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

>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> ---
>  fs/cifs/cifs_dfs_ref.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index e1c1836..aa4feef 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -128,7 +128,6 @@ static char *compose_mount_options(const char *sb_mountdata,
>        char *srvIP = NULL;
>        char sep = ',';
>        int off, noff;
> -       char *fullpath;
>
>        if (sb_mountdata == NULL)
>                return ERR_PTR(-EINVAL);
> @@ -200,16 +199,13 @@ static char *compose_mount_options(const char *sb_mountdata,
>        if (tkn_e == NULL) /* invalid unc, missing share name*/
>                goto compose_mount_options_out;
>
> -       fullpath = build_path_from_dentry(dentry);
>        tkn_e = strchr(tkn_e + 1, '\\');
> -       if (tkn_e || strlen(fullpath) - (ref->path_consumed)) {
> +       if (tkn_e) {
>                strncat(mountdata, &sep, 1);
>                strcat(mountdata, "prefixpath=");
>                if (tkn_e)
>                        strcat(mountdata, tkn_e + 1);
> -               strcat(mountdata, fullpath + (ref->path_consumed));
>        }
> -       kfree(fullpath);
>
>        /*cFYI(1,("%s: parent mountdata: %s", __func__,sb_mountdata));*/
>        /*cFYI(1, ("%s: submount mountdata: %s", __func__, mountdata ));*/
> --
> 1.5.5.1
>
>